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

UnnecessarilyFullyQualified and package-info.java #1652

Closed
Stephan202 opened this issue May 31, 2020 · 0 comments · Fixed by #2078
Closed

UnnecessarilyFullyQualified and package-info.java #1652

Stephan202 opened this issue May 31, 2020 · 0 comments · Fixed by #2078

Comments

@Stephan202
Copy link
Contributor

Consider the following code:

@com.google.errorprone.annotations.CheckReturnValue
package foo;

Applying the UnnecessarilyFullyQualified check to this code:

wget \
  https://repo1.maven.org/maven2/com/google/errorprone/error_prone_core/2.4.0/error_prone_core-2.4.0-with-dependencies.jar
wget \
  https://repo1.maven.org/maven2/com/google/errorprone/error_prone_annotations/2.4.0/error_prone_annotations-2.4.0.jar
javac \
  -XDcompilePolicy=simple \
  -processorpath error_prone_core-2.4.0-with-dependencies.jar \
  '-Xplugin:ErrorProne -XepPatchChecks:UnnecessarilyFullyQualified -XepPatchLocation:/tmp' \
  -cp error_prone_annotations-2.4.0.jar \
  foo/package-info.java

This yields:

--- /tmp/foo/package-info.java
+++ /tmp/foo/package-info.java
@@ -1,3 +1,5 @@
-@com.google.errorprone.annotations.CheckReturnValue
+@CheckReturnValue
 package foo;
 
+import com.google.errorprone.annotations.CheckReturnValue;
+

In the case of package-level annotations, single-use fully qualified types arguably result in more straightforward code, as otherwise the type is used before it is imported.

(In case a type is referenced multiple times (perhaps in a nested annotation or Javadoc), I guess importing does make sense. 🤔 )

copybara-service bot pushed a commit that referenced this issue Jan 8, 2021
copybara-service bot pushed a commit that referenced this issue Jan 8, 2021
stevie400 pushed a commit to HubSpot/error-prone that referenced this issue Jan 15, 2021
copybara-service bot pushed a commit that referenced this issue Jun 28, 2021
A colleague using Windows reported the issue discussed in #1652, even though we use (a fork of) Error Prone 2.7.1, which contains a fix for said issue.

The problem appears to be that `UnnecessarilyFullyQualified` looks for a forward slash in `compilationUnitTree.getSourceFile().getName()`, though `FileObject#getName()` says:
```java
    /**
     * Returns a user-friendly name for this file object.  The exact
     * value returned is not specified but implementations should take
     * care to preserve names as given by the user.  For example, if
     * the user writes the filename {@code "BobsApp\Test.java"} on
     * the command line, this method should return {@code
     * "BobsApp\Test.java"} whereas the {@linkplain #toUri toUri}
     * method might return {@code
     * file:///C:/Documents%20and%20Settings/UncleBob/BobsApp/Test.java}.
     *
     * @return a user-friendly name
     */
```

I found a similar bit of code in the `PackageInfo` check. This change replaces the forward slash with `File.separatorChar`. An alternative (presumably slightly less performant) fix is to rely on `FileObject#toUri` instead. Let me know if you prefer that approach.

I'll update our fork with this change and ask my colleague to test the fix.

Fixes #2404

COPYBARA_INTEGRATE_REVIEW=#2404 from PicnicSupermarket:bugfix/windows-package-info-detection e928dae
PiperOrigin-RevId: 381954227
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 a pull request may close this issue.

1 participant