Skip to content

Commit

Permalink
Editions: Stop propagating partially resolved feature sets to plugins.
Browse files Browse the repository at this point in the history 
Since protoc can't know the full set of features that matter to a plugin, sending partially resolved features is unnecessarily confusing.  Plugins written in C++ will have automatic access to the fully resolved features via previous enhancements.  Plugins written in other languages will need to reimplement feature resolution logic.  Future changes will document this algorithm and provide conformance testing.

PiperOrigin-RevId: 557969789
  • Loading branch information
@mkruskal-google @copybara-github
mkruskal-google authored and copybara-github committed Aug 17, 2023
1 parent 52b571f commit b66ef4c
Show file tree
Hide file tree
Showing 7 changed files with 10 additions and 211 deletions.
10 changes: 0 additions & 10 deletions src/google/protobuf/compiler/code_generator.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,16 +145,6 @@ class PROTOC_EXPORT CodeGenerator {
static const FeatureSet& GetSourceRawFeatures(const DescriptorT& desc) {
return ::google::protobuf::internal::InternalFeatureHelper::GetRawFeatures(desc);
}

// Converts a FileDescriptor to a FileDescriptorProto suitable for passing off
// to a runtime. Notably, this strips all source-retention options and
// includes both raw and resolved features.
static FileDescriptorProto GetRuntimeProto(const FileDescriptor& file) {
FileDescriptorProto proto =
::google::protobuf::internal::InternalFeatureHelper::GetGeneratorProto(file);
StripSourceRetentionOptions(*file.pool(), proto);
return proto;
}
};

// CodeGenerators generate one or more files in a given directory. This
Expand Down
84 changes: 0 additions & 84 deletions src/google/protobuf/compiler/code_generator_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ class TestGenerator : public CodeGenerator {
}

// Expose the protected methods for testing.
using CodeGenerator::GetRuntimeProto;
using CodeGenerator::GetSourceFeatures;
using CodeGenerator::GetSourceRawFeatures;
};
Expand Down Expand Up @@ -224,89 +223,6 @@ TEST_F(CodeGeneratorTest, GetSourceFeaturesInherited) {
EXPECT_EQ(ext.string_source_feature(), "field");
}

TEST_F(CodeGeneratorTest, GetRuntimeProtoTrivial) {
auto file = BuildFile(R"schema(
edition = "2023";
package protobuf_unittest;
)schema");
ASSERT_THAT(file, NotNull());

FileDescriptorProto proto = TestGenerator::GetRuntimeProto(*file);
const FeatureSet& features = proto.options().features();

EXPECT_TRUE(features.has_raw_features());
EXPECT_THAT(features.raw_features(), EqualsProto(R"pb()pb"));
}

