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

Support @SuppressWarnings("all") #329

Closed
tbroyer opened this issue May 18, 2015 · 15 comments
Closed

Support @SuppressWarnings("all") #329

tbroyer opened this issue May 18, 2015 · 15 comments
Assignees

Comments

@tbroyer
Copy link
Contributor

tbroyer commented May 18, 2015

Immutables.org (similar to AutoValue) uses @SuppressWarnings("all") on the files they generate.

Their generated withers use reference equality as an optimization (returning this –the current immutable object– rather than a new immutable object that would be strictly identical): https://github.com/immutables/immutables/blob/d8075fc66be685696bb2ccba36085b2437e11545/value-processor/src/org/immutables/value/processor/Immutables.generator#L1391-L1400

This causes a lot of warnings during compilation (only for strings though):

warning: [StringEquality] String comparison using reference equality instead of value equality
    if (this.authorizationEndpoint == value) {
                                   ^
    (see http://errorprone.info/bugpattern/StringEquality)
  Did you mean 'if (Objects.equals(this.authorizationEndpoint, value)) {'?

because Error Prone doesn't recognize @SuppressWarnings("all").

(note: if you believe it's a really bad practice to suppress all warnings even in code generated by an annotation processor –i.e. code you don't control–, then I'll open an issue on Immutables.org so they emit @SuppressionWarnings("StringEquality") instead)

/cc @elucash

@tbroyer
Copy link
Contributor Author

tbroyer commented May 18, 2015

Workaround if you don't need "modify-by-copy" for your value types: disable it entirely.

// Disable modify-by-copy generation by Immutables.org
// to avoid StringEquality warnings from Error Prone
@Value.Style(
    defaults = @Value.Immutable(copy = false)
)
package com.example;

import org.immutables.value.Value;

Of course using the -Xep:StringEquality:OFF compiler argument would also work, but would then also ignore your own code.

Alternatively, maybe Error Prone could have an option to ignore warnings for code generated by annotation processors? (ignoring errors might not be a good idea)


BTW, apparently, at least Eclipse and IntelliJ IDEA support @SuppressWarnings("all").

@eaftan
Copy link
Contributor

eaftan commented May 26, 2015

Sorry for the slow reply. We don't want to support @SuppressWarnings("all") because we want people to be precise in suppressing warnings, and to explain why it's safe to do so. For the code generator case, what if we turned off warnings for files with an @generated annotation? That would match what we do internally -- we don't show warnings on generated code, but we do issue errors.

@tbroyer
Copy link
Contributor Author

tbroyer commented May 26, 2015

👍
Works for me.

I wonder if that couldn't be triggered on any generated class (you have access to the information in the Filer right?) though, rather than relying on a @Generated annotation.

@cushon
Copy link
Collaborator

cushon commented Jun 26, 2015

I'm finally getting around to implementing this. @tbroyer, any ideas on how to use the Filer to tell if the sources are generated? AFAICT javac doesn't really distinguish between file objects from generated and non-generated files.

I guess we could assume that -s is set to something reasonable and check if the file came from the source output folder...

@cushon cushon self-assigned this Jun 26, 2015
@Stephan202
Copy link
Contributor

If the code generator includes it in the produced source code, you could look for the javax.annotation.Generated annotation.

@cushon
Copy link
Collaborator

cushon commented Jun 28, 2015

Yep, that's the same @Generated annotation eaftan and tbroyer mentioned above.

My plan is to add a flag that will suppress warnings in @generated code. Any fancier ways of recognizing annotation processor generated code would be in addition to recognizing @generated, not a replacement.

@elucash
Copy link

elucash commented Jun 28, 2015

AFAIK, there are no standard ways to find out if specific file was generated by annotation processing API. Javac specific classes could possibly keep track of this, but then, processing rounds and different annotation processors come into play, and also annotation processing is not the only mean to generate code. Fancier ways to detect generated code could possibly cover only fraction of such cases and, in result, have low power-to-weight ratio. So, I think, relying on the presence @Generated annotation is reliable and sufficient enough.

@tbroyer
Copy link
Contributor Author

tbroyer commented Jun 28, 2015

@cushon I was initially thinking about intercepting calls to createSourceFile, or to getJavaFileForOutput of JavaFileManager (with JavaFileObject.Kind.SOURCE).

Looking at com.sun.tools.javac.processing.JavacFile though, I see there are getGeneratedSourceNames() and getGeneratedSourceFileObjects(), so if you have direct access to the JavacFiler between each processing round, you could probably know whether a class being compiled/processed is generated (either from the enclosing top-level class name, or the location of the file being compiled).

@elucash Not all generators include a @Generated annotation, if only because Android doesn't have it in its runtime so it has to be explicitly added as a dependency there. See google/auto#240, and I see Immutables.org does exactly that too: https://github.com/immutables/immutables/blob/d8075fc66be685696bb2ccba36085b2437e11545/value-processor/src/org/immutables/value/processor/Immutables.generator#L32-L34

@cushon cushon closed this as completed in 51ec2fd Jun 30, 2015
@cushon
Copy link
Collaborator

cushon commented Jun 30, 2015

@tbroyer thanks for the pointers. Getting the information out of the filer looks doable, but I think it ends up requiring hacky reflection. I'd prefer to avoid that.

I implemented the version that just checks for @Generated. Hopefully that gets us most of the way there. If there are a lot of generators that don't use the annotation or don't want to add a dependency on a third party copy of jsr250, we can always revisit this.

@tbroyer
Copy link
Contributor Author

tbroyer commented Jun 30, 2015

@cushon Thanks. Now eagerly awaiting the next error-prone release ;-)

@cushon
Copy link
Collaborator

cushon commented Jul 1, 2015

The last release is only a week old, so it could be a few weeks before the next one. You could try it out with the snapshot releases, though: https://oss.sonatype.org/content/repositories/snapshots/com/google/errorprone/

@tbroyer
Copy link
Contributor Author

tbroyer commented Jul 2, 2015

Hmm, 2.0.5-SNAPSHOT doesn't work for me (and this is version 2.0.5-20150630.195032-3, which is what the Travis build deployed).

Repro-case:

  1. clone https://github.com/ozwillo/ozwillo-kernel
  2. git revert --no-commit aae39685c10b1a17579315e90cf444d2e94ae00f
  3. edit build.gradle to use 2.0.5-SNAPSHOT:
repositories { maven { url 'https://oss.sonatype.org/content/repositories/snapshots/' } }
configurations.errorprone {
  resolutionStrategy.force 'com.google.errorprone:error_prone_core:2.0.5-SNAPSHOT'
}

Then run ./gradlew assemble:

:oasis-webapp:compileJava
…/oasis-webapp/build/generated/source/apt/main/oasis/usecases/ImmutableDeleteAppInstance.java:115: warning: [StringEquality] String comparison using reference equality instead of value equality
      if (this.instanceId == value) {
                          ^
    (see http://errorprone.info/bugpattern/StringEquality)
  Did you mean 'if (Objects.equals(this.instanceId, value)) {'?
error: warnings found and -Werror specified
…/oasis-webapp/build/generated/source/apt/main/oasis/usecases/ImmutableChangeOrganizationStatus.java:102: warning: [StringEquality] String comparison using reference equality instead of value equality
      if (this.requesterId == value) {
                           ^
    (see http://errorprone.info/bugpattern/StringEquality)
  Did you mean 'if (Objects.equals(this.requesterId, value)) {'?
…/oasis-webapp/build/generated/source/apt/main/oasis/usecases/ImmutableChangeAppInstanceStatus.java:341: warning: [StringEquality] String comparison using reference equality instead of value equality
      if (this.requesterId == value) {
                           ^
    (see http://errorprone.info/bugpattern/StringEquality)
  Did you mean 'if (Objects.equals(this.requesterId, value)) {'?
…/oasis-webapp/build/generated/source/apt/main/oasis/usecases/ImmutableChangeAppInstanceStatus.java:693: warning: [StringEquality] String comparison using reference equality instead of value equality
      if (this.instance_id == value) {
                           ^
    (see http://errorprone.info/bugpattern/StringEquality)
  Did you mean 'if (Objects.equals(this.instance_id, value)) {'?
1 error
4 warnings

@cushon
Copy link
Collaborator

cushon commented Jul 3, 2015

I should have mentioned that it's behind a flag. Does passing -XepDisableWarningsInGeneratedCode work?

@tbroyer
Copy link
Contributor Author

tbroyer commented Jul 3, 2015

Indeed works much better ;-)
Thanks

@xenoterracide
Copy link

xenoterracide commented Dec 29, 2016

hmm... errorprone like this

    <errorprone.version>2.0.14</errorprone.version>
    <errorprone.plexuscompiler.version>2.8.1</errorprone.plexuscompiler.version>
      <plugin>
        <groupId>org.apache.maven.plugins</groupId>
        <artifactId>maven-compiler-plugin</artifactId>
        <configuration>
          <compilerId>javac-with-errorprone</compilerId>
          <forceJavacCompilerUse>true</forceJavacCompilerUse>
          <showDeprecation>true</showDeprecation>
          <showWarnings>true</showWarnings>
          <compilerArgs>
            <arg>-Xep:MissingOverride:ERROR</arg>
            <arg>-XepDisableWarningsInGeneratedCode</arg>
          </compilerArgs>
        </configuration>
        <dependencies>
          <dependency>
            <groupId>org.codehaus.plexus</groupId>
            <artifactId>plexus-compiler-javac-errorprone</artifactId>
            <version>${errorprone.plexuscompiler.version}</version>
          </dependency>
          <dependency>
            <groupId>com.google.errorprone</groupId>
            <artifactId>error_prone_core</artifactId>
            <version>${errorprone.version}</version>
          </dependency>
        </dependencies>
      </plugin>

getting this failure though on an immutables generated class.

[INFO] Compiling 13 source files to /pipeline/source/target/classes
/pipeline/source/target/generated-sources/annotations/com/xenoterracide/util/exception/TryPredicate.java:272: error: [MissingOverride] build implements method in Builder; expected @Override
    public TryPredicate<T> build() {
                           ^
    (see http://errorprone.info/bugpattern/MissingOverride)
  Did you mean '@Override public TryPredicate<T> build() {'?
1 error

I'd guess it's because it's an error and not a warning? I've set it to error because I want CI failures, but I also don't want it to fail on generated code.

davido added a commit to davido/buck that referenced this issue Jan 9, 2017
Summary: This fixes OptionalEquality bug pattern flagged by error prone.

[OptionalEquality] Comparison using reference equality instead of value
equality
    if (this.stubBinaryPath == value) return this;
                            ^
    (see http://errorprone.info/bugpattern/OptionalEquality)
  Did you mean 'if (Objects.equals(this.stubBinaryPath, value)) return
this;' or 'if (this.stubBinaryPath.equals(value)) return this;'?

Test Plan: CI.

References:

google/error-prone#329
google/error-prone#505

immutables/immutables#502
immutables/immutables#519
lucamilanesio pushed a commit to GerritCodeReview/gerrit that referenced this issue May 25, 2019
The one suppression is already adequately explained, and Error Prone
intentionally doesn't respect @SuppressWarnings("all"):
google/error-prone#329

Change-Id: I76f21fff3d727c3c49aa83d6af54145ab45717e5
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

No branches or pull requests

6 participants