Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Java Class Name for Immutable #19573

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -83,4 +83,32 @@ message JavaFeatures {
edition_defaults = { edition: EDITION_LEGACY, value: "true" },
edition_defaults = { edition: EDITION_2024, value: "false" }
];

enum NestInFileClass {
// Invalid default, which should never be used.
NEST_IN_FILE_CLASS_UNKNOWN = 0;
// Do not nest the generated class in the file class.
NO = 1;
// Nest the generated class in the file class.
YES = 2;
// Fall back to the `java_multiple_files` option. Users won't be able to
// set this option.
LEGACY = 3 [feature_support = {
edition_introduced: EDITION_2024
edition_removed: EDITION_2024
}];
}

optional NestInFileClass nest_in_file_class = 5 [
retention = RETENTION_RUNTIME,
targets = TARGET_TYPE_FILE,
targets = TARGET_TYPE_MESSAGE,
targets = TARGET_TYPE_ENUM,
targets = TARGET_TYPE_SERVICE,
feature_support = {
edition_introduced: EDITION_2024,
},
edition_defaults = { edition: EDITION_LEGACY, value: "LEGACY" },
edition_defaults = { edition: EDITION_2024, value: "NO" }
];
}
13 changes: 13 additions & 0 deletions src/google/protobuf/compiler/command_line_interface_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2310,6 +2310,19 @@ TEST_F(CommandLineInterfaceTest, EditionDefaultsInvalidMaximumUnknown) {
ExpectErrorSubstring("unknown edition \"2022\"");
}

TEST_F(CommandLineInterfaceTest, JavaMultipleFilesEdition2024Invalid) {
CreateTempFile("foo.proto",
R"schema(
edition = "2024";
option java_multiple_files = true;
message Bar {}
)schema");
Run("protocol_compiler --proto_path=$tmpdir "
"foo.proto --test_out=$tmpdir --experimental_editions");
ExpectErrorSubstring(
"`java_multiple_files` is not supported in editions 2024 and above");
}


