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
pull/16875/head
Mike Kruskal 7 months ago
parent 615e7045c5
commit 8c5f3a747b
  1. 26
      src/google/protobuf/descriptor.cc
  2. 60
      src/google/protobuf/descriptor_unittest.cc

@ -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> template <typename DescriptorT>
absl::string_view GetFullName(const DescriptorT& desc) { absl::string_view GetFullName(const DescriptorT& desc) {
return desc.full_name(); return desc.full_name();
@ -8774,6 +8761,19 @@ bool DescriptorBuilder::OptionInterpreter::InterpretSingleOption(
// accumulate field numbers to form path to interpreted option // accumulate field numbers to form path to interpreted option
dest_path.push_back(field->number()); 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 (i < uninterpreted_option_->name_size() - 1) {
if (field->cpp_type() != FieldDescriptor::CPPTYPE_MESSAGE) { if (field->cpp_type() != FieldDescriptor::CPPTYPE_MESSAGE) {
return AddNameError([&] { return AddNameError([&] {

@ -4059,8 +4059,8 @@ class ValidationErrorTest : public testing::Test {
return ABSL_DIE_IF_NULL(pool_.BuildFile(file_proto)); return ABSL_DIE_IF_NULL(pool_.BuildFile(file_proto));
} }
const FileDescriptor* ParseAndBuildFile(absl::string_view file_name, FileDescriptorProto ParseFile(absl::string_view file_name,
absl::string_view file_text) { absl::string_view file_text) {
io::ArrayInputStream input_stream(file_text.data(), file_text.size()); io::ArrayInputStream input_stream(file_text.data(), file_text.size());
SimpleErrorCollector error_collector; SimpleErrorCollector error_collector;
io::Tokenizer tokenizer(&input_stream, &error_collector); io::Tokenizer tokenizer(&input_stream, &error_collector);
@ -4072,7 +4072,12 @@ class ValidationErrorTest : public testing::Test {
<< file_text; << file_text;
ABSL_CHECK_EQ("", error_collector.last_error()); ABSL_CHECK_EQ("", error_collector.last_error());
proto.set_name(file_name); 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));
} }
@ -4096,6 +4101,17 @@ class ValidationErrorTest : public testing::Test {
BuildFileWithErrors(file_proto, expected_errors); 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 // Parse file_text as a FileDescriptorProto in text format and add it
// to the DescriptorPool. Expect errors to be produced which match the // to the DescriptorPool. Expect errors to be produced which match the
// given warning text. // given warning text.
@ -10283,6 +10299,44 @@ TEST_F(FeaturesTest, InvalidOpenEnumNonZeroFirstValue) {
"enums.\n"); "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"} }];
}
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) { TEST_F(FeaturesTest, ClosedEnumNonZeroFirstValue) {
BuildDescriptorMessagesInTestPool(); BuildDescriptorMessagesInTestPool();
const FileDescriptor* file = BuildFile( const FileDescriptor* file = BuildFile(

Loading…
Cancel
Save