Semantic highlighting improvements #1501

0dinD · 2020-07-08T18:05:03Z

Implements redhat-developer/vscode-java#1488
Fixes redhat-developer/vscode-java#1514

Changes

Added some javadoc comments to the semantic highlighting classes
Fixed indentation of the semantic highlighting classes, as per the .editorconfig

Token types and modifiers

Both token types and modifiers can now be found in their respective enum.
Made the naming scheme more consistent
- The enum members now have "Java-names", such as TokenType.METHOD instead of TokenType.FUNCTION
- The toString() method on the enum members return the more generic name used in the legend.
Removed unused token types
Added new token types:
- Class
- Interface
- Enum
- Enum member
- Type parameter
- Parameter
- Annotation member (Example: value in @SuppressWarnings(value = "all"))
Added new token modifiers:
- Declaration

Semantic tokens visitor

Moved all the logic that figures out applicable token types and modifiers into TokenType and TokenModifier enums.
No longer merges the entire qualified name of a package into one token
- This is to avoid inconsistencies with the "dots", for example:
  - The dot between java and lang in java.lang.Math is part of a namespace token
  - The dot between lang and Math is not part of a namespace token
  - It does not make sense for any of the dots to be part of a namespace token
- I am aware that this change was implemented to fix Semantic highlighting doesn't like java.* packages redhat-developer/vscode-java#1434
  - But in my testing this issue seems to no longer appear even with my revert
  - If someone finds that this change introduces the issue again, we can revert it
Now tries to more precisely highlight the invocation of a constructor, fixes Incorrect semantic highlighting for complex constructor invocations redhat-developer/vscode-java#1514
Now applies token modifiers in cases where it previously did not. (Types and annotations, for example.)

Testing

I did not manage to run the tests, since they failed halfway through even before I made any changes to the code (I will try to debug this soon). At least one test will fail because of the change I made to the merging of package names, so that test will have to be rewritten. Also, I haven't written any tests for the above changes, since I could not set up testing to work on my machine. It would be very kind of someone to help me write these tests, maybe @Eskibear since you wrote the other semantic highlighting tests? I will try to get testing to work for me in the meantime.

eclipse-ls-bot · 2020-07-08T18:05:07Z

Can one of the admins verify this patch?

fbricon · 2020-07-08T22:51:33Z

add to whitelist

0dinD · 2020-07-08T23:15:29Z

Some of the tests for semantic tokens are failing now because they are expecting the token type type, but are getting the more specific class, interface or enum token types. This is fixable by just testing for the correct token type.

A related issue to this is that color themes using the old type token type will no longer work correctly. This can be fixed by giving the more specific token types a supertype of type. I don't know if it can be done via the semantic tokens API, but one solution is to declare the semantic token types in the contribution point "semanticTokenTypes" in the package.json of vscode-java.

See https://code.visualstudio.com/api/language-extensions/semantic-highlight-guide#semantic-token-classification

fbricon · 2020-07-09T08:23:06Z

@0dinD thanks for your contribution! Please make sure your commits are signed (git commit -s) and sign the Eclipse Contributor Agreement, before we can accept this PR.

For the tests, we're aware of some unstable tests that might case some build failures now and again. Now, given the scope of your changes, you can safely simply amend and run the SemanticTokensCommandTest (mvn clean verify -Dtest=SemanticTokensCommandTest).

I'll let @Eskibear do the review for this one.

0dinD · 2020-07-09T12:30:40Z

Thank you for the suggestion about the tests! I have now fixed the existing tests to make them run, but I think some new tests should be added because of the additions I made.

I will look into signing the Eclipse Contributor Agreement and signing my commits.

Eskibear

I also played for a while and it was working as expected. Overall it's a great improvement, and I have below concerns:

It does bring Semantic highlighting doesn't like java.* packages redhat-developer/vscode-java#1434 back, and we should fix that or at least work it around.
It works well now for cases in Incorrect semantic highlighting for complex constructor invocations redhat-developer/vscode-java#1514 , better to add some test cases to guard.

