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

Semantic highlighting improvements #1488

Closed
aeschli opened this issue Jun 9, 2020 · 7 comments
Closed

Semantic highlighting improvements #1488

aeschli opened this issue Jun 9, 2020 · 7 comments

Comments

@aeschli
Copy link
Collaborator

aeschli commented Jun 9, 2020

Have the following code:

package foo.bar.apiImpl;

import java.util.function.Function;

abstract class AbstractClass {
    protected Object inheritedField;
    protected abstract Object abstractMethod(Object o);
    protected static Object staticMethod() { return null; }
}

interface InterfaceName<T> {
}

public abstract class Sem<E> extends AbstractClass implements InterfaceName<String> {
	enum Color { RED, GREEN, BLUE };
	static Object staticField;
	private E field;
	private AbstractClass field2;

	@SuppressWarnings(value="all")
	public int foo(final Integer parameter) {
        abstractMethod(inheritedField);
        final int local = 42 * hashCode() + this.field.hashCode() + field2.hashCode();
		staticMethod();
		return x.length() + parameter;
	}


}

Use the Developer: Inspect Editor Tokens and Scopes tool to look at the semantic tokens

The following improvements to semantic highlighting would great to be consistent with TypeScript:

  • parameters: should get token type parameter
  • enum: Color should be enum and RED could be enumMember
  • Annotations: SuppressWarnings could be new type annotation (extends type), value should be parameter
  • instead of function, type member should be used
  • instead of always type, use class, interface ....
@fbricon
Copy link
Collaborator

fbricon commented Jun 9, 2020

@aeschli I think we can do most of those for now, but looks like we'll need an LSP4J update for enumMember and annotation.

@Eskibear since you've worked on Semantic Highlighting before, do you want to take this one?

@Eskibear
Copy link
Contributor

Eskibear commented Jun 9, 2020

Sure, I'll take it.

looks like we'll need an LSP4J update for enumMember and annotation.

Currently it's provided through DocumentSemanticTokensProvider instead of LSP, I think if it's not related with LSP4J. Am I missing anything?

@Eskibear
Copy link
Contributor

instead of function, type member should be used

As far as I know about Java, a method/function should belong to a certain class. As for anonymous lambda function, if a name is assigned, I think the name is of type variable. E.g. token "func" in Function func = (x) -> x+1;
Thus, all methods/functions should be of type member? Any exception?

@aeschli
Copy link
Collaborator Author

aeschli commented Jul 10, 2020

Just FYI, in TypeScript we give all variables that are callable the function type.

Example is a callback:

function process(a: number, callback: (resilt: number) => void)

Just mentioning, not sure this applies to Java too.

@0dinD
Copy link
Contributor

0dinD commented Jul 18, 2020

What is the reasoning behind using member instead of function? In Java, variables are not directly callable, as opposed to JavaScript or TypeScript. Functions are usually referred to as "methods" in Java, and are always members of a class, interface or an enum. The example Eskibear gave (Function func = (x) -> x+1;) is called a "Functional Interface", which is just syntactic sugar for instantiating an interface and implementing a method on it:

Function func = (x) -> x + 1;
// This is the same as doing:
Function func = new Function() {
    @Override
    int apply(int x) {
        return x + 1;
    }
}

Again, you cannot directly call the functional interface, because it is not a method, but an anonymous instance of the Function interface:

// This does not work, since "func" is not a method
int result = func(3);
// This is how you use the functional interface:
int result = func.apply(3);

So in my opinion, func should be a variable token, and apply should be a function token.

Yes, methods (or functions, if you prefer) are indeed members. But it is more specific to say that they are methods, in order to differentiate them from fields, which is what variable members are usually called in Java. Currently, methods are given the token type function and fields are given the token type property, which I believe is the most correct way to apply the tokens, while not requiring theme authors to know about Java terminology.

@0dinD
Copy link
Contributor

0dinD commented Jul 19, 2020

If the purpose of using member instead of function was to give theme authors a way to style all members in a certain way, I think it would be better to rely on token supertypes (see this documentation) rather than making the token types less specific. This way, theme authors can choose whether to style all members a certain way or to have specific colors for methods and fields.

I have implemented your suggested improvements, and some more, in eclipse-jdtls/eclipse.jdt.ls#1501. Hopefully my code is ready to merge soon, after which I can submit a pull request for this extension to declare all the new token types and modifiers, as well as their supertypes.

@fbricon fbricon added this to the End July 2020 milestone Jul 20, 2020
@fbricon
Copy link
Collaborator

fbricon commented Jul 21, 2020

fixed with eclipse-jdtls/eclipse.jdt.ls#1501

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants