Skip to content

Commit 63fa9ab

Browse files
shaod2shaod2
authored
Merge pull request #21978 from shaod2/31.1-cp
Backport changes that support late injection of feature set defaults
2 parents bec5b5a + 96a9ef6 commit 63fa9ab

23 files changed

+705
-91
lines changed

‎editions/generated_files_test.cc‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
#include "editions/golden/test_messages_proto2_editions.pb.h"
1313
#include "editions/golden/test_messages_proto3_editions.pb.h"
1414
#include "editions/proto/test_editions_default_features.pb.h"
15+
#include "google/protobuf/internal_feature_helper.h"
1516
#include "google/protobuf/test_textproto.h"
1617

1718
// These tests provide some basic minimal coverage that protos work as expected.

‎python/google/protobuf/pyext/descriptor.cc‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
#include "absl/strings/string_view.h"
2424
#include "google/protobuf/descriptor.h"
2525
#include "google/protobuf/dynamic_message.h"
26+
#include "google/protobuf/internal_feature_helper.h"
2627
#include "google/protobuf/io/coded_stream.h"
2728
#include "google/protobuf/pyext/descriptor_containers.h"
2829
#include "google/protobuf/pyext/descriptor_pool.h"

‎src/file_lists.cmake‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ set(libprotobuf_srcs
4646
${protobuf_SOURCE_DIR}/src/google/protobuf/generated_message_util.cc
4747
${protobuf_SOURCE_DIR}/src/google/protobuf/implicit_weak_message.cc
4848
${protobuf_SOURCE_DIR}/src/google/protobuf/inlined_string_field.cc
49+
${protobuf_SOURCE_DIR}/src/google/protobuf/internal_feature_helper.cc
4950
${protobuf_SOURCE_DIR}/src/google/protobuf/io/coded_stream.cc
5051
${protobuf_SOURCE_DIR}/src/google/protobuf/io/gzip_stream.cc
5152
${protobuf_SOURCE_DIR}/src/google/protobuf/io/io_win32.cc
@@ -137,6 +138,7 @@ set(libprotobuf_hdrs
137138
${protobuf_SOURCE_DIR}/src/google/protobuf/has_bits.h
138139
${protobuf_SOURCE_DIR}/src/google/protobuf/implicit_weak_message.h
139140
${protobuf_SOURCE_DIR}/src/google/protobuf/inlined_string_field.h
141+
${protobuf_SOURCE_DIR}/src/google/protobuf/internal_feature_helper.h
140142
${protobuf_SOURCE_DIR}/src/google/protobuf/internal_visibility.h
141143
${protobuf_SOURCE_DIR}/src/google/protobuf/io/coded_stream.h
142144
${protobuf_SOURCE_DIR}/src/google/protobuf/io/gzip_stream.h
@@ -851,6 +853,7 @@ set(protoc-gen-upb_srcs
851853
${protobuf_SOURCE_DIR}/src/google/protobuf/generated_message_util.cc
852854
${protobuf_SOURCE_DIR}/src/google/protobuf/implicit_weak_message.cc
853855
${protobuf_SOURCE_DIR}/src/google/protobuf/inlined_string_field.cc
856+
${protobuf_SOURCE_DIR}/src/google/protobuf/internal_feature_helper.cc
854857
${protobuf_SOURCE_DIR}/src/google/protobuf/io/coded_stream.cc
855858
${protobuf_SOURCE_DIR}/src/google/protobuf/io/gzip_stream.cc
856859
${protobuf_SOURCE_DIR}/src/google/protobuf/io/io_win32.cc
@@ -930,6 +933,7 @@ set(protoc-gen-upb_hdrs
930933
${protobuf_SOURCE_DIR}/src/google/protobuf/has_bits.h
931934
${protobuf_SOURCE_DIR}/src/google/protobuf/implicit_weak_message.h
932935
${protobuf_SOURCE_DIR}/src/google/protobuf/inlined_string_field.h
936+
${protobuf_SOURCE_DIR}/src/google/protobuf/internal_feature_helper.h
933937
${protobuf_SOURCE_DIR}/src/google/protobuf/internal_visibility.h
934938
${protobuf_SOURCE_DIR}/src/google/protobuf/io/coded_stream.h
935939
${protobuf_SOURCE_DIR}/src/google/protobuf/io/gzip_stream.h
@@ -1020,6 +1024,7 @@ set(protoc-gen-upbdefs_srcs
10201024
${protobuf_SOURCE_DIR}/src/google/protobuf/generated_message_util.cc
10211025
${protobuf_SOURCE_DIR}/src/google/protobuf/implicit_weak_message.cc
10221026
${protobuf_SOURCE_DIR}/src/google/protobuf/inlined_string_field.cc
1027+
${protobuf_SOURCE_DIR}/src/google/protobuf/internal_feature_helper.cc
10231028
${protobuf_SOURCE_DIR}/src/google/protobuf/io/coded_stream.cc
10241029
${protobuf_SOURCE_DIR}/src/google/protobuf/io/gzip_stream.cc
10251030
${protobuf_SOURCE_DIR}/src/google/protobuf/io/io_win32.cc
@@ -1100,6 +1105,7 @@ set(protoc-gen-upbdefs_hdrs
11001105
${protobuf_SOURCE_DIR}/src/google/protobuf/has_bits.h
11011106
${protobuf_SOURCE_DIR}/src/google/protobuf/implicit_weak_message.h
11021107
${protobuf_SOURCE_DIR}/src/google/protobuf/inlined_string_field.h
1108+
${protobuf_SOURCE_DIR}/src/google/protobuf/internal_feature_helper.h
11031109
${protobuf_SOURCE_DIR}/src/google/protobuf/internal_visibility.h
11041110
${protobuf_SOURCE_DIR}/src/google/protobuf/io/coded_stream.h
11051111
${protobuf_SOURCE_DIR}/src/google/protobuf/io/gzip_stream.h
@@ -1419,6 +1425,7 @@ set(protobuf_test_files
14191425
${protobuf_SOURCE_DIR}/src/google/protobuf/generated_message_tctable_lite_test.cc
14201426
${protobuf_SOURCE_DIR}/src/google/protobuf/has_bits_test.cc
14211427
${protobuf_SOURCE_DIR}/src/google/protobuf/inlined_string_field_unittest.cc
1428+
${protobuf_SOURCE_DIR}/src/google/protobuf/internal_feature_helper_test.cc
14221429
${protobuf_SOURCE_DIR}/src/google/protobuf/internal_message_util_unittest.cc
14231430
${protobuf_SOURCE_DIR}/src/google/protobuf/map_field_test.cc
14241431
${protobuf_SOURCE_DIR}/src/google/protobuf/map_test.cc

‎src/google/protobuf/BUILD.bazel‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -718,6 +718,7 @@ PROTOBUF_HEADERS = [
718718
"descriptor_visitor.h",
719719
"dynamic_message.h",
720720
"feature_resolver.h",
721+
"internal_feature_helper.h",
721722
"field_access_listener.h",
722723
"generated_enum_reflection.h",
723724
"generated_message_bases.h",
@@ -754,6 +755,7 @@ cc_library(
754755
"generated_message_reflection.cc",
755756
"generated_message_tctable_full.cc",
756757
"generated_message_tctable_gen.cc",
758+
"internal_feature_helper.cc",
757759
"map_field.cc",
758760
"message.cc",
759761
"reflection_mode.cc",
@@ -1704,6 +1706,37 @@ cc_test(
17041706
],
17051707
)
17061708

1709+
cc_test(
1710+
name = "internal_feature_helper_test",
1711+
srcs = [
1712+
"internal_feature_helper_test.cc",
1713+
],
1714+
copts = COPTS,
1715+
deps = [
1716+
":cc_test_protos",
1717+
":port",
1718+
":protobuf",
1719+
":protobuf_lite",
1720+
":test_textproto",
1721+
":test_util",
1722+
"//src/google/protobuf/compiler:importer",
1723+
"//src/google/protobuf/io",
1724+
"//src/google/protobuf/io:tokenizer",
1725+
"//src/google/protobuf/testing",
1726+
"//src/google/protobuf/testing:file",
1727+
"@abseil-cpp//absl/log:absl_check",
1728+
"@abseil-cpp//absl/log:absl_log",
1729+
"@abseil-cpp//absl/log:die_if_null",
1730+
"@abseil-cpp//absl/memory",
1731+
"@abseil-cpp//absl/status",
1732+
"@abseil-cpp//absl/status:statusor",
1733+
"@abseil-cpp//absl/strings",
1734+
"@abseil-cpp//absl/strings:str_format",
1735+
"@googletest//:gtest",
1736+
"@googletest//:gtest_main",
1737+
],
1738+
)
1739+
17071740
cc_test(
17081741
name = "generated_message_reflection_unittest",
17091742
srcs = ["generated_message_reflection_unittest.cc"],

‎src/google/protobuf/compiler/code_generator.h‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
#include "google/protobuf/compiler/code_generator_lite.h" // IWYU pragma: export
2626
#include "google/protobuf/descriptor.h"
2727
#include "google/protobuf/descriptor.pb.h"
28+
#include "google/protobuf/internal_feature_helper.h"
2829

2930
// Must be included last.
3031
#include "google/protobuf/port_def.inc"
@@ -142,6 +143,20 @@ class PROTOC_EXPORT CodeGenerator {
142143
return ::google::protobuf::internal::InternalFeatureHelper::GetFeatures(desc);
143144
}
144145

146+
// Returns the resolved FeatureSet for the language extension. It is
147+
// guaranteed that the result is fully aware of the language feature set
148+
// defaults, either the defaults set to the descriptor pool, or, if not set,
149+
// the defaults embedded in the language FeatureSet extension.
150+
template <typename DescriptorT, typename TypeTraitsT, uint8_t field_type,
151+
bool is_packed>
152+
static auto GetResolvedSourceFeatureExtension(
153+
const DescriptorT& desc,
154+
const google::protobuf::internal::ExtensionIdentifier<
155+
FeatureSet, TypeTraitsT, field_type, is_packed>& extension) {
156+
return ::google::protobuf::internal::InternalFeatureHelper::
157+
GetResolvedFeatureExtension(desc, extension);
158+
}
159+
145160
// Retrieves the unresolved source features for a given descriptor. These
146161
// should be used to validate the original .proto file. These represent the
147162
// original proto files from generated code, but should be stripped of

‎src/google/protobuf/compiler/code_generator_unittest.cc‎

Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@ class TestGenerator : public CodeGenerator {
6767
}
6868

6969
// Expose the protected methods for testing.
70+
using CodeGenerator::GetResolvedSourceFeatureExtension;
7071
using CodeGenerator::GetResolvedSourceFeatures;
7172
using CodeGenerator::GetUnresolvedSourceFeatures;
7273

@@ -246,6 +247,143 @@ TEST_F(CodeGeneratorTest, GetResolvedSourceFeaturesInherited) {
246247
EXPECT_EQ(ext.source_feature2(), pb::EnumFeature::VALUE3);
247248
}
248249

250+
TEST_F(CodeGeneratorTest, GetResolvedSourceFeatureExtension) {
251+
TestGenerator generator;
252+
generator.set_feature_extensions({GetExtensionReflection(pb::test)});
253+
ASSERT_OK(pool_.SetFeatureSetDefaults(*generator.BuildFeatureSetDefaults()));
254+
255+
ASSERT_THAT(BuildFile(DescriptorProto::descriptor()->file()), NotNull());
256+
ASSERT_THAT(BuildFile(pb::TestMessage::descriptor()->file()), NotNull());
257+
auto file = BuildFile(R"schema(
258+
edition = "2023";
259+
package proto2_unittest;
260+
261+
import "google/protobuf/unittest_features.proto";
262+
263+
option features.(pb.test).file_feature = VALUE6;
264+
option features.(pb.test).source_feature = VALUE5;
265+
)schema");
266+
ASSERT_THAT(file, NotNull());
267+
const pb::TestFeatures& ext1 =
268+
TestGenerator::GetResolvedSourceFeatureExtension(*file, pb::test);
269+
const pb::TestFeatures& ext2 =
270+
TestGenerator::GetResolvedSourceFeatures(*file).GetExtension(pb::test);
271+
272+
// Since the pool provides the feature set defaults, there should be no
273+
// difference between the two results.
274+
EXPECT_EQ(ext1.enum_feature(), pb::EnumFeature::VALUE1);
275+
EXPECT_EQ(ext1.field_feature(), pb::EnumFeature::VALUE1);
276+
EXPECT_EQ(ext1.file_feature(), pb::EnumFeature::VALUE6);
277+
EXPECT_EQ(ext1.source_feature(), pb::EnumFeature::VALUE5);
278+
EXPECT_EQ(ext2.enum_feature(), ext1.enum_feature());
279+
EXPECT_EQ(ext2.field_feature(), ext1.field_feature());
280+
EXPECT_EQ(ext2.file_feature(), ext1.file_feature());
281+
EXPECT_EQ(ext2.source_feature(), ext1.source_feature());
282+
}
283+
284+
TEST_F(CodeGeneratorTest, GetResolvedSourceFeatureExtensionEditedDefaults) {
285+
FeatureSetDefaults defaults = ParseTextOrDie(R"pb(
286+
minimum_edition: EDITION_PROTO2
287+
maximum_edition: EDITION_2024
288+
defaults {
289+
edition: EDITION_LEGACY
290+
overridable_features {}
291+
fixed_features {
292+
field_presence: EXPLICIT
293+
enum_type: CLOSED
294+
repeated_field_encoding: EXPANDED
295+
utf8_validation: NONE
296+
message_encoding: LENGTH_PREFIXED
297+
json_format: LEGACY_BEST_EFFORT
298+
enforce_naming_style: STYLE_LEGACY
299+
default_symbol_visibility: EXPORT_ALL
300+
}
301+
}
302+
defaults {
303+
edition: EDITION_2023
304+
overridable_features {
305+
field_presence: EXPLICIT
306+
enum_type: OPEN
307+
repeated_field_encoding: PACKED
308+
utf8_validation: VERIFY
309+
message_encoding: LENGTH_PREFIXED
310+
json_format: ALLOW
311+
[pb.test] {
312+
file_feature: VALUE3
313+
field_feature: VALUE15
314+
enum_feature: VALUE14
315+
source_feature: VALUE1
316+
}
317+
}
318+
fixed_features {
319+
enforce_naming_style: STYLE_LEGACY
320+
default_symbol_visibility: EXPORT_ALL
321+
}
322+
}
323+
)pb");
324+
ASSERT_OK(pool_.SetFeatureSetDefaults(defaults));
325+
326+
ASSERT_THAT(BuildFile(DescriptorProto::descriptor()->file()), NotNull());
327+
ASSERT_THAT(BuildFile(pb::TestMessage::descriptor()->file()), NotNull());
328+
auto file = BuildFile(R"schema(
329+
edition = "2023";
330+
package proto2_unittest;
331+
332+
import "google/protobuf/unittest_features.proto";
333+
334+
option features.(pb.test).file_feature = VALUE6;
335+
option features.(pb.test).source_feature = VALUE5;
336+
)schema");
337+
ASSERT_THAT(file, NotNull());
338+
const pb::TestFeatures& ext =
339+
TestGenerator::GetResolvedSourceFeatureExtension(*file, pb::test);
340+
341+
// Since the pool provides the modified feature set defaults, the result
342+
// should be the one reflecting the pool's defaults.
343+
EXPECT_EQ(ext.enum_feature(), pb::EnumFeature::VALUE14);
344+
EXPECT_EQ(ext.field_feature(), pb::EnumFeature::VALUE15);
345+
EXPECT_EQ(ext.file_feature(), pb::EnumFeature::VALUE6);
346+
EXPECT_EQ(ext.source_feature(), pb::EnumFeature::VALUE5);
347+
}
348+
349+
TEST_F(CodeGeneratorTest,
350+
GetResolvedSourceFeatureExtensionDefaultsFromFeatureSetExtension) {
351+
// Make sure feature set defaults are empty in the pool.
352+
TestGenerator generator;
353+
generator.set_feature_extensions({});
354+
ASSERT_OK(pool_.SetFeatureSetDefaults(*generator.BuildFeatureSetDefaults()));
355+
356+
ASSERT_THAT(BuildFile(DescriptorProto::descriptor()->file()), NotNull());
357+
ASSERT_THAT(BuildFile(pb::TestMessage::descriptor()->file()), NotNull());
358+
auto file = BuildFile(R"schema(
359+
edition = "2023";
360+
package proto2_unittest;
361+
362+
import "google/protobuf/unittest_features.proto";
363+
364+
option features.(pb.test).file_feature = VALUE6;
365+
option features.(pb.test).source_feature = VALUE5;
366+
)schema");
367+
ASSERT_THAT(file, NotNull());
368+
369+
const pb::TestFeatures& ext1 =
370+
TestGenerator::GetResolvedSourceFeatureExtension(*file, pb::test);
371+
const pb::TestFeatures& ext2 =
372+
TestGenerator::GetResolvedSourceFeatures(*file).GetExtension(pb::test);
373+
374+
// No defaults were added to the pool, but they should be still present in the
375+
// result. On the other hand, features that are explicitly set should be also
376+
// present.
377+
EXPECT_EQ(ext1.enum_feature(), pb::EnumFeature::VALUE1);
378+
EXPECT_EQ(ext1.field_feature(), pb::EnumFeature::VALUE1);
379+
EXPECT_EQ(ext1.file_feature(), pb::EnumFeature::VALUE6);
380+
EXPECT_EQ(ext1.source_feature(), pb::EnumFeature::VALUE5);
381+
EXPECT_EQ(ext2.enum_feature(), pb::EnumFeature::TEST_ENUM_FEATURE_UNKNOWN);
382+
EXPECT_EQ(ext2.field_feature(), pb::EnumFeature::TEST_ENUM_FEATURE_UNKNOWN);
383+
EXPECT_EQ(ext2.file_feature(), pb::EnumFeature::VALUE6);
384+
EXPECT_EQ(ext2.source_feature(), pb::EnumFeature::VALUE5);
385+
}
386+
249387
// TODO: Use the gtest versions once that's available in OSS.
250388
MATCHER_P(HasError, msg_matcher, "") {
251389
return arg.status().code() == absl::StatusCode::kFailedPrecondition &&

‎src/google/protobuf/compiler/cpp/BUILD.bazel‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,7 @@ cc_library(
147147
"@abseil-cpp//absl/log:die_if_null",
148148
"@abseil-cpp//absl/memory",
149149
"@abseil-cpp//absl/status",
150+
"@abseil-cpp//absl/status:statusor",
150151
"@abseil-cpp//absl/strings",
151152
"@abseil-cpp//absl/strings:str_format",
152153
"@abseil-cpp//absl/synchronization",

0 commit comments

Comments
 (0)