8320220: Compilation of cyclic hierarchy causes infinite recursion #23704
Conversation
|
👋 Welcome back acobbs! A progress list of the required criteria for merging this PR into |
|
@archiecobbs This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 116 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
@archiecobbs The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
| log.error(pos, Errors.CyclicInheritance(c)); | ||
| seenClasses.stream() | ||
| .filter(s -> !s.type.isErroneous()) | ||
| .filter(ClassSymbol.class::isInstance) |
There was a problem hiding this comment.
wouldn't a ClassSymbol method be better here like ClassSymbol::isSubClass?
There was a problem hiding this comment.
wouldn't a ClassSymbol method be better here like ClassSymbol::isSubClass?
...
this method will invoke Class::isInstance again, probably better to just do (c -> (ClassSymbol)c)?
My goal was to replicate the existing logic, but do it for every class in seenClasses instead of just the current one. But I was not able to prove to myself that every symbol in seenClasses is in fact a ClassSymbol, although that may actually be true in practice. So that's why the code is being careful not to blind casting everything in seenClasses to ClassSymbol.
If you're confident that every symbol in seenClasses is always a ClassSymbol then we can simplify this... but if so, why wasn't seenClasses declared as a Set<ClassSymbol> instead of a Set<Symbol> in the first place? I'm wondering there is some weird (probably invalid) input that could cause that assumption to be violated.
Thanks for taking a look.
There was a problem hiding this comment.
Following up on previous comment...
I'm wondering there is some weird (probably invalid) input that could cause that assumption to be violated.
In fact, if we just blindly cast then there are 3 regression test failures. In particular, this input crashes the compiler:
public class Cyclic {
static class Outer {
class Inner {}
}
static class Test<X extends Outer> {
class InnerTest extends X.Inner { InnerTest(Outer o) {o.super();} }
}
}with this error:
Finished building target 'interim-langtools' in configuration 'macosx-aarch64-server-release'
An exception has occurred in the compiler (25-internal). Please file a bug against the Java compiler via the Java bug reporting page (https://bugreport.java.com) after checking the Bug Database (https://bugs.java.com) for duplicates. Include your program, the following diagnostic, and the parameters passed to the Java compiler in your report. Thank you.
java.lang.ClassCastException: class com.sun.tools.javac.code.Symbol$TypeVariableSymbol cannot be cast to class com.sun.tools.javac.code.Symbol$ClassSymbol (com.sun.tools.javac.code.Symbol$TypeVariableSymbol and com.sun.tools.javac.code.Symbol$ClassSymbol are in module jdk.compiler.interim of loader 'app')
at jdk.compiler.interim/com.sun.tools.javac.comp.Check$CycleChecker.checkSymbol(Check.java:2329)
at jdk.compiler.interim/com.sun.tools.javac.comp.Check$CycleChecker.visitIdent(Check.java:2345)
at jdk.compiler.interim/com.sun.tools.javac.tree.JCTree$JCIdent.accept(JCTree.java:2710)
at jdk.compiler.interim/com.sun.tools.javac.tree.TreeScanner.scan(TreeScanner.java:50)
...
There was a problem hiding this comment.
yep I was thinking that tests will tell us if we could go or not with my proposal, nice that we have tests covering this, thanks for trying out
| seenClasses.stream() | ||
| .filter(s -> !s.type.isErroneous()) | ||
| .filter(ClassSymbol.class::isInstance) | ||
| .map(ClassSymbol.class::cast) |
There was a problem hiding this comment.
this method will invoke Class::isInstance again, probably better to just do (c -> (ClassSymbol)c)?
There was a problem hiding this comment.
this method will invoke Class::isInstance again, probably better to just do
(c -> (ClassSymbol)c)?
I was under the impression that with Hotspot, there was no performance difference between ClassSymbol.class::cast and c -> (ClassSymbol)c. Is that not correct?
There was a problem hiding this comment.
yep we should be fine here, probably just a matter of personal preference I guess
|
Thanks for the review! |
Thanks for taking a look. My hope is that (other than the minor performance hit) this change can only help avoid problems, because when you have a loop, the choice of starting class is fundamentally arbitrary. So if we assume the previous code, which only marked the first class in the loop as erroneous, was correct, then it must have been correct if it started with any other class in the loop (i.e., there is no "order"). Therefore, it must be correct to mark every class in the loop as erroneous 🤞 |
This input currently causes an infinite loop:
However, less complicated cycles are handled properly.
When a cycle is found, we currently:
(a) Emit a warning; and
(b) Set the symbol's type to the error type.
These two steps are done in
Check.noteCyclic().Step (b) is what normally prevents the infinite loop from happening later in the compilation. But we only do this for the first class in the loop, presumably because it would be too verbose to do (a) for every class in the loop. But that means we're also only doing (b) for the first class in the loop.
In more complicated scenarios like the bug example, that means some classes in the cycle can escape without (b) being applied. But this is incorrect (or, at least, weirdly indeterminate) because a loop is a loop no matter which class you start with.
So the solution is to continue to do (a) only to the first class in the cycle but do (b) for every class in the cycle.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23704/head:pull/23704$ git checkout pull/23704Update a local copy of the PR:
$ git checkout pull/23704$ git pull https://git.openjdk.org/jdk.git pull/23704/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 23704View PR using the GUI difftool:
$ git pr show -t 23704Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23704.diff
Using Webrev
Link to Webrev Comment