-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[prone] fix UnnecessarilyFullyQualified
#5189
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import junitbuild.extensions.dependencyFromLibs | ||
| import net.ltgt.gradle.errorprone.errorprone | ||
| import net.ltgt.gradle.nullaway.nullaway | ||
| import org.gradle.jvm.toolchain.JvmImplementation.J9 | ||
|
|
||
| plugins { | ||
| `java-library` | ||
|
|
@@ -16,17 +17,10 @@ dependencies { | |
|
|
||
| tasks.withType<JavaCompile>().configureEach { | ||
| options.errorprone { | ||
| val shouldDisableErrorProne = java.toolchain.implementation.orNull == JvmImplementation.J9 | ||
| if (name == "compileJava" && !shouldDisableErrorProne) { | ||
| disable( | ||
| "AnnotateFormatMethod", // We don`t want to use ErrorProne's annotations. | ||
| "BadImport", // This check is opinionated wrt. which method names it considers unsuitable for import which includes a few of our own methods in `ReflectionUtils` etc. | ||
| "DoNotCallSuggester", // We don`t want to use ErrorProne's annotations. | ||
| "ImmutableEnumChecker", // We don`t want to use ErrorProne's annotations. | ||
| "InlineMeSuggester", // We don`t want to use ErrorProne's annotations. | ||
| "MissingSummary", // Produces a lot of findings that we consider to be false positives, for example for package-private classes and methods. | ||
| "StringSplitter", // We don`t want to use Guava. | ||
| "UnnecessaryLambda", // The findings of this check are subjective because a named constant can be more readable in many cases. | ||
| val enableErrorProne = java.toolchain.implementation.orNull != J9 | ||
| if (name == "compileJava" && enableErrorProne) { | ||
| disableAllWarnings = true // considering this immense spam burden, remove this once to fix dedicated flaw. https://github.com/diffplug/spotless/pull/2766 | ||
| disable( // We don`t want to use ErrorProne's annotations. | ||
| // picnic (https://error-prone.picnic.tech) | ||
| "ConstantNaming", | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. therefore this still seems an bug, as need to disable picnic warnings, as they fail the build if not treated the extra way? isn´t it? @rickie There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @Pankraz76, Thanks for flagging the issues with the Picnic warnings in these various tickets, such as google/error-prone#5365. I want to emphasize that I appreciate you taking the time to apply our checks. It is great to see them being applied in the wild. That said, to make these reports actionable, we really need a minimal reproduction case when things break. As noted by @cushon here google/error-prone#5365 (comment) and here google/error-prone#5277 (comment), simply knowing that it fails in a repository makes it very hard for us to debug. If we can isolate the specific code causing the crash, we can fix the root cause instead of just disabling the check. To be clear, I do believe you could be right and that there is a bug in our code or in Error Prone itself. If so, we definitely want to fix it, but we need those isolated examples to make that happen. |
||
| "DirectReturn", // We don`t want to use this: https://github.com/junit-team/junit-framework/pull/5006#discussion_r2403984446 | ||
|
|
@@ -45,18 +39,23 @@ tasks.withType<JavaCompile>().configureEach { | |
| error( | ||
| "CanonicalAnnotationSyntax", | ||
| "IsInstanceLambdaUsage", | ||
| "MissingOverride", | ||
| "PackageLocation", | ||
| "RedundantStringConversion", | ||
| "RedundantStringEscape", | ||
| "SelfAssignment", | ||
| "StringCharset", | ||
| "StringJoin", | ||
| "UnnecessarilyFullyQualified", | ||
| ) | ||
| } else { | ||
| disableAllChecks = true | ||
| } | ||
| nullaway { | ||
| if (shouldDisableErrorProne) { | ||
| disable() | ||
| } else { | ||
| if (enableErrorProne) { | ||
| enable() | ||
| } else { | ||
| disable() | ||
| } | ||
| onlyNullMarked = true | ||
| isJSpecifyMode = true | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dont need to disable warnings they are already treaded like this for a reason by convention.