gax-java icon indicating copy to clipboard operation
gax-java copied to clipboard

Published 20 hours ago • verified publisher icon googleapis

fix: change return types of build/toBuilder methods to not cause unchecked cast warnings

Open dpcollins-google opened this issue 3 years ago • 5 comments

dpcollins-google avatar Mar 14 '22 01:03 dpcollins-google

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell D 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

sonarqubecloud[bot] avatar Mar 14 '22 01:03 sonarqubecloud[bot]

@dpcollins-google Can you please provide some more context for this change? Is there a related issue somewhere? Thanks!

meltsufin avatar Mar 14 '22 13:03 meltsufin

Sure. When building google-cloud-pubsublite, I get a lot of warnings like this with generated code:

[WARNING] /Users/dpcollins/Documents/java-pubsublite/google-cloud-pubsublite/src/main/java/com/google/cloud/pubsublite/v1/PublisherServiceSettings.java:[127,17] [unchecked] toBuilder() in PublisherServiceSettings overrides <B>toBuilder() in ClientSettings
  return type requires unchecked conversion from com.google.cloud.pubsublite.v1.PublisherServiceSettings.Builder to B
  where B,SettingsT are type-variables:
    B extends com.google.api.gax.rpc.ClientSettings.Builder<SettingsT,B> declared in method <B>toBuilder()
    SettingsT extends ClientSettings<SettingsT> declared in class ClientSettings

This is because the toBuilder abstract method claims to be generic- but is actually only instantiated for one builder type, while the base class method claims it should be instantiated in derived types for all instances of ClientSettings.Builder in every implementation. Since it's okay to override the return type of an abstract method in java as long as the new return type conforms to the abstract one, this change will not cause failing builds since all builders already must conform to Builder<SettingsT, ?>

I also see errors like this:

[WARNING] /Users/dpcollins/Documents/java-pubsublite/google-cloud-pubsublite/src/main/java/com/google/cloud/pubsublite/v1/stub/CursorServiceStubSettings.java:[421,37] [unchecked] build() in com.google.cloud.pubsublite.v1.stub.CursorServiceStubSettings.Builder overrides <B>build() in com.google.api.gax.rpc.StubSettings.Builder
  return type requires unchecked conversion from CursorServiceStubSettings to StubSettings<B>
  where B is a type-variable:
    B extends StubSettings<B> declared in method <B>build()

Which is due to the same issue with the build() method, although for this one we already have access to the settings object type as a generic type parameter.

dpcollins-google avatar Mar 14 '22 15:03 dpcollins-google

@dpcollins-google Is current gax causing errors or warnings for you? If it is just warnings, given that those classes are used literally everywhere in whole google cloud and Ads libraries (I'm worrying especially about Ads ones, because they have custom post-processing and manual layer), I would propose to refrain from pushing this changes.

vam-google avatar Mar 14 '22 18:03 vam-google

This would also fix this issue: https://github.com/googleapis/gapic-generator-java/issues/1306

igorbernstein2 avatar Mar 16 '22 15:03 igorbernstein2

The repository has moved to https://github.com/googleapis/sdk-platform-java/tree/main/gax-java. We had triaged pull requests and had moved necessary ones to the new locations. Closing this pull request.

suztomo avatar Jul 06 '23 14:07 suztomo