Skip to content

Commit

Permalink
Editions: Refactor feature resolution to use an intermediate message.
Browse files Browse the repository at this point in the history 
This places all of the tricky reflection logic of feature resolution into a single location that outputs a portable protobuf message.  With this message, feature resolution becomes drastically simpler and easy to reimplement in other languages.

Follow-up changes will provide:
- An API to specify generator features from the CodeGenerator class for C++ plugins
- A CLI for building these default mappings
- Bazel/CMake rules to help embed these mappings in target languages (i.e. for runtimes and non-C++ plugins)

PiperOrigin-RevId: 559615720
  • Loading branch information
@mkruskal-google @copybara-github
mkruskal-google authored and copybara-github committed Aug 24, 2023
1 parent 7d5592e commit 57d8049
Show file tree
Hide file tree
Showing 9 changed files with 422 additions and 399 deletions.
3 changes: 3 additions & 0 deletions src/google/protobuf/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1109,6 +1109,9 @@ cc_test(
"//src/google/protobuf/compiler:importer",
"//src/google/protobuf/stubs",
"//src/google/protobuf/testing",
"@com_google_absl//absl/log:absl_check",
"@com_google_absl//absl/log:absl_log",
"@com_google_absl//absl/log:die_if_null",
"@com_google_googletest//:gtest",
"@com_google_googletest//:gtest_main",
],
Expand Down
3 changes: 2 additions & 1 deletion src/google/protobuf/compiler/code_generator_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,8 @@ TEST_F(CodeGeneratorTest, GetResolvedSourceFeaturesRoot) {
EXPECT_EQ(features.field_presence(), FeatureSet::EXPLICIT);
EXPECT_EQ(features.enum_type(), FeatureSet::CLOSED);

EXPECT_TRUE(ext.has_int_message_feature());
// TODO(b/296638633) Flip this once generators can specify their feature sets.
EXPECT_FALSE(ext.has_int_message_feature());
EXPECT_EQ(ext.int_file_feature(), 8);
EXPECT_EQ(ext.string_source_feature(), "file");
}
Expand Down
14 changes: 8 additions & 6 deletions src/google/protobuf/compiler/command_line_interface_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1353,7 +1353,7 @@ TEST_F(CommandLineInterfaceTest, AllowServicesHasService) {

TEST_F(CommandLineInterfaceTest, EditionsAreNotAllowed) {
CreateTempFile("foo.proto",
"edition = \"very-cool\";\n"
"edition = \"2023\";\n"
"message FooRequest {}\n");

Run("protocol_compiler --proto_path=$tmpdir --test_out=$tmpdir foo.proto");
Expand All @@ -1365,7 +1365,7 @@ TEST_F(CommandLineInterfaceTest, EditionsAreNotAllowed) {

TEST_F(CommandLineInterfaceTest, EditionsFlagExplicitTrue) {
CreateTempFile("foo.proto",
"edition = \"very-cool\";\n"
"edition = \"2023\";\n"
"message FooRequest {}\n");

Run("protocol_compiler --proto_path=$tmpdir --test_out=$tmpdir "
Expand Down Expand Up @@ -1477,14 +1477,16 @@ TEST_F(CommandLineInterfaceTest, FeatureExtensionError) {
import "features.proto";
message Foo {
int32 bar = 1;
int32 baz = 2 [features.(pb.test).int_feature = 5];
int32 baz = 2 [features.(pb.test).repeated_feature = 5];
})schema");

Run("protocol_compiler --proto_path=$tmpdir --test_out=$tmpdir "
"--experimental_editions foo.proto");
ExpectErrorSubstring(
"Feature field pb.TestFeatures.repeated_feature is an unsupported "
"repeated field");
// TODO(b/296638633) Flip this once generators can specify their feature sets.
ExpectNoErrors();
// ExpectErrorSubstring(
// "Feature field pb.TestFeatures.repeated_feature is an unsupported "
// "repeated field");
}

TEST_F(CommandLineInterfaceTest, Plugin_LegacyFeatures) {
Expand Down
50 changes: 31 additions & 19 deletions src/google/protobuf/descriptor.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
#include <sstream>
#include <string>
#include <type_traits>
#include <utility>
#include <vector>

#include "google/protobuf/stubs/common.h"
Expand All @@ -57,6 +58,7 @@
#include "absl/hash/hash.h"
#include "absl/log/absl_check.h"
#include "absl/log/absl_log.h"
#include "absl/memory/memory.h"
#include "absl/status/statusor.h"
#include "absl/strings/ascii.h"
#include "absl/strings/escaping.h"
Expand Down Expand Up @@ -1111,6 +1113,18 @@ bool AllowedExtendeeInProto3(const std::string& name) {
allowed_proto3_extendees->end();
}

const FeatureSetDefaults& GetCppFeatureSetDefaults() {
static const FeatureSetDefaults* default_spec = [] {
auto default_spec = FeatureResolver::CompileDefaults(
FeatureSet::descriptor(),
// TODO(b/297261063) Move this range to a central location.
{pb::CppFeatures::descriptor()->file()->extension(0)}, "2023", "2023");
ABSL_CHECK(default_spec.ok()) << default_spec.status();
return new FeatureSetDefaults(std::move(default_spec).value());
}();
return *default_spec;
}

// Create global defaults for proto2/proto3 compatibility.
FeatureSet* CreateProto2DefaultFeatures() {
FeatureSet* features = new FeatureSet();
Expand Down Expand Up @@ -4577,14 +4591,7 @@ class DescriptorBuilder {

const FileDescriptor* DescriptorPool::BuildFile(
const FileDescriptorProto& proto) {
ABSL_CHECK(fallback_database_ == nullptr)
<< "Cannot call BuildFile on a DescriptorPool that uses a "
"DescriptorDatabase. You must instead find a way to get your file "
"into the underlying database.";
ABSL_CHECK(mutex_ == nullptr); // Implied by the above ABSL_CHECK.
tables_->known_bad_symbols_.clear();
tables_->known_bad_files_.clear();
return DescriptorBuilder::New(this, tables_.get(), nullptr)->BuildFile(proto);
return BuildFileCollectingErrors(proto, nullptr);
}

const FileDescriptor* DescriptorPool::BuildFileCollectingErrors(
Expand All @@ -4596,13 +4603,15 @@ const FileDescriptor* DescriptorPool::BuildFileCollectingErrors(
ABSL_CHECK(mutex_ == nullptr); // Implied by the above ABSL_CHECK.
tables_->known_bad_symbols_.clear();
tables_->known_bad_files_.clear();
build_started_ = true;
return DescriptorBuilder::New(this, tables_.get(), error_collector)
->BuildFile(proto);
}

const FileDescriptor* DescriptorPool::BuildFileFromDatabase(
const FileDescriptorProto& proto) const {
mutex_->AssertHeld();
build_started_ = true;
if (tables_->known_bad_files_.contains(proto.name())) {
return nullptr;
}
Expand All @@ -4615,6 +4624,14 @@ const FileDescriptor* DescriptorPool::BuildFileFromDatabase(
return result;
}

void DescriptorPool::SetFeatureSetDefaults(FeatureSetDefaults spec) {
ABSL_CHECK(!build_started_)
<< "Feature set defaults can't be changed once the pool has started "
"building.";
feature_set_defaults_spec_ =
absl::make_unique<FeatureSetDefaults>(std::move(spec));
}

DescriptorBuilder::DescriptorBuilder(
const DescriptorPool* pool, DescriptorPool::Tables* tables,
DescriptorPool::ErrorCollector* error_collector)
Expand Down Expand Up @@ -5699,8 +5716,13 @@ FileDescriptor* DescriptorBuilder::BuildFileImpl(
}
ABSL_CHECK(descriptor);

const FeatureSetDefaults& defaults =
pool_->feature_set_defaults_spec_ == nullptr
? GetCppFeatureSetDefaults()
: *pool_->feature_set_defaults_spec_;

absl::StatusOr<FeatureResolver> feature_resolver =
FeatureResolver::Create(proto.edition(), descriptor);
FeatureResolver::Create(proto.edition(), defaults);
if (!feature_resolver.ok()) {
AddError(
proto.name(), proto, DescriptorPool::ErrorCollector::EDITIONS,
Expand Down Expand Up @@ -5816,16 +5838,6 @@ FileDescriptor* DescriptorBuilder::BuildFileImpl(
return nullptr;
}

// Look for feature extensions in regular imports.
if (feature_resolver_.has_value() && dependency != nullptr) {
absl::Status status = feature_resolver_->RegisterExtensions(*dependency);
if (!status.ok()) {
AddError(dependency->name(), proto,
DescriptorPool::ErrorCollector::EDITIONS,
[&] { return std::string(status.message()); });
}
}

if (dependency == nullptr) {
if (!pool_->lazily_build_dependencies_) {
if (pool_->allow_unknown_ ||
Expand Down
13 changes: 13 additions & 0 deletions src/google/protobuf/descriptor.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ class MethodOptions;
class FileOptions;
class UninterpretedOption;
class FeatureSet;
class FeatureSetDefaults;
class SourceCodeInfo;

// Defined in message_lite.h
Expand Down Expand Up @@ -2289,6 +2290,13 @@ class PROTOBUF_EXPORT DescriptorPool {
// DescriptorPool will report a import not found error.
void EnforceWeakDependencies(bool enforce) { enforce_weak_ = enforce; }

// Sets the default feature mappings used during the build. If this function
// isn't called, the C++ feature set defaults are used. If this function is
// called, these defaults will be used instead.
// FeatureSetDefaults includes a minimum/maximum supported edition, which will
// be enforced while building proto files.
void SetFeatureSetDefaults(FeatureSetDefaults spec);

// Toggles enforcement of extension declarations.
// This enforcement is disabled by default because it requires full
// descriptors with source-retention options, which are generally not
Expand Down Expand Up @@ -2469,11 +2477,16 @@ class PROTOBUF_EXPORT DescriptorPool {
bool enforce_extension_declarations_;
bool disallow_enforce_utf8_;
bool deprecated_legacy_json_field_conflicts_;
mutable bool build_started_ = false;

// Set of files to track for unused imports. The bool value when true means
// unused imports are treated as errors (and as warnings when false).
absl::flat_hash_map<std::string, bool> unused_import_track_files_;

// Specification of defaults to use for feature resolution. This defaults to
// just the global and C++ features, but can be overridden for other runtimes.
std::unique_ptr<FeatureSetDefaults> feature_set_defaults_spec_;

// Returns true if the field extends an option message of descriptor.proto.
bool IsExtendingDescriptor(const FieldDescriptor& field) const;

Expand Down
106 changes: 53 additions & 53 deletions src/google/protobuf/descriptor_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
#include <memory>
#include <string>
#include <tuple>
#include <utility>
#include <vector>

#include "google/protobuf/stubs/common.h"
Expand All @@ -67,6 +68,7 @@
#include "google/protobuf/descriptor_database.h"
#include "google/protobuf/descriptor_legacy.h"
#include "google/protobuf/dynamic_message.h"
#include "google/protobuf/feature_resolver.h"
#include "google/protobuf/io/tokenizer.h"
#include "google/protobuf/io/zero_copy_stream_impl_lite.h"
#include "google/protobuf/test_textproto.h"
Expand Down Expand Up @@ -514,17 +516,17 @@ TEST_F(FileDescriptorTest, Syntax) {
}
{
proto.set_syntax("editions");
proto.set_edition("very-cool");
proto.set_edition("2023");
DescriptorPool pool;
const FileDescriptor* file = pool.BuildFile(proto);
ASSERT_TRUE(file != nullptr);
EXPECT_EQ(FileDescriptorLegacy::Syntax::SYNTAX_EDITIONS,
FileDescriptorLegacy(file).syntax());
EXPECT_EQ("very-cool", file->edition());
EXPECT_EQ("2023", file->edition());
FileDescriptorProto other;
file->CopyTo(&other);
EXPECT_EQ("editions", other.syntax());
EXPECT_EQ("very-cool", other.edition());
EXPECT_EQ("2023", other.edition());
}
}

Expand Down Expand Up @@ -552,7 +554,7 @@ TEST_F(FileDescriptorTest, CopyHeadingTo) {
EXPECT_EQ(&other.options().features(), &FeatureSet::default_instance());
{
proto.set_syntax("editions");
proto.set_edition("very-cool");
proto.set_edition("2023");

DescriptorPool pool;
const FileDescriptor* file = pool.BuildFile(proto);
Expand All @@ -563,7 +565,7 @@ TEST_F(FileDescriptorTest, CopyHeadingTo) {
EXPECT_EQ(other.name(), "foo.proto");
EXPECT_EQ(other.package(), "foo.bar.baz");
EXPECT_EQ(other.syntax(), "editions");
EXPECT_EQ(other.edition(), "very-cool");
EXPECT_EQ(other.edition(), "2023");
EXPECT_EQ(other.options().java_package(), "foo.bar.baz");
EXPECT_TRUE(other.message_type().empty());
EXPECT_EQ(&other.options().features(), &FeatureSet::default_instance());
Expand Down Expand Up @@ -7235,7 +7237,24 @@ TEST_F(ValidationErrorTest, UnusedImportWithOtherError) {
"foo.proto: Foo.foo: EXTENDEE: \"Baz\" is not defined.\n");
}

using FeaturesTest = ValidationErrorTest;
using FeaturesBaseTest = ValidationErrorTest;

class FeaturesTest : public FeaturesBaseTest {
protected:
void SetUp() override {
ValidationErrorTest::SetUp();

auto default_spec = FeatureResolver::CompileDefaults(
FeatureSet::descriptor(),
{pb::CppFeatures::descriptor()->file()->extension(0),
pb::TestFeatures::descriptor()->file()->extension(0),
pb::TestMessage::descriptor()->extension(0),
pb::TestMessage::Nested::descriptor()->extension(0)},
"2023", "2025");
ASSERT_OK(default_spec);
pool_.SetFeatureSetDefaults(std::move(default_spec).value());
}
};

template <typename T>
const FeatureSet& GetFeatures(const T* descriptor) {
Expand Down Expand Up @@ -7544,12 +7563,36 @@ TEST_F(FeaturesTest, Edition2023Defaults) {
[pb.cpp] { legacy_closed_enum: false utf8_validation: VERIFY_PARSE }
)pb"));

// Since pb::test is linked in, it should end up with defaults in our
// FeatureSet.
// Since pb::test is registered in the pool, it should end up with defaults in
// our FeatureSet.
EXPECT_TRUE(GetFeatures(file).HasExtension(pb::test));
EXPECT_EQ(GetFeatures(file).GetExtension(pb::test).int_file_feature(), 1);
}

TEST_F(FeaturesBaseTest, DefaultEdition2023Defaults) {
BuildDescriptorMessagesInTestPool();
BuildFileInTestPool(pb::TestFeatures::descriptor()->file());
const FileDescriptor* file = BuildFile(R"pb(
name: "foo.proto"
syntax: "editions"
edition: "2023"
dependency: "google/protobuf/unittest_features.proto"
)pb");
ASSERT_NE(file, nullptr);

EXPECT_THAT(file->options(), EqualsProto(""));
EXPECT_THAT(
GetFeatures(file), EqualsProto(R"pb(
field_presence: EXPLICIT
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"));
EXPECT_FALSE(GetFeatures(file).HasExtension(pb::test));
}

TEST_F(FeaturesTest, ClearsOptions) {
BuildDescriptorMessagesInTestPool();
const FileDescriptor* file = BuildFile(R"pb(
Expand Down Expand Up @@ -7842,9 +7885,8 @@ TEST_F(FeaturesTest, InvalidEdition) {
R"pb(
name: "foo.proto" syntax: "editions" edition: "2022"
)pb",
"foo.proto: foo.proto: EDITIONS: No valid default found for edition 2022 "
"in "
"feature field google.protobuf.FeatureSet.field_presence\n");
"foo.proto: foo.proto: EDITIONS: Edition 2022 is earlier than the "
"minimum supported edition 2023\n");
}

TEST_F(FeaturesTest, FileFeatures) {
Expand Down Expand Up @@ -9110,48 +9152,6 @@ TEST_F(FeaturesTest, FeaturesOutsideEditions) {
"editions.\n");
}

TEST_F(FeaturesTest, InvalidExtensionNonMessage) {
BuildDescriptorMessagesInTestPool();
ASSERT_NE(BuildFile(R"pb(
name: "unittest_invalid_features.proto"
syntax: "proto2"
package: "pb"
dependency: "google/protobuf/descriptor.proto"
message_type {
name: "TestInvalid"
extension {
name: "scalar_extension"
number: 9996
label: LABEL_OPTIONAL
type: TYPE_STRING
extendee: ".google.protobuf.FeatureSet"
}
}
)pb"),
nullptr);
BuildFileWithErrors(
R"pb(
name: "foo.proto"
syntax: "editions"
edition: "2023"
dependency: "unittest_invalid_features.proto"
options {
uninterpreted_option {
name { name_part: "features" is_extension: false }
name {
name_part: "pb.TestInvalid.scalar_extension"
is_extension: true
}
identifier_value: "hello"
}
}
)pb",
"foo.proto: unittest_invalid_features.proto: EDITIONS: FeatureSet "
"extension pb.TestInvalid.scalar_extension is not of message type. "
"Feature extensions should always use messages to allow for "
"evolution.\n");
}

TEST_F(FeaturesTest, InvalidFieldImplicitDefault) {
BuildDescriptorMessagesInTestPool();
BuildFileWithErrors(
Expand Down
Loading

0 comments on commit 57d8049

Please sign in to comment.