TEST_F(CommandLineInterfaceTest, DirectDependencies_Missing_EmptyList) {
CreateTempFile("foo.proto",
Expand Down
71 changes: 38 additions & 33 deletions src/google/protobuf/compiler/java/file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -349,17 +349,21 @@ void FileGenerator::Generate(io::Printer* printer) {

// -----------------------------------------------------------------

if (!MultipleJavaFiles(file_, immutable_api_)) {
for (int i = 0; i < file_->enum_type_count(); i++) {
for (int i = 0; i < file_->enum_type_count(); i++) {
if (NestedInFileClass(*file_->enum_type(i), immutable_api_)) {
generator_factory_->NewEnumGenerator(file_->enum_type(i))
->Generate(printer);
}
for (int i = 0; i < file_->message_type_count(); i++) {
}
for (int i = 0; i < file_->message_type_count(); i++) {
if (NestedInFileClass(*file_->message_type(i), immutable_api_)) {
message_generators_[i]->GenerateInterface(printer);
message_generators_[i]->Generate(printer);
}
if (HasGenericServices(file_, context_->EnforceLite())) {
for (int i = 0; i < file_->service_count(); i++) {
}
if (HasGenericServices(file_, context_->EnforceLite())) {
for (int i = 0; i < file_->service_count(); i++) {
if (NestedInFileClass(*file_->service(i), immutable_api_)) {
std::unique_ptr<ServiceGenerator> generator(
generator_factory_->NewServiceGenerator(file_->service(i)));
generator->Generate(printer);
Expand Down Expand Up @@ -578,38 +582,39 @@ void FileGenerator::GenerateSiblings(
const std::string& package_dir, GeneratorContext* context,
std::vector<std::string>* file_list,
std::vector<std::string>* annotation_list) {
if (MultipleJavaFiles(file_, immutable_api_)) {
for (int i = 0; i < file_->enum_type_count(); i++) {
std::unique_ptr<EnumGenerator> generator(
generator_factory_->NewEnumGenerator(file_->enum_type(i)));
GenerateSibling<EnumGenerator>(
package_dir, java_package_, file_->enum_type(i), context, file_list,
options_.annotate_code, annotation_list, "", generator.get(),
options_.opensource_runtime, &EnumGenerator::Generate);
}
for (int i = 0; i < file_->message_type_count(); i++) {
if (immutable_api_) {
GenerateSibling<MessageGenerator>(
package_dir, java_package_, file_->message_type(i), context,
file_list, options_.annotate_code, annotation_list, "OrBuilder",
message_generators_[i].get(), options_.opensource_runtime,
&MessageGenerator::GenerateInterface);
}
for (int i = 0; i < file_->enum_type_count(); i++) {
if (NestedInFileClass(*file_->enum_type(i), immutable_api_)) continue;
std::unique_ptr<EnumGenerator> generator(
generator_factory_->NewEnumGenerator(file_->enum_type(i)));
GenerateSibling<EnumGenerator>(
package_dir, java_package_, file_->enum_type(i), context, file_list,
options_.annotate_code, annotation_list, "", generator.get(),
options_.opensource_runtime, &EnumGenerator::Generate);
}
for (int i = 0; i < file_->message_type_count(); i++) {
if (NestedInFileClass(*file_->message_type(i), immutable_api_)) continue;
if (immutable_api_) {
GenerateSibling<MessageGenerator>(
package_dir, java_package_, file_->message_type(i), context,
file_list, options_.annotate_code, annotation_list, "",
file_list, options_.annotate_code, annotation_list, "OrBuilder",
message_generators_[i].get(), options_.opensource_runtime,
&MessageGenerator::Generate);
&MessageGenerator::GenerateInterface);
}
if (HasGenericServices(file_, context_->EnforceLite())) {
for (int i = 0; i < file_->service_count(); i++) {
std::unique_ptr<ServiceGenerator> generator(
generator_factory_->NewServiceGenerator(file_->service(i)));
GenerateSibling<ServiceGenerator>(
package_dir, java_package_, file_->service(i), context, file_list,
options_.annotate_code, annotation_list, "", generator.get(),
options_.opensource_runtime, &ServiceGenerator::Generate);
}
GenerateSibling<MessageGenerator>(
package_dir, java_package_, file_->message_type(i), context, file_list,
options_.annotate_code, annotation_list, "",
message_generators_[i].get(), options_.opensource_runtime,
&MessageGenerator::Generate);
}
if (HasGenericServices(file_, context_->EnforceLite())) {
for (int i = 0; i < file_->service_count(); i++) {
if (NestedInFileClass(*file_->service(i), immutable_api_)) continue;
std::unique_ptr<ServiceGenerator> generator(
generator_factory_->NewServiceGenerator(file_->service(i)));
GenerateSibling<ServiceGenerator>(
package_dir, java_package_, file_->service(i), context, file_list,
options_.annotate_code, annotation_list, "", generator.get(),
options_.opensource_runtime, &ServiceGenerator::Generate);
}
}
}
Expand Down
12 changes: 6 additions & 6 deletions src/google/protobuf/compiler/java/full/message.cc
Original file line number Diff line number Diff line change
Expand Up @@ -103,12 +103,12 @@ void ImmutableMessageGenerator::GenerateStaticVariables(
if (descriptor_->containing_type() != nullptr) {
vars["parent"] = UniqueFileScopeIdentifier(descriptor_->containing_type());
}
if (MultipleJavaFiles(descriptor_->file(), /* immutable = */ true)) {
if (NestedInFileClass(*descriptor_, /* immutable = */ true)) {
// We can only make these package-private since the classes that use them
// are in separate files.
vars["private"] = "";
} else {
vars["private"] = "private ";
} else {
vars["private"] = "";
}
if (*bytecode_estimate <= kMaxStaticSize) {
vars["final"] = "final ";
Expand Down Expand Up @@ -179,12 +179,12 @@ void ImmutableMessageGenerator::GenerateFieldAccessorTable(
io::Printer* printer, int* bytecode_estimate) {
absl::flat_hash_map<absl::string_view, std::string> vars;
vars["identifier"] = UniqueFileScopeIdentifier(descriptor_);
if (MultipleJavaFiles(descriptor_->file(), /* immutable = */ true)) {
if (NestedInFileClass(*descriptor_, /* immutable = */ true)) {
// We can only make these package-private since the classes that use them
// are in separate files.
vars["private"] = "";
} else {
vars["private"] = "private ";
} else {
vars["private"] = "";
}
if (*bytecode_estimate <= kMaxStaticSize) {
vars["final"] = "final ";
Expand Down
47 changes: 47 additions & 0 deletions src/google/protobuf/compiler/java/helpers.cc
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
#include "absl/strings/str_split.h"
#include "absl/strings/string_view.h"
#include "absl/strings/substitute.h"
#include "google/protobuf/compiler/java/java_features.pb.h"
#include "google/protobuf/compiler/java/generator.h"
#include "google/protobuf/compiler/java/name_resolver.h"
#include "google/protobuf/compiler/versions.h"
#include "google/protobuf/descriptor.pb.h"
Expand Down Expand Up @@ -918,6 +920,51 @@ const FieldDescriptor* MapValueField(const FieldDescriptor* descriptor) {
}


namespace {

// Get the value of `nest_in_file_class` feature and returns whether the
// generated class should be nested in the generated proto file Java class.
template <typename Descriptor>
inline bool NestInFileClass(const Descriptor& descriptor) {
// TODO b/373884685 - Clean up this check once when we have a way to query
// Java features in the C++ runtime.
// if (JavaGenerator::GetEdition(*descriptor.file()) < EDITION_2024) {
// return !descriptor.file()->options().java_multiple_files();
// }
auto nest_in_file_class = JavaGenerator::GetResolvedSourceFeatures(descriptor)
.GetExtension(pb::java)
.nest_in_file_class();
CHECK(nest_in_file_class != pb::JavaFeatures::NEST_IN_FILE_CLASS_UNKNOWN)
<< "MMP\n"
<< descriptor.DebugString() << "\n"
<< descriptor.file()->name();
if (nest_in_file_class == pb::JavaFeatures::LEGACY) {
return !descriptor.file()->options().java_multiple_files();
}
ABSL_CHECK(nest_in_file_class != pb::JavaFeatures::LEGACY);
return nest_in_file_class == pb::JavaFeatures::YES;
}

// Returns whether multiple Java files should be generated for the given
// descriptor, depending on different Protobuf Java API versions.
template <typename Descriptor>
bool MultipleJavaFiles(const Descriptor& descriptor, bool immutable) {
(void)immutable;
return !NestInFileClass(descriptor);
}
} // namespace

bool NestedInFileClass(const Descriptor& descriptor, bool immutable) {
return !MultipleJavaFiles(descriptor, immutable);
}

bool NestedInFileClass(const EnumDescriptor& descriptor, bool immutable) {
return !MultipleJavaFiles(descriptor, immutable);
}

bool NestedInFileClass(const ServiceDescriptor& descriptor, bool immutable) {
return !MultipleJavaFiles(descriptor, immutable);
}
} // namespace java
} // namespace compiler
} // namespace protobuf
Expand Down
26 changes: 14 additions & 12 deletions src/google/protobuf/compiler/java/helpers.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,7 @@

#include <cstdint>
#include <string>
#include <utility>
#include <vector>

#include "absl/container/flat_hash_map.h"
#include "absl/strings/string_view.h"
#include "google/protobuf/compiler/java/names.h"
#include "google/protobuf/compiler/java/options.h"
Expand Down Expand Up @@ -142,25 +139,30 @@ inline Proto1EnumRepresentation GetProto1EnumRepresentation(
return Proto1EnumRepresentation::kInteger;
}

// Whether we should generate multiple java files for messages.
inline bool MultipleJavaFiles(const FileDescriptor* descriptor,
bool immutable) {
(void)immutable;
return descriptor->options().java_multiple_files();
}

// Returns true if the generated class for the type is nested in the generated
// proto file Java class.
// `immutable` should be set to true if we're generating for the immutable API.
// TODO b/372482046 - Make these functions public so that plugins can use them
// to determine whether to generate multiple files for arbitrary editions
// instead of accessing the `java_multiple_files` file option directly.
bool NestedInFileClass(const Descriptor& descriptor, bool immutable);
bool NestedInFileClass(const EnumDescriptor& descriptor, bool immutable);
bool NestedInFileClass(const ServiceDescriptor& descriptor, bool immutable);

// Returns true if `descriptor` will be written to its own .java file.
// `immutable` should be set to true if we're generating for the immutable API.
// For nested messages, this always returns false, since their generated Java
// class is always nested in their parent message's Java class i.e. they never
// have their own Java file.
template <typename Descriptor>
bool IsOwnFile(const Descriptor* descriptor, bool immutable) {
return descriptor->containing_type() == nullptr &&
MultipleJavaFiles(descriptor->file(), immutable);
!NestedInFileClass(*descriptor, immutable);
}

template <>
inline bool IsOwnFile(const ServiceDescriptor* descriptor, bool immutable) {
return MultipleJavaFiles(descriptor->file(), immutable);
return !NestedInFileClass(*descriptor, immutable);
}

// If `descriptor` describes an object with its own .java file,
Expand Down
Loading
Loading