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

Turn off UnnecessaryLambda, ThrowSpecificity on Java 13 #1095

Closed
wants to merge 3 commits into from

Conversation

iamdanfox
Copy link
Contributor

@iamdanfox iamdanfox commented Dec 5, 2019

Before this PR

A few early adopters internally are building/running with Java13, but the recent error-prone 2.3.4 upgrade seems to have introduced a couple of blockers, seemingly with a similar root cause:

Caused by: java.lang.NoSuchMethodException: com.sun.tools.javac.comp.Resolve.findIdent(com.sun.tools.javac.comp.Env,com.sun.tools.javac.util.Name,com.sun.tools.javac.code.Kinds$KindSelector)
An unhandled exception was thrown by the Error Prone static analysis plugin.
    private static final Supplier<RuntimeException> NOT_AUTHORIZED = () -> new ServiceException(
                                                    ^
     Please report this at https://github.com/google/error-prone/issues/new and include the following:

     error-prone version: 2.3.4
     BugPattern: UnnecessaryLambda
     Stack Trace:
     java.lang.LinkageError: com.sun.tools.javac.comp.Resolve.findIdent(com.sun.tools.javac.comp.Env,com.sun.tools.javac.util.Name,com.sun.tools.javac.code.Kinds$KindSelector)
        at com.google.errorprone.util.FindIdentifiers.findIdent(FindIdentifiers.java:104)
        at com.google.errorprone.fixes.SuggestedFixes.qualifyType(SuggestedFixes.java:289)
        at com.google.errorprone.fixes.SuggestedFixes$5.visitClassType(SuggestedFixes.java:1082)
        at com.google.errorprone.fixes.SuggestedFixes$5.visitClassType(SuggestedFixes.java:1065)
        at jdk.compiler/com.sun.tools.javac.code.Type$ClassType.accept(Type.java:1010)
        at com.google.errorprone.fixes.SuggestedFixes.prettyType(SuggestedFixes.java:1064)
        at com.google.errorprone.bugpatterns.UnnecessaryLambda.lambdaToMethod(UnnecessaryLambda.java:215)
        at com.google.errorprone.bugpatterns.UnnecessaryLambda.matchVariable(UnnecessaryLambda.java:162)
        at com.google.errorprone.scanner.ErrorProneScanner.processMatchers(ErrorProneScanner.java:451)
        ...
  Caused by: java.lang.NoSuchMethodException: com.sun.tools.javac.comp.Resolve.findIdent(com.sun.tools.javac.comp.Env,com.sun.tools.javac.util.Name,com.sun.tools.javac.code.Kinds$KindSelector)
        at java.base/java.lang.Class.getDeclaredMethod(Class.java:2476)
        at com.google.errorprone.util.FindIdentifiers.findIdent(FindIdentifiers.java:98)
        ... 174 more
An unhandled exception was thrown by the Error Prone static analysis plugin.
    private Stream<SourceMessage> runInternal() throws Exception {
                                  ^
     Please report this at https://github.com/google/error-prone/issues/new and include the following:
  
     error-prone version: 2.3.4
     BugPattern: ThrowSpecificity
     Stack Trace:
     java.lang.LinkageError: com.sun.tools.javac.comp.Resolve.findIdent(com.sun.tools.javac.comp.Env,com.sun.tools.javac.util.Name,com.sun.tools.javac.code.Kinds$KindSelector)
  	at com.google.errorprone.util.FindIdentifiers.findIdent(FindIdentifiers.java:104)
  	at com.google.errorprone.fixes.SuggestedFixes.qualifyType(SuggestedFixes.java:289)
  	at com.google.errorprone.fixes.SuggestedFixes$5.visitClassType(SuggestedFixes.java:1082)
  	at com.google.errorprone.fixes.SuggestedFixes$5.visitClassType(SuggestedFixes.java:1065)
  	at jdk.compiler/com.sun.tools.javac.code.Type$ClassType.accept(Type.java:1010)
  	at com.google.errorprone.fixes.SuggestedFixes.prettyType(SuggestedFixes.java:1064)
  	at com.palantir.baseline.errorprone.ThrowSpecificity.lambda$matchMethod$1(ThrowSpecificity.java:93)
  	...
  Caused by: java.lang.NoSuchMethodException: com.sun.tools.javac.comp.Resolve.findIdent(com.sun.tools.javac.comp.Env,com.sun.tools.javac.util.Name,com.sun.tools.javac.code.Kinds$KindSelector)
  	at java.base/java.lang.Class.getDeclaredMethod(Class.java:2476)
  	at com.google.errorprone.util.FindIdentifiers.findIdent(FindIdentifiers.java:98)
  	... 276 more

After this PR

==COMMIT_MSG==
When building with Java13, two more error-prone checks (UnnecessaryLambda and ThrowSpecificity) are turned off to avoid NoSuchMethodErrors in the compiler. They still run if your project builds on Java8 or 11.
==COMMIT_MSG==

Possible downsides?

  • if we forget to re-enable these in the future then we're just taking away people's nice static analysis :/
  • honestly I think there are probably a crap ton more problems like this, so this PR is likely not completely sufficient to get people building with Java13...

@changelog-app
Copy link

changelog-app bot commented Dec 5, 2019

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

When building with Java13, two more error-prone checks (UnnecessaryLambda and ThrowSpecificity) are turned off to avoid NoSuchMethodErrors in the compiler. They still run if your project builds on Java8 or 11.

Check the box to generate changelog(s)

  • Generate changelog entry

@@ -211,6 +211,8 @@ private static void configureErrorProneOptions(
// https://github.com/google/error-prone/issues/1106
errorProneOptions.check("TypeParameterUnusedInFormals", CheckSeverity.OFF);
errorProneOptions.check("PreferCollectionConstructors", CheckSeverity.OFF);
errorProneOptions.check("UnnecessaryLambda", CheckSeverity.OFF);
errorProneOptions.check("ThrowSpecificity", CheckSeverity.OFF);
Copy link
Contributor

Choose a reason for hiding this comment

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

These are the two where we've seen failures on java13 projects so far, but all checks using SuggestedFixes.qualifyType and SuggestedFixes.prettyType are impacted:

[gradle-baseline]$ git grep "SuggestedFixes\..*Type"
baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/AssertjPrimitiveComparison.java:        String cast = '(' + SuggestedFixes.prettyType(state, null, targetType) + ") ";
baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/CatchSpecificity.java:                                .map(type -> SuggestedFixes.prettyType(state, fix, type))
baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/LambdaMethodReference.java:        return toMethodReference(SuggestedFixes.qualifyType(state, builder, symbol))
baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/MoreASTHelpers.java:                .sorted(Comparator.comparing(type -> SuggestedFixes.prettyType(state, null, type)))
baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferAssertj.java:                            + SuggestedFixes.qualifyType(state, fix, "org.assertj.core.api.HamcrestCondition")
baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferAssertj.java:                            + SuggestedFixes.qualifyType(state, fix, "org.assertj.core.api.HamcrestCondition")
baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferAssertj.java:            String qualifiedMap = SuggestedFixes.prettyType(
baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferAssertj.java:            return SuggestedFixes.qualifyType(state, fix, "org.assertj.core.api.Assertions.assertThat");
baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferBuiltInConcurrentKeySet.java:            String qualifiedType = SuggestedFixes.qualifyType(state, fix, "java.util.concurrent.ConcurrentHashMap");
baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionConstructors.java:        String collectionType = SuggestedFixes.qualifyType(state, fixBuilder, collectionClass.getName());
baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionTransform.java:                    qualifiedType = SuggestedFixes.qualifyType(state, fix, "com.google.common.collect.Lists");
baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCollectionTransform.java:                    qualifiedType = SuggestedFixes.qualifyType(state, fix, "com.google.common.collect.Collections2");
baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferListsPartition.java:                String qualifiedType = SuggestedFixes.qualifyType(state, fix, "com.google.common.collect.Lists");
baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferSafeLoggingPreconditions.java:        String logSafeQualifiedClassName = SuggestedFixes.qualifyType(state, fix, "com.palantir.logsafe.Preconditions");
baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ReadReturnValueIgnored.java:        String qualifiedReference = SuggestedFixes.qualifyType(state, fix, fullyQualifiedReplacement);
baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StreamOfEmpty.java:                    .addFix(MoreSuggestedFixes.renameInvocationRetainingTypeArguments(tree, "empty", state))
baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StrictCollectionIncompatibleType.java:        return SuggestedFixes.prettyType(null, null, type);
baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ThrowError.java:            String qualifiedName = SuggestedFixes.qualifyType(state, fix, IllegalStateException.class.getName());
baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ThrowError.java:            String qualifiedName = SuggestedFixes.qualifyType(state, fix,
baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ThrowSpecificity.java:                        .map(checkedException -> SuggestedFixes.prettyType(state, fix, checkedException))

Should we disable all these checks on java13? We should probably migrate our calls to our MoreSuggestedFixes utility with a fallback for java13 which simply adds an import and returns the class name. It would be nice to get tests running on java13 in this repo so we ca be more confident moving forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that seems much more reasonable - too much late night hacking for Dan.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done here: #1110

I think we still may want to disable UnnecessaryLambda, UnnecessaryAbstractClass, etc on jdk13 because we can't modify upstream code.

@robert3005
Copy link
Contributor

ping? This is blocking latest baseline on java 13

@stale
Copy link

stale bot commented Dec 24, 2019

This PR has been automatically marked as stale because it has not been touched in the last 14 days. If you'd like to keep it open, please leave a comment or add the 'long-lived' label, otherwise it'll be closed in 7 days.

@stale stale bot added the stale label Dec 24, 2019
@stale stale bot closed this Dec 31, 2019
@robert3005 robert3005 deleted the dfox/unblock-java-13 branch November 18, 2020 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants