Skip to content

JDK 9+ Support#91

Merged
olafurpg merged 1 commit into
mainfrom
nsc/jdk9+
Mar 1, 2021
Merged

JDK 9+ Support#91
olafurpg merged 1 commit into
mainfrom
nsc/jdk9+

Conversation

@Strum355

@Strum355 Strum355 commented Feb 25, 2021

Copy link
Copy Markdown
Contributor

Previously, the semanticdb-javac compiler plugin only supported Java 8. The plugin failed when compiling with Java 9+ due to our use of 'unsupported' APIs that changed from JDK 9.

This PR adds support for all JDKs from 9 onwards in addition to the previously supported JDK 8 by

  1. Using APIs that have been stable (thus far) and
  2. Removing usages of (as tested thus far) redundant and unstable APIs.

Notes:

  • With JDK9+ anonymous classes and everything enclosed within don't get skipped. We should see why this happens in JDK8
  • There was 1 erroneous reference that moved to 0:0, why? JDK 9+ Support #91 (comment)

Closes #90

@@ -1,4 +1,5 @@
package minimized;
^^^^^ reference minimized/Enums#

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This one is unusual 🤔 @olafurpg

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤔 Hmmm not sure what's going on here. It's not a blocking issue. I think it makes sense that we try to document these quirks at some later point as issues. For now, it's most important to the the infrastructure working.

@olafurpg olafurpg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The diffs look like a mix of nice improvements and minor regressions. I think it's OK to merge these changes and follow up with a PR that adds new testing infrastructure to compile with java 8 and java 11 within the same sbt build.

Some blocking issues:

  • Update CI configuration to use java 11.
+++ .github/workflows/ci.yml
      - uses: olafurpg/setup-scala@v10
+       with:
+         java-version: adopt@1.11
  • Update readme to reflect Java 11 support.
  • Update title and description of the PR.The format I try to follow in PR description is something like "Previously, the semanticdb-javac compiler plugin only supported Java 8. The plugin crashed when compiling with Java 11. This PR adds support for Java 11 so that the plugin works on both Java 8 and Java 11.".
  • Manually confirm that the plugin work on Java 8. We can fix the testing infrastructure later to automatically prevent regressions.

Comment thread README.md
| `snapshot/testOnly tests.LibrarySnapshotSuite` | sbt | Runs slow snapshot tests. Indexes a corpus of external Java libraries. |
| `snapshot/test` | sbt | Runs all snapshot tests. |
| `snapshot/run` | sbt | Update snapshot tests. Use this command after you have fixed a bug. |
| `snapshots/testOnly tests.MinimizedSnapshotSuite` | sbt | Runs fast snapshot tests. Indexes a small set of files under `tests/minimized`. |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

}

return new Handler(looper);
// ^^^^^^^^^^^^^^^^^^^ reference `<init>`#

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks like an improvement 👍

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Im assuming this isnt correct yet, but progress?

@Override
// ^^^^^^^^ reference java/lang/Override#
public boolean areItemsTheSame(EpoxyModel<?> oldItem, EpoxyModel<?> newItem) {
// ^^^^^^^^^^^^^^^ definition com/airbnb/epoxy/EpoxyControllerAdapter#ITEM_CALLBACK.``#areItemsTheSame().

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Improvement 👍

@@ -1,4 +1,5 @@
package minimized;
^^^^^ reference minimized/Enums#

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤔 Hmmm not sure what's going on here. It's not a blocking issue. I think it makes sense that we try to document these quirks at some later point as issues. For now, it's most important to the the infrastructure working.

while (lookup != null) {
if (lookup.sym != null) {
peers.add(lookup.sym);
Iterable<? extends Element> elements = sym.owner.getEnclosedElements();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice find 👍 This looks like a decent replacement for members().lookup()


public boolean isNone(Symbol sym) {
return sym == null || sym.kind == Kinds.NIL || (sym.kind & Kinds.ERRONEOUS) != 0;
return sym == null; // || sym.kind == Kinds.Kind.NIL || sym.kind == Kinds.Kind.ERR;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems to be working ok. No? I'm OK with removing the comment.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The commented part is not backwards compatible. JDK 8 it was Kinds.NIL etc, JDK 9+ the enum was moved to be a nested enum. The behaviour appears to be identical at best, but I'm unsure if we're testing enough edge cases to surface where this change may not be sufficient

@Strum355 Strum355 changed the title ⚠️⚠️ WIP ⚠️⚠️ JDK 9+ Support Feb 25, 2021

@olafurpg olafurpg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM 🚀

@olafurpg olafurpg marked this pull request as ready for review March 1, 2021 11:15
@olafurpg olafurpg merged commit 0d693a4 into main Mar 1, 2021
@olafurpg olafurpg deleted the nsc/jdk9+ branch March 1, 2021 11:15
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

Successfully merging this pull request may close these issues.

JDK 11+ support for javac plugin indexer

2 participants