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

Avoid ambiguous imports with @UseImportPolicy(STATIC_IMPORT_ALWAYS) #3584

Conversation

Stephan202
Copy link
Contributor

These changes prevent Refaster rules from introducing a static import that
conflicts with an existing static import. Without these changes the new test
case fails with the following error:

com/google/errorprone/refaster/testdata/StaticImportClashTemplate.java:27: error: reference to reverseOrder is ambiguous
    return reverseOrder();
           ^
  both method <T>reverseOrder() in java.util.Comparator and method <T>reverseOrder() in java.util.Collections match

Stephan202 added a commit to PicnicSupermarket/error-prone-support that referenced this pull request Dec 7, 2022
This is a workaround for the issue resolved by google/error-prone#3584.

After application of this Refaster rule, any static imports of
`java.util.Collections.reverseOrder` are obsolete. These can be removed by
running Google Java Format or Error Prone's `RemoveUnusedImports` check.

Where possible, subsequent application of the `StaticImport` checker will
statically import `java.util.Comparator.reverseOrder`.
rickie pushed a commit to PicnicSupermarket/error-prone-support that referenced this pull request Dec 7, 2022
This is a workaround for the issue resolved by google/error-prone#3584.

After application of this Refaster rule, any static imports of
`java.util.Collections.reverseOrder` are obsolete. These can be removed by
running Google Java Format or Error Prone's `RemoveUnusedImports` check.

Where possible, subsequent application of the `StaticImport` checker will
statically import `java.util.Comparator.reverseOrder`.
rickie pushed a commit to PicnicSupermarket/error-prone-support that referenced this pull request Dec 8, 2022
…#397)

This is a workaround for the issue resolved by google/error-prone#3584.

After application of this Refaster rule, any static imports of
`java.util.Collections.reverseOrder` are obsolete. These can be removed by
running Google Java Format or Error Prone's `RemoveUnusedImports` check.

Where possible, subsequent application of the `StaticImport` check will
statically import `java.util.Comparator.reverseOrder`.
@Stephan202 Stephan202 force-pushed the bugfix/refaster-static-import-clash branch from d548298 to 6086de5 Compare December 31, 2022 11:30
These changes prevent Refaster rules from introducing a static import that
conflicts with an existing static import. Without these changes the new test
case fails with the following error:

```
com/google/errorprone/refaster/testdata/StaticImportClashTemplate.java:27: error: reference to reverseOrder is ambiguous
    return reverseOrder();
           ^
  both method <T>reverseOrder() in java.util.Comparator and method <T>reverseOrder() in java.util.Collections match
```
@Stephan202 Stephan202 force-pushed the bugfix/refaster-static-import-clash branch from 6086de5 to 8185405 Compare March 25, 2023 08:02
@Stephan202
Copy link
Contributor Author

Rebased and resolved conflicts.

@rickie
Copy link
Contributor

rickie commented May 10, 2023

We still encounter this problem every now and then. Could you take a look at this @cushon?

@cushon
Copy link
Collaborator

cushon commented May 15, 2023

Sorry for the delay here.

I tried importing this, and ran into some issues with templates we have for cleanups related to https://github.com/google/truth. Truth deliberately declares overloads of assertThat in different classes, with the expectation that they will all get static-imported and the most specific one will be selected, e.g.:

import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth8.assertThat;

In general I'm not sure there's a good way for refaster to know if a particular static import is going to be ambiguous or not, and I'd like to keep the assertThat templates.

Do you have any ideas for dealing with that case?

@Stephan202
Copy link
Contributor Author

Wow... I didn't even know this was supported. 🤯

It might be possible to resolve the conflicting symbol and check whether it'd be ambiguous. But that does sound like the kind of rabbit hole for which one should set aside a few hours. I'll make a note and try to get back to this.

Copy link
Collaborator

@cushon cushon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is blocked on #3584 (comment)

I'm open to ideas about how to solve this. If it isn't possible to make the existing policy smart enough to handle that case, we could always consider adding yet another @UseImportPolicy or something

@Stephan202
Copy link
Contributor Author

Tnx for the ping on this one. Can't say exactly when I'll have time for a closer look, but I'll try to bump it on the TODO list 👍

Stephan202 added a commit to PicnicSupermarket/error-prone-support that referenced this pull request Dec 24, 2023
…e}` Refaster rules

This is a workaround for google/error-prone#3584. While there, drop an
unused method from `JUnitToAssertJRules`.
rickie pushed a commit to PicnicSupermarket/error-prone-support that referenced this pull request Dec 28, 2023
…e}` Refaster rules

This is a workaround for google/error-prone#3584. While there, drop an
unused method from `JUnitToAssertJRules`.
rickie pushed a commit to PicnicSupermarket/error-prone-support that referenced this pull request Jan 2, 2024
…e}` Refaster rules

This is a workaround for google/error-prone#3584. While there, drop an
unused method from `JUnitToAssertJRules`.
Stephan202 added a commit to PicnicSupermarket/error-prone-support that referenced this pull request Jan 2, 2024
…e}` Refaster rules (#939)

This is a workaround for google/error-prone#3584. While there, drop an
unused method from `JUnitToAssertJRules`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants