Resolve more java field accessor name conflicts

[philsttr]

Previously, some proto field names would cause the java code generator to generate accessor names that conflict with method names from the message super classes/interfaces, leading to java code that would not compile.
A list of field names that cause such conflicts previously existed, but the list did not contain every field name that would cause a conflict.
Additionally, only snake_case field names would be detected. If the field name was in camelCase or began with a leading underscore, the conflict would not be detected.

This change adds the complete set of field names that will cause accessor name conflicts, and detects conflicts in snake_case, _snake_case (with a leading underscore), and camelCase field names.

Fixes #8142

[philsttr]

Can someone review this PR?

Side note, I can't add the required labels (release notes: yes, java) since I do not have permission.

[JonathanLeech]

Does this fix a field named _class?

[philsttr]

@JonathanLeech It didn't before, but I just pushed an update to this PR so that it now handles fields named _class (and also for other conflicting names beginning with a leading underscore)

[shaod2]

Thanks for the fix!

Since we're changing the field names somehow, can you also add a log warning somewhere (e.g. conflicting field names/will be changed)? I know there isn't one before, but I think it'd be good to add one here.

[philsttr]

That's a good idea.

I might not be able to get to it for a little while. Would you consider merging this bug-fix PR as-is? And create an enhancement feature request for a warning message? I can submit a new PR for that when I get a chance.

[elharo]

Why both __ and _? Is there a conflict if both cases uses a single underscore?

[philsttr]

The single and double underscores referenced here are simply to match the java code that is currently generated by the protoc java compiler. Notice the previous code used the single and double underscore. In other words, my change does not change the existing semantics for single and double underscores.

The protoc java compiler currently (prior to my change) uses a single or double underscore depending on the proto field name.

The protoc java compiler always appends at least one underscore to java field names. This prevents those field names from clashing with java keywords. For example, if the proto field name was int, then the protoc java compiler would generate a java field name of int_.

The protoc java compiler does not include the last trailing underscore in accessor method names for these fields. For example, the getter method for the proto field named int would be getInt().

In addition, the protoc java compiler adds another underscore (for a total of two underscores) to the java field name for names that would result in accessor method names that clash with other method names. For example, if the proto field name was class, the java field name would be class__ (double underscore). And again, the last trailing underscore is not included in the accessor method names. So the getter method for the proto field named class would be getClass_()

This behavior existed prior to my change. My change simply adds more detection of field names that result in accessor method name clashes. Previously, only cached_size, serialized_size, and class were detected. Unfortunately, those are only a subset of the proto field names that would actually cause java assessor method name conflicts. My change adds the complete set of proto field names that cause java accessor method name conflicts.

Again, I'm not changing the semantics of single or double underscores. I am also not going to refactor the protoc compiler to change its current semantics for single and double underscores. I'm simply fixing a bug in the protoc compiler where it was generating non-compilable java code due to missing detection of field names that cause conflicts.

[elharo]

Should field names here be field method accessor names as well?

[philsttr]

No, the protoc compiler appends a single underscore specifically to the java field names to prevent clashes with java keywords. The protoc compiler does not include this last undescore in accessor method names.

[elharo]

Don't split the ternary ?: operator across these lines. I found this really hard to read. Possibly this method should be split up, one for handling method names and one for handling field names. As is, I'm finding it very hard to grok. I did not initially understand what it was doing.

[philsttr]

I removed the ternary operator and improved the comments. As stated in my other comment, I'm not going to refactor the semantics of how the protoc compiler uses single and double underscores. I'm also not going to refactor how accessor method names are currently generated from field names. Both would be great improvements, but they are outside of the scope of this bugfix.

For comparison, take a look at the previous implementation of this method...

protobuf/java/core/src/main/java/com/google/protobuf/DescriptorMessageInfoFactory.java

Lines 592 to 598 in 3a4d931

	static String getFieldName(FieldDescriptor fd) {
	String name = (fd.getType() == FieldDescriptor.Type.GROUP)
	? fd.getMessageType().getName()
	: fd.getName();
	String suffix = specialFieldNames.contains(name) ? "__" : "_";
	return snakeCaseToCamelCase(name) + suffix;
	}

I believe the comments in the new implementation have made it more clear, and are sufficient for this bugfix.

[zhangskz]

Thanks for the contribution! This change seems alright, but does differ from similar existing behavior (

protobuf/src/google/protobuf/compiler/java/java_context.cc

Line 163 in d37dcf9

info.capitalized_name += StrCat(field->number());

) which instead appends field numbers to handle clashes rather than underscores.

I'd like to hold off a bit before approving to take a closer look at how best to handle this as there are some pros/cons to each solution, but ultimately we'd probably want a consistent approach for this type of scenario.

[philsttr]

I would like to reiterate that the underscores are existing behavior as well. For example, the existing code (prior to my change) will generate a java field named class_ and a getter method named getClass_() for a proto field named class. And similarly for proto fields named cached_size and serialized_size.

This change just adds more detection of field names that cause conflicts, and utilizes the existing behavior to resolve those conflicts.

I understand the desire for collapsing the two previously existing behaviors though. But note, you will not be able to change the existing behavior for fields named class, cached_size, and serialized_size without breaking compatibility.

[philsttr]

Hi @zhangskz What were the results of your closer look?

[zhangskz]

I haven't had the opportunity to get through a complete audit of how we handle escaping across conflict types and languages. This would actually be a large undertaking that would need to be approached carefully since modifying this would indeed break compatibility. In any case, this shouldn't affect your fix since it doesn't really make the incompatibilities much worse -- we already use both underscores to escape in some cases.

I'll review your CL shortly (aka tomorrow). Thanks for the patience!

[zhangskz]

I think snakeCaseToUpperCamelCase is only ever used for checking against specialFieldNames. Can we just keep specialFieldNames in lower camel case instead? This would simplify our code and IMO is a bit more intuitive anyways.

[philsttr]

The specialFieldNames are in UpperCamelCase to handle proto fields named _my_field, since _my_field will be MyField when it is converted to camel case (either upper or lower). It seemed more straightforward to keep the special field names in UpperCamelCase for comparison (since it handles proto fields named both my_field, _my_field, and __my_field), rather than adding additional logic to handle proto fields starting with _.

Note the tests that use ForbiddenWordsLeadingUnderscoreMessage test this case (pun intended ;)).