Eskibear · 2020-07-10T04:26:14Z

...e.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/semantictokens/SemanticTokensVisitor.java

+
+	@Override
+	public boolean visit(SimpleName node) {
+		TokenType tokenType = TokenType.getApplicableToken(node.resolveBinding());


redhat-developer/vscode-java#1434 is introduced again. And by the way it's not only for java.

0dinD · 2020-07-10T11:01:22Z

As far as redhat-developer/vscode-java#1434 goes, I think it would be better to fix whatever causes the binding of the package to be null, rather than working around the fact that it is null. Can you provide me with the environment and some instructions to reproduce this issue? I am currently unable to debug it since I cannot reproduce it.

I will get to work adding new test cases for my changes.

Eskibear · 2020-07-10T12:48:34Z

Can you provide me with the environment and some instructions to reproduce this issue? I am currently unable to debug it since I cannot reproduce it.

I'm using a mac, and I have JDK 8 and 14, the issue occurs for both JDKs. The project is a simple folder without any project management tool. You can create an empty folder, open it in vscode, and create a .java file under the folder.

openjdk version "14.0.1" 2020-04-14
OpenJDK Runtime Environment Zulu14.28+21-CA (build 14.0.1+8)
OpenJDK 64-Bit Server VM Zulu14.28+21-CA (build 14.0.1+8, mixed mode, sharing)

java version "1.8.0_191"
Java(TM) SE Runtime Environment (build 1.8.0_191-b12)
Java HotSpot(TM) 64-Bit Server VM (build 25.191-b12, mixed mode)

I also observed one thing (not sure if it's related), if you get an error for import <package-name>.*, then the package name has no binding (not correctly highlighted).
When I switch to Java 8, there is a warning instead of the error, but the highligting is still broken.

0dinD · 2020-07-10T15:41:10Z

After some testing, I think I can conclude that this issue has something to do with the version of Java used to launch the server.
I am using Linux (Debian 10), and tried to reproduce this with your instructions using AdoptOpenJDK 8, 11 and 14 (hotspot).

Using JDK8 it works like expected:

Using JDK11 or JDK14 I get the same issue as you do:

When I switch to Java 8, there is a warning instead of the error, but the highligting is still broken.

Are you sure that Java 8 is used to start the server? As in, "java.home": "/path/to/jdk-8/" in settings.json.

Can you confirm my results on your machine? Any ideas as to why this might be happening?
@fbricon @Eskibear

0dinD · 2020-07-10T16:13:02Z

After thinking about it, I have a strong suspicion that this issue has something to do with the module system introduced in Java 9. That would explain why it does not appear on JDK8, and why it seems to only happen with the Java standard library. I will try to find out if this is the case, and what can be done about it.

0dinD · 2020-07-10T17:24:00Z

I have now found an easier way to reproduce this issue. It is not necessarily dependent upon the JDK version used to launch the server, as I previously thought. Rather, the issue is introduced whenever the property org.eclipse.jdt.core.compiler.compliance is set to 9 or higher in the file <workspace>/.settings/org.eclipse.jdt.core.prefs.

This pretty much confirms my suspicion of Java 9 modules being the cause of this issue. Now it's just a matter of figuring out why SimpleName#resolveBinding() returns null for some simple names that are linked to Java 9 modules. Does anyone have an idea?

Also, can you verify that this issue is reproduced by setting org.eclipse.jdt.core.compiler.compliance to 9 or higher? (And also that it isn't an issue if the setting is 1.8 or lower.)

0dinD · 2020-07-10T21:27:03Z

I have now finally found the root cause of this issue!

The actual issue

SimpleName#resolveBinding() returns null for many packages in the Java standard library. The expected result is a package binding, which is needed in order to add the correct semantic token type. The lack of a package binding can be worked around, like in the previous fix, but this causes other problems, as noted in my first comment.

Why can the package binding not be resolved?

Because it is a ProblemPackageBinding, which means that it will never be returned by DefaultBindingResolver#getPackageBinding().

Why is it a `ProblemPackageBinding`?

Because the package is not accessible. It is not accessible because it is not explicitly exported by the module it is contained in. In the example below, the java package is not exported by any module in the Java standard library, so it cannot be accessed. This does not mean that sub-packages like java.awt cannot be accessed, it only applies to the java package.

This explains why some simple names in import statements cannot have their package binding resolved: those packages are not accessible. The example below might clarify which simple names can get their package bindings resolved:

The current vs expected behavior

Looking at the above screenshot, the import statement of lines 3, 6 and 7 do not have any package bindings. This is probably correct since they are completely invalid import statements anyway. (In Java 9+)

However, the import statements of lines 4 and 8 contain packages with missing package bindings (hence the missing semantic tokens). One these lines, one would expect package bindings to be available since the import statements are valid. But they are not, since the packages java, org and org.ietf are not explicitly exported from any modules in the Java standard library.

To clarify the expected behavior:

A package binding should not be classified as a ProblemPackageBinding (see this line) unless the package itself is the target of an invalid import statement.

What can be done about this?

We can either continue to work around this issue, or get it fixed. The problem is of course that the fix needs to be made in eclipse.jdt.core, not in eclipse.jdt.ls.

What do you think is the best course of action @fbricon ? Maybe you can talk to someone at Eclipse about the issue to get it fixed? Or is it not worth it?

0dinD · 2020-07-11T02:02:08Z

After making the root cause of the issue more clear, I realised that there is a better way to work around the null package bindings (See 7b53dce). I think the logic behind it should be pretty clear by looking at the small code addition and my javadoc comment.

This is how the semantic highlighting looks now:

Can you review this change? @Eskibear

fbricon · 2020-07-15T09:30:36Z

@0dinD in order for us to be able to merge this PR, make sure you squash all your commits into one, and sign it off using git commit -s, you committer email should be identical to the one used for signing the ECA.

fbricon · 2020-07-15T09:33:43Z

Even if you have unresolved packages, they should still be highlighted as packages. Same should go for java.* and others, for consistency.

0dinD · 2020-07-15T11:46:54Z

@fbricon I will make sure to squash all my commits and sign once the code seems ready to merge.

As for your second comment, I do agree, but this behavior is not something that I changed with 7b53dce (correct me if I am wrong). Even with the current version of eclipse.jdt.ls, semantic tokens are missing for imports where the package bindings are null as a result of package accessibility for Java 9 modules. Merging of the qualified name (the current workaround) still only works if the import statement's target package does not resolve to a ProblemPackageBinding.

This screenshot is taken with the marketplace version of vscode-java:

As I said earlier, the package bindings are only missing with org.eclipse.jdt.core.compiler.compliance set to 9 or higher. The issue with missing package bindings needs to be fixed in eclipse.jdt.core, or else we need to create yet another workaround. I do have an idea for a workaround that might fix this, which I can implement if you want me to.

The image you sent is another strange issue, but is seemingly unrelated to the package bindings. I can reproduce the same issue on the marketplace version of vscode-java. The semantic tokens on dont and exists are type (or class in this PR), which is strange. I can look into fixing that issue in this PR if you would like.

Eskibear · 2020-07-16T03:31:31Z

...e.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/semantictokens/SemanticTokensVisitor.java

+
+	@Override
+	public boolean visit(QualifiedName node) {
+		if (node.resolveBinding() instanceof IPackageBinding) {


Can add || node.resolveBinding() instanceof ITypeBinding to cover more cases. E.g.

import org.jcp.*; import org.ietf.Eskibear;

Eskibear · 2020-07-16T03:45:25Z

I just verified your latest commit 7b53dce :

it does fix the regression Check for suspicious gradle-wrapper.jar #1434 , e.g. import java.util.*
it does not cover the new issue we found here, e.g. a) import org.ietf.Eskibear, b) import java.*

Room to improve

See below snippet:

For a)
we can at least cover cases in Line 6 & 7, by applying the same workaround to ITypeBinding .

For b)
now what I can think of is, to check whether its parent is a ImportDeclaration. E.g. for QualifiedName org.xml,

> node
QualifiedName@584 "org.xml"
> node.getParent()
ImportDeclaration@586 "import org.xml.*;"

But it doesn't fix import java.* because "java" is a SimpleName.

Alternative implementation

So here comes an alternative approach. As the issues are all about import statements, can we just do it in override visit(ImportDeclaration)?

0dinD · 2020-07-16T08:28:30Z

Yes, overriding visit(ImportDeclaration) was an idea that I had. If we want to give semantic tokens to import java.*; then I don't think there is any other way to do it, since there are no bindings that can be resolved in that import statement. I will take a look at implementing a new workaround based on that solution.

0dinD · 2020-07-17T13:53:16Z

After thinking about this some more, I realised that we do still need to visit qualified names, since package names not only occur in import statements, but also in cases where Types have their package explicitly specified:

// Stupid example, but you get the point
java.lang.String string = new java.lang.String();

The main problem here is that we are doing guesswork as far as applying the package tokens, since they are discarded by DefaultBindingResolver#getPackageBinding() (see this line). I have no idea whether that is the intended behavior, but it does make life harder for us.

Despite this, I made yet another workaround that should fix the comments you have made about the package highlighting. Can you review my latest commit @Eskibear ?

Eskibear · 2020-07-20T06:43:36Z

Great. I tried you PR, it manages to cover all the cases we mentioned above.

On-demand (star) imports. e.g import java.*, import org.xml.*. Coverred by overriding visit(ImportDeclaration) + isOnDemand() check.
Type imports. e.g. import java.util.List. Coverred by overriding visit(QualifiedName)

So overall it looks good to me.

I realised that we do still need to visit qualified names, since package names not only occur in import statements, but also in cases where Types have their package explicitly specified

Not true, the problems only occur in import statements. resolveBinding works well for fully qualified name (e.g. java.lang.String aString = new java.lang.String() ) in the body.

So my original proposal was to only override visit(ImportDeclaration), roughly as below:

@Override
public boolean visit(ImportDeclaration node) {
  if node.isOnDemand()
      visitPackage(node.getName())   // to highlight "java" in "import java.util.*"
  else
      visitPackageWithType(node.getName()) // to highlight "java" in "import java.util.List"
}

// in "visitPackageWithType", you can assign correct tokens for Qualifier and Name .

An advantage of this proposal would be better performance. It only affects import statements, thus you don't have to perform the check isPackageName (where resolveBinding() is costly) for any other qualifiled names in the method body.

fbricon · 2020-07-20T09:27:48Z

I'm planning to perform a release on Wednesday, so if we can get that in, that'd be great. @0dinD make sure you get your signed commits + ECA tomorrow at the latest.

0dinD · 2020-07-20T09:48:38Z

Okay @fbricon , I can probably commit some final touches (addressing Eskibear's comment + adding new test cases) later today. I will make sure to squash and sign everything so you can merge it tomorrow if it looks ready to you.

By the way, these changes might break styling for projects not using the new, more specific token types (Class, Interface, Enum, Parameter etc). For users of vscode-java this should not be a problem if we declare token types and their supertypes in the package.json contribution point. I plan to make a pull request for that also later today, which you should probably take a look at before you use the new version of eclipse.jdt.ls in vscode-java. For other projects using eclipse.jdt.ls, they probably have to figure out token supertypes in their own implementation, since what I am talking about is only available to VS Code extensions.

This is the token type hierarchy I was thinking of declaring:

namespace
type
- class
- interface
- enum
- annotation
- typeParameter
member
- function
  - annotationMember
- property
  - enumMember
variable
- parameter

Do you agree? I am unsure of whether or not to give annotationMember a supertype of function or property, since they are technically methods but probably not often thought of as such when using them in an annotation.

0dinD · 2020-07-20T22:38:00Z

I have now addressed Eskibear's comment in 3473f16. The new implementation only performs a check for null package bindings in import statements, when absolutely necessary. Also, I added some new test cases (483c0ad) to cover the new token types and modifiers I added in this PR. @Eskibear can you do a review of these changes?

Finally, I have signed the ECA and squashed my old commits into a new signed commit. So feel free to merge this if you feel that it is ready, @fbricon . Also don't forget to merge my PR redhat-developer/vscode-java#1537 for declaring token supertypes in the vscode-java extension if you merge this PR, since it ensures backwards compatibility for VS Code themes using the "old" token types.

fbricon · 2020-07-20T23:27:33Z

...e.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/semantictokens/SemanticTokensVisitor.java

+		return new SemanticTokens(data);
+	}
+
+	private List<Integer> encodeTokens() {


encodedTokens

Fixed and amended to previous commit

Eskibear · 2020-07-21T02:05:58Z

LGTM

Signed-off-by: 0dinD <[email protected]>

fbricon · 2020-07-21T10:10:35Z

@0dinD awesome work, thanks!

tommai78101 · 2020-07-24T13:56:29Z

@0dinD @fbricon Is it me or, after the merge, the formatting is no longer consistent with the rest of the Eclipse code format?

For example, in org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/semantictokens/SemanticTokenManager.java, the first indentation is a 1-space wide indent, but in the original, it's a 4-space indent.

0dinD · 2020-07-24T14:14:33Z

@tommai78101 Are you sure that it is a space and not a tab character that you have configured to be 1-space wide? I re-indented everything under org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/semantictokens/ because the .editorconfig said to use tabs for indentation, but those source files were indented using spaces.

tommai78101 · 2020-07-24T22:16:41Z

@0dinD Ok, so it is really me. I was just looking at this directly, and verified it's indeed a tab character, just somehow became a 1-character-wide character:

785a6f1

Thanks.

Eskibear reviewed Jul 10, 2020

View reviewed changes

Eskibear reviewed Jul 16, 2020

View reviewed changes

0dinD mentioned this pull request Jul 19, 2020

Semantic highlighting improvements redhat-developer/vscode-java#1488

Closed

0dinD force-pushed the semantic-highlighting-improvements branch from 483c0ad to 97fe211 Compare July 20, 2020 20:40

0dinD mentioned this pull request Jul 20, 2020

Declare semantic tokens in contribution points redhat-developer/vscode-java#1537

Closed

fbricon reviewed Jul 20, 2020

View reviewed changes

Improve semantic highlighting

785a6f1

Signed-off-by: 0dinD <[email protected]>

0dinD force-pushed the semantic-highlighting-improvements branch from 97fe211 to 785a6f1 Compare July 21, 2020 08:09

fbricon merged commit 2eb3c19 into eclipse-jdtls:master Jul 21, 2020

fbricon added enhancement semantic highlighting labels Jul 21, 2020

fbricon added this to the End July 2020 milestone Jul 21, 2020

0dinD mentioned this pull request Jul 24, 2020

Incorrect semantic tokens for some wildcard import statements redhat-developer/vscode-java#1545

Closed

0dinD mentioned this pull request Nov 18, 2020

Enhance highlighting experience microsoft/vscode-java-pack#270

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Semantic highlighting improvements #1501

Semantic highlighting improvements #1501

0dinD commented Jul 8, 2020 •

edited

Loading

eclipse-ls-bot commented Jul 8, 2020

fbricon commented Jul 8, 2020

0dinD commented Jul 8, 2020 •

edited

Loading

fbricon commented Jul 9, 2020 •

edited

Loading

0dinD commented Jul 9, 2020

Eskibear left a comment

Eskibear Jul 10, 2020

0dinD commented Jul 10, 2020

Eskibear commented Jul 10, 2020 •

edited

Loading

0dinD commented Jul 10, 2020

0dinD commented Jul 10, 2020

0dinD commented Jul 10, 2020

0dinD commented Jul 10, 2020

0dinD commented Jul 11, 2020 •

edited

Loading

fbricon commented Jul 15, 2020

fbricon commented Jul 15, 2020

0dinD commented Jul 15, 2020 •

edited

Loading

Eskibear Jul 16, 2020

Eskibear commented Jul 16, 2020

0dinD commented Jul 16, 2020

0dinD commented Jul 17, 2020

Eskibear commented Jul 20, 2020

fbricon commented Jul 20, 2020

0dinD commented Jul 20, 2020

0dinD commented Jul 20, 2020

fbricon Jul 20, 2020

0dinD Jul 21, 2020

Eskibear commented Jul 21, 2020

fbricon commented Jul 21, 2020

tommai78101 commented Jul 24, 2020

0dinD commented Jul 24, 2020

tommai78101 commented Jul 24, 2020 •

edited

Loading

Semantic highlighting improvements #1501

Semantic highlighting improvements #1501

Conversation

0dinD commented Jul 8, 2020 • edited Loading

Changes

Token types and modifiers

Semantic tokens visitor

Testing

eclipse-ls-bot commented Jul 8, 2020

fbricon commented Jul 8, 2020

0dinD commented Jul 8, 2020 • edited Loading

fbricon commented Jul 9, 2020 • edited Loading

0dinD commented Jul 9, 2020

Eskibear left a comment

Choose a reason for hiding this comment

Eskibear Jul 10, 2020

Choose a reason for hiding this comment

0dinD commented Jul 10, 2020

Eskibear commented Jul 10, 2020 • edited Loading

0dinD commented Jul 10, 2020

Using JDK8 it works like expected:

Using JDK11 or JDK14 I get the same issue as you do:

0dinD commented Jul 10, 2020

0dinD commented Jul 10, 2020

0dinD commented Jul 10, 2020

The actual issue

Why can the package binding not be resolved?

Why is it a ProblemPackageBinding?

The current vs expected behavior

To clarify the expected behavior:

What can be done about this?

0dinD commented Jul 11, 2020 • edited Loading

This is how the semantic highlighting looks now:

fbricon commented Jul 15, 2020

fbricon commented Jul 15, 2020

0dinD commented Jul 15, 2020 • edited Loading

This screenshot is taken with the marketplace version of vscode-java:

Eskibear Jul 16, 2020

Choose a reason for hiding this comment

Eskibear commented Jul 16, 2020

Room to improve

Alternative implementation

0dinD commented Jul 16, 2020

0dinD commented Jul 17, 2020

Eskibear commented Jul 20, 2020

fbricon commented Jul 20, 2020

0dinD commented Jul 20, 2020

0dinD commented Jul 20, 2020

fbricon Jul 20, 2020

Choose a reason for hiding this comment

0dinD Jul 21, 2020

Choose a reason for hiding this comment

Eskibear commented Jul 21, 2020

fbricon commented Jul 21, 2020

tommai78101 commented Jul 24, 2020

0dinD commented Jul 24, 2020

tommai78101 commented Jul 24, 2020 • edited Loading

0dinD commented Jul 8, 2020 •

edited

Loading

0dinD commented Jul 8, 2020 •

edited

Loading

fbricon commented Jul 9, 2020 •

edited

Loading

Eskibear commented Jul 10, 2020 •

edited

Loading

Why is it a `ProblemPackageBinding`?

0dinD commented Jul 11, 2020 •

edited

Loading

0dinD commented Jul 15, 2020 •

edited

Loading

tommai78101 commented Jul 24, 2020 •

edited

Loading