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

Update build matrix for Java 14 #1571

Closed
wants to merge 14 commits into from

Conversation

TimvdLippe
Copy link
Contributor

This allows us to check that ErrorProne works on Java 12. It is part of
issue #1106 to make ErrorProne work on newer versions of Java.

It will allow us to verify that #1110 indeed fixes the problem with Java
12.

This allows us to check that ErrorProne works on Java 12. It is part of
issue google#1106 to make ErrorProne work on newer versions of Java.

It will allow us to verify that google#1110 indeed fixes the problem with Java
12.
@tbroyer
Copy link
Contributor

tbroyer commented Apr 15, 2020

12 is EOL since last September, 13 for nearly one month already. Shouldn't this use JDK 14 instead?
(oh, and that method is gone in 14: https://docs.oracle.com/en/java/javase/14/docs/api/jdk.compiler/com/sun/source/tree/BreakTree.html)

FWIW google-java-format doesn't even try to be compatible with 13: google/google-java-format#436 (comment)

@don-vip
Copy link
Contributor

don-vip commented Apr 15, 2020

It should always use the latest version of Java. That means at minimum an update every 6 months.

@TimvdLippe
Copy link
Contributor Author

The compilation error I am currently hitting is a method signature changed in JDK 12 but remains in JDK 14. So we will need to fix that also for JDK 14. We are discussing mitigations in an internal thread.

@TimvdLippe TimvdLippe changed the title Update build matrix for Java 12 Update build matrix for Java 14 Apr 17, 2020
@tbroyer
Copy link
Contributor

tbroyer commented Apr 17, 2020

You may also want to migrate to Xenial or even Bionic rather than Trusty, and openjdk8 rather than oraclejdk8

Note: it is expected that this will fail on JDK 8 and 11.
This is not strictly necessary for JDK 14 and we want a minimal diff
at this point in time.
}

public abstract JCPattern getPattern();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method is problematic, as JCPattern has only been introduced in JDK 14. It implements the interface com.sun.source.tree.PatternTree, which seems like a marker interface: http://hg.openjdk.java.net/jdk/jdk14/file/6c954123ee8d/src/jdk.compiler/share/classes/com/sun/source/tree/PatternTree.java

I am not sure how we can implement this interface on JDK 14 without breaking JDK 8/11.

sym.type = typeVar;
typeVarCache.put(var.getName(), typeVar);
// Any recursive uses of var will point to the same TypeVar object generated above.
typeVar.bound = var.getUpperBound().inline(this);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These fields are removed and are instead put in the constructor. Looking at the code, it seems to make sense to put them in the constructor in the first place, since we would still override them. This should be 8/11-compatible.

@@ -282,7 +282,7 @@ private static int getPrecedence(JCTree leaf, Context context) {
return (typeCast.expr == leaf) ? TreeInfo.prefixPrec : TreeInfo.noPrec;
} else if (parent instanceof JCInstanceOf) {
JCInstanceOf instanceOf = (JCInstanceOf) parent;
return TreeInfo.ordPrec + ((instanceOf.clazz == leaf) ? 1 : 0);
return TreeInfo.ordPrec + ((instanceOf.getType() == leaf) ? 1 : 0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar as below, the field has been removed and we should use getType() (which is a getter for the field).

return result.exists() ? result : null;
} catch (ReflectiveOperationException e) {
throw new LinkageError(e.getMessage(), e);
}
}

private static final class FindIdent {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pulled all of this in from #1439 to fix a lot of test failures.

@@ -665,7 +665,8 @@ public Boolean visitIdentifier(IdentifierTree node, Unifier unifier) {
return chooseSubtrees(
state,
s -> unifyStatements(node.getStatements(), s),
stmts -> maker().Case((JCExpression) node.getExpression(), stmts));
s -> unify(node.getBody(), s),
(stmts, body) -> maker().Case(CaseTree.CaseKind.STATEMENT, List.<JCExpression>of((JCExpression) node.getExpression()), stmts, body));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have to provide 2 additional arguments on JDK 14. It seems like this should be fixed similarly to the FindIdentifiers solution (e.g. reflection).

@cushon
Copy link
Collaborator

cushon commented May 19, 2020

CI is now passing for 12 and 13: 937b3d2. (14+ is still WIP: #1106 (comment))

@cushon cushon closed this May 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants