Skip to content

Commit 8c5f3a7

Browse files
Prohibit using features in the same file they're defined in.
This is an edge case we can't handle properly today. Rather than allowing poorly defined behavior, we'll make this an error condition until we can actually support it. In the future, it may be necessary to upgrade feature files to newer editions. Closes #16756 PiperOrigin-RevId: 634122584
1 parent 615e704 commit 8c5f3a7

File tree

2 files changed

+70
-16
lines changed

2 files changed

+70
-16
lines changed

src/google/protobuf/descriptor.cc

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1110,19 +1110,6 @@ void RestoreFeaturesToOptions(const FeatureSet* features, ProtoT* proto) {
11101110
}
11111111
}
11121112

1113-
template <typename OptionsT>
1114-
bool HasFeatures(const OptionsT& options) {
1115-
if (options.has_features()) return true;
1116-
1117-
for (const auto& opt : options.uninterpreted_option()) {
1118-
if (opt.name_size() > 0 && opt.name(0).name_part() == "features" &&
1119-
!opt.name(0).is_extension()) {
1120-
return true;
1121-
}
1122-
}
1123-
return false;
1124-
}
1125-
11261113
template <typename DescriptorT>
11271114
absl::string_view GetFullName(const DescriptorT& desc) {
11281115
return desc.full_name();
@@ -8774,6 +8761,19 @@ bool DescriptorBuilder::OptionInterpreter::InterpretSingleOption(
87748761
// accumulate field numbers to form path to interpreted option
87758762
dest_path.push_back(field->number());
87768763

8764+
// Special handling to prevent feature use in the same file as the
8765+
// definition.
8766+
// TODO Add proper support for cases where this can work.
8767+
if (field->file() == builder_->file_ &&
8768+
uninterpreted_option_->name(0).name_part() == "features" &&
8769+
!uninterpreted_option_->name(0).is_extension()) {
8770+
return AddNameError([&] {
8771+
return absl::StrCat(
8772+
"Feature \"", debug_msg_name,
8773+
"\" can't be used in the same file it's defined in.");
8774+
});
8775+
}
8776+
87778777
if (i < uninterpreted_option_->name_size() - 1) {
87788778
if (field->cpp_type() != FieldDescriptor::CPPTYPE_MESSAGE) {
87798779
return AddNameError([&] {

src/google/protobuf/descriptor_unittest.cc

Lines changed: 57 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4059,8 +4059,8 @@ class ValidationErrorTest : public testing::Test {
40594059
return ABSL_DIE_IF_NULL(pool_.BuildFile(file_proto));
40604060
}
40614061

4062-
const FileDescriptor* ParseAndBuildFile(absl::string_view file_name,
4063-
absl::string_view file_text) {
4062+
FileDescriptorProto ParseFile(absl::string_view file_name,
4063+
absl::string_view file_text) {
40644064
io::ArrayInputStream input_stream(file_text.data(), file_text.size());
40654065
SimpleErrorCollector error_collector;
40664066
io::Tokenizer tokenizer(&input_stream, &error_collector);
@@ -4072,7 +4072,12 @@ class ValidationErrorTest : public testing::Test {
40724072
<< file_text;
40734073
ABSL_CHECK_EQ("", error_collector.last_error());
40744074
proto.set_name(file_name);
4075-
return pool_.BuildFile(proto);
4075+
return proto;
4076+
}
4077+
4078+
const FileDescriptor* ParseAndBuildFile(absl::string_view file_name,
4079+
absl::string_view file_text) {
4080+
return pool_.BuildFile(ParseFile(file_name, file_text));
40764081
}
40774082

40784083

@@ -4096,6 +4101,17 @@ class ValidationErrorTest : public testing::Test {
40964101
BuildFileWithErrors(file_proto, expected_errors);
40974102
}
40984103

4104+
// Parse a proto file and build it. Expect errors to be produced which match
4105+
// the given error text.
4106+
void ParseAndBuildFileWithErrors(absl::string_view file_name,
4107+
absl::string_view file_text,
4108+
absl::string_view expected_errors) {
4109+
MockErrorCollector error_collector;
4110+
EXPECT_TRUE(pool_.BuildFileCollectingErrors(ParseFile(file_name, file_text),
4111+
&error_collector) == nullptr);
4112+
EXPECT_EQ(expected_errors, error_collector.text_);
4113+
}
4114+
40994115
// Parse file_text as a FileDescriptorProto in text format and add it
41004116
// to the DescriptorPool. Expect errors to be produced which match the
41014117
// given warning text.
@@ -10283,6 +10299,44 @@ TEST_F(FeaturesTest, InvalidOpenEnumNonZeroFirstValue) {
1028310299
"enums.\n");
1028410300
}
1028510301

10302+
TEST_F(FeaturesTest, InvalidUseFeaturesInSameFile) {
10303+
BuildDescriptorMessagesInTestPool();
10304+
ParseAndBuildFileWithErrors("foo.proto", R"schema(
10305+
edition = "2023";
10306+
10307+
package test;
10308+
import "google/protobuf/descriptor.proto";
10309+
10310+
message Foo {
10311+
string bar = 1 [
10312+
features.(test.custom).foo = "xyz",
10313+
features.(test.another) = {foo: -321}
10314+
];
10315+
}
10316+
10317+
message Custom {
10318+
string foo = 1 [features = { [test.custom]: {foo: "abc"} }];
10319+
}
10320+
message Another {
10321+
Enum foo = 1;
10322+
}
10323+
10324+
enum Enum {
10325+
option features.enum_type = CLOSED;
10326+
ZERO = 0;
10327+
ONE = 1;
10328+
}
10329+
10330+
extend google.protobuf.FeatureSet {
10331+
Custom custom = 1002 [features.message_encoding=DELIMITED];
10332+
Another another = 1001;
10333+
}
10334+
)schema",
10335+
"foo.proto: test.Foo.bar: OPTION_NAME: Feature "
10336+
"\"features.(test.custom)\" can't be used in the "
10337+
"same file it's defined in.\n");
10338+
}
10339+
1028610340
TEST_F(FeaturesTest, ClosedEnumNonZeroFirstValue) {
1028710341
BuildDescriptorMessagesInTestPool();
1028810342
const FileDescriptor* file = BuildFile(

0 commit comments

Comments
 (0)