Skip to content

Make updated Java parser fully functional #49

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

Closed
jeffvalk opened this issue Apr 22, 2019 · 6 comments
Closed

Make updated Java parser fully functional #49

jeffvalk opened this issue Apr 22, 2019 · 6 comments

Comments

@jeffvalk
Copy link
Contributor

The updated Java parser (added in 33f0791) works with JDK9+, but currently only supports non-modular sources. Practically speaking, it can parse everything except the Java 9+ platform sources in src.zip -- but this is primary use case.

The machinery for parsing modular sources is in place. The issue is the new access policies enforced for modules. Need to figure out how to have the compiler read these without throwing up.

@jeffvalk
Copy link
Contributor Author

jeffvalk commented Apr 22, 2019

The issue mentioned here may be caused by modular source parsing being disabled.

Currently, orchard.eldoc-test produces the following error occasionally (it's not reproducable on every run):

ERROR in (bug-hunt) (HashMap.java:1134)
expected: (not (nil? (resolve-symbol (quote clojure.core) (quote .toString))))
  actual: java.util.ConcurrentModificationException: null
 at java.util.HashMap.computeIfAbsent (HashMap.java:1134)
    com.sun.tools.javac.model.JavacElements.unboundNameToSymbol (JavacElements.java:199)
    com.sun.tools.javac.model.JavacElements.doGetElement (JavacElements.java:182)
    com.sun.tools.javac.model.JavacElements.doGetTypeElement (JavacElements.java:172)
    com.sun.tools.javac.model.JavacElements.getTypeElement (JavacElements.java:160)
    com.sun.tools.javac.model.JavacElements.getTypeElement (JavacElements.java:87)
    orchard.java.parser$typesym.invokeStatic (parser.clj:180)
    orchard.java.parser$typesym.invoke (parser.clj:174)
  • The orchard code that triggers it is here:
    (if-let [c (.getTypeElement util t)]
  • The javax.lang.model.util.Elements implementation that causes the issue is here.
  • The actual ConcurrentModificationException is thrown in java.util.HashMap here.

This appears to be related to this patch applied to address a JDK performance issue.

The hypothesis related to the current state of orchard.java.parser is that the JDK compiler is trying to resolve a method of a Java base class (e.g. Object/toString) while loading of modular sources has been disabled, and...some compiler assumption is being broken. Still not sure what.

jeffvalk added a commit that referenced this issue Apr 22, 2019
Catch the `ConcurrentModificationException` which is thrown by the updated
`orchard.java.parser` sometimes (but not always). This is a temporary fix; the
cause isn't completely understood.

See #49
See #20
@bbatsov
Copy link
Member

bbatsov commented Apr 23, 2019

Thanks for the exhaustive research on the subject!

@jeffvalk
Copy link
Contributor Author

jeffvalk commented May 8, 2019

As of 12acdcd, the java parser has all the same functionality in JDK9+ as it did in JDK8 and earlier. That's nice to say!

@jeffvalk
Copy link
Contributor Author

jeffvalk commented May 8, 2019

The hypothesis (hope?) that this might eliminate the ConcurrentModificationException issue mentioned above did not pan out. Without the fix applied in 1ad6e36, these can still happen. The exact cause is unknown. The fix is still in place, so this occurance has no impact.

@bbatsov
Copy link
Member

bbatsov commented May 8, 2019

As of 12acdcd, the java parser has all the same functionality in JDK9+ as it did in JDK8 and earlier. That's nice to say!

Great work! Hopefully the JVM internals are now going to stay stable for a very long time!

The hypothesis (hope?) that this might eliminate the ConcurrentModificationException issue mentioned above did not pan out. Without the fix applied in 1ad6e36, these can still happen. The exact cause is unknown. The fix is still in place, so this occurance has no impact.

Thanks for the update! It's unfortunate that the problem is still around, but I guess we'll figure it out sooner or later. Worst case scenario - it will get auto-solved when JDK 8 reaches its EOL (but I assume JDK 8 is going to be around for a veeery long time).

@jeffvalk
Copy link
Contributor Author

jeffvalk commented May 9, 2019

Great work! Hopefully the JVM internals are now going to stay stable for a very long time!

We're only using public APIs now, so if it's not stable, they broke it!

Worst case scenario - it will get auto-solved when JDK 8 reaches its EOL

Regrettably, this is a JDK9+ issue. It sure looks like a JDK bug to me (we're not doing anything that isn't advertised behavior of the public API), so hopefully it will get fixed at some point. In the meanwhile, I've yet to see any actual impact, so swallowing the exception seems reasonable.

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

2 participants