TEST_F(CodeGeneratorTest, GetRuntimeProtoRoot) {
auto file = BuildFile(R"schema(
edition = "2023";
package protobuf_unittest;
import "google/protobuf/unittest_features.proto";
option features.enum_type = CLOSED;
option features.(pb.test).int_source_feature = 5;
option features.(pb.test).int_file_feature = 6;
)schema");
ASSERT_THAT(file, NotNull());

FileDescriptorProto proto = TestGenerator::GetRuntimeProto(*file);
const FeatureSet& features = proto.options().features();
const pb::TestFeatures& ext = features.GetExtension(pb::test);

EXPECT_THAT(features.raw_features(),
EqualsProto(R"pb(enum_type: CLOSED
[pb.test] { int_file_feature: 6 })pb"));
EXPECT_EQ(features.enum_type(), FeatureSet::CLOSED);
EXPECT_TRUE(features.has_field_presence());
EXPECT_EQ(features.field_presence(), FeatureSet::EXPLICIT);

EXPECT_FALSE(ext.has_int_source_feature());
EXPECT_EQ(ext.int_file_feature(), 6);
}

TEST_F(CodeGeneratorTest, GetRuntimeProtoInherited) {
auto file = BuildFile(R"schema(
edition = "2023";
package protobuf_unittest;
import "google/protobuf/unittest_features.proto";
option features.enum_type = CLOSED;
option features.(pb.test).int_source_feature = 5;
option features.(pb.test).int_file_feature = 6;
message EditionsMessage {
option features.(pb.test).int_message_feature = 7;
option features.(pb.test).int_multiple_feature = 8;
string field = 1 [
features.field_presence = IMPLICIT,
features.(pb.test).int_multiple_feature = 9,
features.(pb.test).string_source_feature = "field"
];
}
)schema");
ASSERT_THAT(file, NotNull());

FileDescriptorProto proto = TestGenerator::GetRuntimeProto(*file);
const FieldDescriptorProto& field = proto.message_type(0).field(0);
const FeatureSet& features = field.options().features();
const pb::TestFeatures& ext = features.GetExtension(pb::test);

EXPECT_THAT(features.raw_features(), google::protobuf::EqualsProto(R"pb(
field_presence: IMPLICIT
[pb.test] { int_multiple_feature: 9 }
)pb"));
EXPECT_EQ(features.enum_type(), FeatureSet::CLOSED);
EXPECT_EQ(features.field_presence(), FeatureSet::IMPLICIT);
EXPECT_EQ(ext.int_multiple_feature(), 9);
EXPECT_EQ(ext.int_message_feature(), 7);
EXPECT_EQ(ext.int_file_feature(), 6);
EXPECT_FALSE(ext.has_int_source_feature());
EXPECT_FALSE(ext.has_string_source_feature());
}

} // namespace
} // namespace compiler
} // namespace protobuf
Expand Down
6 changes: 2 additions & 4 deletions src/google/protobuf/compiler/command_line_interface.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -344,8 +344,7 @@ void CommandLineInterface::GetTransitiveDependencies(

// Add this file.
FileDescriptorProto* new_descriptor = output->Add();
*new_descriptor =
google::protobuf::internal::InternalFeatureHelper::GetGeneratorProto(*file);
file->CopyTo(new_descriptor);
if (options.include_source_code_info) {
file->CopySourceCodeInfoTo(new_descriptor);
}
Expand Down Expand Up @@ -2632,8 +2631,7 @@ bool CommandLineInterface::GeneratePluginOutput(
if (files_to_generate.contains(file_proto.name())) {
const FileDescriptor* file = pool->FindFileByName(file_proto.name());
*request.add_source_file_descriptors() = std::move(file_proto);
file_proto =
google::protobuf::internal::InternalFeatureHelper::GetGeneratorProto(*file);
file->CopyTo(&file_proto);
file->CopySourceCodeInfoTo(&file_proto);
file->CopyJsonNameTo(&file_proto);
StripSourceRetentionOptions(*file->pool(), file_proto);
Expand Down
39 changes: 7 additions & 32 deletions src/google/protobuf/compiler/command_line_interface_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1557,33 +1557,13 @@ TEST_F(CommandLineInterfaceTest, Plugin_RuntimeFeatures) {

EXPECT_THAT(
request.proto_file(0).message_type(0).field(0).options().features(),
EqualsProto(R"pb(field_presence: IMPLICIT
enum_type: OPEN
repeated_field_encoding: PACKED
message_encoding: LENGTH_PREFIXED
json_format: ALLOW
raw_features { field_presence: IMPLICIT }
[pb.cpp] {
legacy_closed_enum: false
utf8_validation: VERIFY_PARSE
}
)pb"));
EqualsProto(R"pb(field_presence: IMPLICIT)pb"));
EXPECT_THAT(request.source_file_descriptors(0)
.message_type(0)
.field(0)
.options()
.features(),
EqualsProto(R"pb(field_presence: IMPLICIT
enum_type: OPEN
repeated_field_encoding: PACKED
message_encoding: LENGTH_PREFIXED
json_format: ALLOW
raw_features { field_presence: IMPLICIT }
[pb.cpp] {
legacy_closed_enum: false
utf8_validation: VERIFY_PARSE
}
)pb"));
EqualsProto(R"pb(field_presence: IMPLICIT)pb"));
}

TEST_F(CommandLineInterfaceTest, Plugin_SourceFeatures) {
Expand Down Expand Up @@ -1629,10 +1609,8 @@ TEST_F(CommandLineInterfaceTest, Plugin_SourceFeatures) {
ASSERT_EQ(request.proto_file(2).name(), "foo.proto");
const FeatureSet& features =
request.proto_file(2).message_type(0).field(0).options().features();
EXPECT_THAT(features.raw_features(),
EXPECT_THAT(features,
EqualsProto(R"pb([pb.test] { int_field_feature: 99 })pb"));
EXPECT_FALSE(features.GetExtension(pb::test).has_int_source_feature());
EXPECT_EQ(features.GetExtension(pb::test).int_field_feature(), 99);
}

{
Expand All @@ -1642,13 +1620,10 @@ TEST_F(CommandLineInterfaceTest, Plugin_SourceFeatures) {
.field(0)
.options()
.features();
EXPECT_THAT(features.raw_features(),
EqualsProto(R"pb([pb.test] {
int_field_feature: 99
int_source_feature: 87
})pb"));
EXPECT_EQ(features.GetExtension(pb::test).int_field_feature(), 99);
EXPECT_EQ(features.GetExtension(pb::test).int_source_feature(), 87);
EXPECT_THAT(features, EqualsProto(R"pb([pb.test] {
int_field_feature: 99
int_source_feature: 87
})pb"));
}
}

Expand Down
30 changes: 1 addition & 29 deletions src/google/protobuf/descriptor.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2746,27 +2746,6 @@ FileDescriptor::FileDescriptor() {}

// CopyTo methods ====================================================

namespace internal {
FileDescriptorProto InternalFeatureHelper::GetGeneratorProto(
const FileDescriptor& file) {
FileDescriptorProto file_proto;
file.CopyTo(&file_proto);

// Insert all the raw features back into the proto.
internal::VisitDescriptors(
file, file_proto, [](const auto& desc, auto& proto) {
const auto& features = GetFeatures(desc);
if (&features != &FeatureSet::default_instance() &&
!IsLegacyFeatureSet(features)) {
*proto.mutable_options()->mutable_features() = features;
*proto.mutable_options()->mutable_features()->mutable_raw_features() =
GetRawFeatures(desc);
}
});
return file_proto;
}
} // namespace internal

void FileDescriptor::CopyTo(FileDescriptorProto* proto) const {
CopyHeadingTo(proto);

Expand Down Expand Up @@ -5315,14 +5294,7 @@ void DescriptorBuilder::ResolveFeaturesImpl(
// internal details.
FeatureSet* mutable_features = alloc.AllocateArray<FeatureSet>(1);
descriptor->proto_features_ = mutable_features;
if (options->features().has_raw_features()) {
// If the raw features are specified, use those and recalculate the
// resolved features.
options->mutable_features()->mutable_raw_features()->Swap(
mutable_features);
} else {
options->mutable_features()->Swap(mutable_features);
}
options->mutable_features()->Swap(mutable_features);
options->clear_features();
} else if (!force_merge) {
// Nothing to merge, and we aren't forcing it.
Expand Down
5 changes: 0 additions & 5 deletions src/google/protobuf/descriptor.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -284,11 +284,6 @@ class PROTOBUF_EXPORT InternalFeatureHelper {
static const FeatureSet& GetRawFeatures(const DescriptorT& desc) {
return *desc.proto_features_;
}

// Provides the full descriptor tree including both resolved features (in the
// `features` fields) and unresolved features (in the `raw_features` fields)
// for every descriptor.
static FileDescriptorProto GetGeneratorProto(const FileDescriptor& file);
};

} // namespace internal
Expand Down
47 changes: 0 additions & 47 deletions src/google/protobuf/descriptor_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9633,53 +9633,6 @@ TEST_F(FeaturesTest, UninterpretedOptionsMergeExtension) {
9);
}

TEST_F(FeaturesTest, RawFeatures) {
BuildDescriptorMessagesInTestPool();
const FileDescriptor* file = BuildFile(R"pb(
name: "foo.proto"
syntax: "editions"
edition: "2023"
options { features { raw_features { field_presence: IMPLICIT } } }
)pb");
EXPECT_THAT(file->options(), EqualsProto(""));
EXPECT_THAT(GetCoreFeatures(file), EqualsProto(R"pb(
field_presence: IMPLICIT
enum_type: OPEN
repeated_field_encoding: PACKED
message_encoding: LENGTH_PREFIXED
json_format: ALLOW
[pb.cpp] {
legacy_closed_enum: false
utf8_validation: VERIFY_PARSE
})pb"));
}

TEST_F(FeaturesTest, RawFeaturesConflict) {
BuildDescriptorMessagesInTestPool();
const FileDescriptor* file = BuildFile(R"pb(
name: "foo.proto"
syntax: "editions"
edition: "2023"
options {
features {
enum_type: CLOSED
raw_features { field_presence: IMPLICIT }
}
}
)pb");
EXPECT_THAT(file->options(), EqualsProto(""));
EXPECT_THAT(GetCoreFeatures(file), EqualsProto(R"pb(
field_presence: IMPLICIT
enum_type: OPEN
repeated_field_encoding: PACKED
message_encoding: LENGTH_PREFIXED
json_format: ALLOW
[pb.cpp] {
legacy_closed_enum: false
utf8_validation: VERIFY_PARSE
})pb"));
}

TEST_F(FeaturesTest, InvalidJsonUniquenessDefaultWarning) {
BuildFileWithWarnings(
R"pb(
Expand Down

0 comments on commit b66ef4c

Please sign in to comment.