Skip to content

Commit

Permalink
Prohibit using features in the same file they're defined in.
Browse files Browse the repository at this point in the history
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: 634512378
  • Loading branch information
mkruskal-google authored and copybara-github committed May 16, 2024
1 parent baeab50 commit 24b91a7
Show file tree
Hide file tree
Showing 2 changed files with 70 additions and 16 deletions.
26 changes: 13 additions & 13 deletions src/google/protobuf/descriptor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1110,19 +1110,6 @@ void RestoreFeaturesToOptions(const FeatureSet* features, ProtoT* proto) {
}
}

template <typename OptionsT>
bool HasFeatures(const OptionsT& options) {
if (options.has_features()) return true;

for (const auto& opt : options.uninterpreted_option()) {
if (opt.name_size() > 0 && opt.name(0).name_part() == "features" &&
!opt.name(0).is_extension()) {
return true;
}
}
return false;
}

template <typename DescriptorT>
absl::string_view GetFullName(const DescriptorT& desc) {
return desc.full_name();
Expand Down Expand Up @@ -8784,6 +8771,19 @@ bool DescriptorBuilder::OptionInterpreter::InterpretSingleOption(
// accumulate field numbers to form path to interpreted option
dest_path.push_back(field->number());

// Special handling to prevent feature use in the same file as the
// definition.
// TODO Add proper support for cases where this can work.
if (field->file() == builder_->file_ &&
uninterpreted_option_->name(0).name_part() == "features" &&
!uninterpreted_option_->name(0).is_extension()) {
return AddNameError([&] {
return absl::StrCat(
"Feature \"", debug_msg_name,
"\" can't be used in the same file it's defined in.");
});
}

if (i < uninterpreted_option_->name_size() - 1) {
if (field->cpp_type() != FieldDescriptor::CPPTYPE_MESSAGE) {
return AddNameError([&] {
Expand Down
60 changes: 57 additions & 3 deletions src/google/protobuf/descriptor_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4059,8 +4059,8 @@ class ValidationErrorTest : public testing::Test {
return ABSL_DIE_IF_NULL(pool_.BuildFile(file_proto));
}

const FileDescriptor* ParseAndBuildFile(absl::string_view file_name,
absl::string_view file_text) {
FileDescriptorProto ParseFile(absl::string_view file_name,
absl::string_view file_text) {
io::ArrayInputStream input_stream(file_text.data(), file_text.size());
SimpleErrorCollector error_collector;
io::Tokenizer tokenizer(&input_stream, &error_collector);
Expand All @@ -4072,7 +4072,12 @@ class ValidationErrorTest : public testing::Test {
<< file_text;
ABSL_CHECK_EQ("", error_collector.last_error());
proto.set_name(file_name);
return pool_.BuildFile(proto);
return proto;
}

const FileDescriptor* ParseAndBuildFile(absl::string_view file_name,
absl::string_view file_text) {
return pool_.BuildFile(ParseFile(file_name, file_text));
}


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

// Parse a proto file and build it. Expect errors to be produced which match
// the given error text.
void ParseAndBuildFileWithErrors(absl::string_view file_name,
absl::string_view file_text,
absl::string_view expected_errors) {
MockErrorCollector error_collector;
EXPECT_TRUE(pool_.BuildFileCollectingErrors(ParseFile(file_name, file_text),
&error_collector) == nullptr);
EXPECT_EQ(expected_errors, error_collector.text_);
}

// Parse file_text as a FileDescriptorProto in text format and add it
// to the DescriptorPool. Expect errors to be produced which match the
// given warning text.
Expand Down Expand Up @@ -10354,6 +10370,44 @@ TEST_F(FeaturesTest, InvalidOpenEnumNonZeroFirstValue) {
"enums.\n");
}

TEST_F(FeaturesTest, InvalidUseFeaturesInSameFile) {
BuildDescriptorMessagesInTestPool();
ParseAndBuildFileWithErrors("foo.proto", R"schema(
edition = "2023";

package test;
import "google/protobuf/descriptor.proto";

message Foo {
string bar = 1 [
features.(test.custom).foo = "xyz",
features.(test.another) = {foo: -321}
];
}

message Custom {
string foo = 1 [features = { [test.custom]: {foo: "abc"} }];

This comment has been minimized.

Copy link
@jhump

jhump May 16, 2024

Contributor

@mkruskal-google, the error below looks to be triggered from line 10383 above. Maybe you could add another test case with an option that instead uses a message literal, like this one? It doesn't look like this change handles this. I think this would need to be a post-process after unmarshalling the uninterpreted aggregate value, to look for any extension fields that were defined in the same file.

}
message Another {
Enum foo = 1;
}

enum Enum {
option features.enum_type = CLOSED;
ZERO = 0;
ONE = 1;
}

extend google.protobuf.FeatureSet {
Custom custom = 1002 [features.message_encoding=DELIMITED];
Another another = 1001;
}
)schema",
"foo.proto: test.Foo.bar: OPTION_NAME: Feature "
"\"features.(test.custom)\" can't be used in the "
"same file it's defined in.\n");
}

TEST_F(FeaturesTest, ClosedEnumNonZeroFirstValue) {
BuildDescriptorMessagesInTestPool();
const FileDescriptor* file = BuildFile(
Expand Down

0 comments on commit 24b91a7

Please sign in to comment.