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: 634512378
pull/16864/head
Mike Kruskal 6 months ago committed by Copybara-Service
parent baeab50df0
commit 24b91a7fec
  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>
absl::string_view GetFullName(const DescriptorT& desc) {
return desc.full_name();
@ -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([&] {

@ -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);
@ -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));
}
@ -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.
@ -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"} }];
}
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(

Loading…
Cancel
Save