Exempt Immutable from UnnecessarilyFullyQualified

[hisener]

UnnecessarilyFullyQualified flags the usage of fully qualified @Value.Immutable annotation from Immutables. We prefer the fully qualified name when @Value from spring-beans is used. So we need to suppress it in those cases.

import org.springframework.beans.factory.annotation.Value;

final class Demo { 
  Demo(@Value("some.property") String property) {}

  @SuppressWarnings("UnnecessarilyFullyQualified" /* Direct `@Immutable` import is confusing. */)
  @org.immutables.value.Value.Immutable
  abstract class AbstractType {}
}

The following report is generated without the suppression:

java.lang.AssertionError: Saw unexpected error on line 8. All errors:
/test/Test.java:8: warning: [UnnecessarilyFullyQualified] This fully qualified name is unambiguous to the compiler if imported.
  @org.immutables.value.Value.Immutable
                             ^
    (see https://errorprone.info/bugpattern/UnnecessarilyFullyQualified)
  Did you mean '@Immutable'?

However, using @Immutable can be confusing as there are many @Immutable annotations, e.g. javax.annotation.concurrent.Immutable, com.google.errorprone.annotations.Immutable, org.hibernate.annotations.Immutable, etc. For that reason, I think it makes sense to add Immutable to exempted names.

[cushon]

Thanks for the PR, and sorry for the very slow response.

In our codebase we're gradually moving towards standardizing oncom.google.errorprone.annotations.Immutable, and using imports and simple names vs. qualified names, so I'd like to have a way to preserve the UnnecessarilyFullyQualified findings for that use-case.

I also see the benefit of skipping them for projects that are mixing different Immutable annotations.

I see a couple ways forward here:

• For the immutables annotation, I wondered about writing @Value.Immutable instead, @org.immutables.value.Value.Immutable. But it sounds like the preference in your code is to use the full qualified name, so maybe that wouldn't help?
• Another option would be to add a flag to UnnecessarilyFullyQualified to make the list of exemptions configurable, and then you could pass e.g. -XepOpt:UnnecessarilyFullyQualified:ExemptedNames=Immutable

What do you think?

[hisener]

No worries.

Disclaimer: I created this issue over two years ago and am trying to remember the context. Also, I work for a different company, where we don't have this problem.

• For the immutables annotation, I wondered about writing @Value.Immutable instead, @org.immutables.value.Value.Immutable. But it sounds like the preference in your code is to use the full qualified name, so maybe that wouldn't help?

I am not sure I understand the proposal here. Using @Value.Immutable is not possible when we already have the org.springframework.beans.factory.annotation.Value import, right? 👀 Or, do you mean adding org.immutables.value.Value.Immutable to the exempted name set?

[Stephan202]

Disclaimer: I created this issue over two years ago and am trying to remember the context. Also, I work for a different company, where we don't have this problem.

I can report that the original company still has this issue. 😉

I guess an ExemptedNames flag would work. Another approach (which is more work but also more versatile and closer to the Picnic use case), would be to introduce an AdditionalTypes (name TBD) flag for the BadImport check, and have UnnecessarilyFullyQualified also respect that configuration. (Note that UnnecessarilyFullyQualified already has a dependency on BadImport, so this isn't too far-fetched.)

IIUC this way one could set -XepOpt:BadImport:AdditionalTypes=org.immutables.value.Value, which would then allow UnnecessarilyFullyQualified to suggest Value.Immutable if possible and remain silent otherwise.

[cushon]

I am not sure I understand the proposal here. Using @Value.Immutable is not possible when we already have the org.springframework.beans.factory.annotation.Value import, right?

You're right--I was thinking about ways to avoid a clash with another Immutable annotation, not with Value.

Another approach (which is more work but also more versatile and closer to the Picnic use case), would be to introduce an AdditionalTypes (name TBD) flag for the BadImport check, and have UnnecessarilyFullyQualified also respect that configuration

That alternative SGTM, as long as UnnecessarilyFullyQualified and BadImport sharing heuristics it seems nice to keep them in sync with a shared flag.

[hisener]

Sorry for the delay; I created #4228 for the alternative approach Stephan suggested.

(Created a separate PR because I don't think I have write access to this branch anymore)