Declare semantic token type for modifier keywords

[0dinD]

Part of eclipse-jdtls/eclipse.jdt.ls#1539.

Declares semantic token type for modifier, and supertypes it to keyword. Also adds scope mapping for the custom token types, since that was missing. This means that themes still using TextMate scopes can style semantically highlighted modifier keywords, annotations and annotation members.

[fbricon]

@Eskibear please review when you can

[Eskibear]

Agree that "modifier" inherits from "keyword", otherwise vscode doesn't know the token (althought it's defined in LSP3.16)

[Eskibear]

Taking @SuppressWarnings(value="all") for example. "value" is annotationMember and its definition is:

public @interface SuppressWarnings {
   /*....*/
    String[] value();  //<---HERE
}

I wonder "constant" is not proper for it.

[0dinD]

I wasn't entirely sure what TextMate scope made the most sense, and just used the same scope as in the default Java TextMate grammar. If we decide to use some other scope it might break existing color themes, although I guess you could work around that by adding multiple TextMate scopes.

I do see another issue (maybe this is what you meant), which is that annotation member declarations are now given the same scope as annotation members inside of an annotation like @SuppressWarnings(value = "all"). It's definitely possible to add scope a different scope mapping for annotationMember.declaration, but the question is again which TextMate scope it should have?

In the default Java grammar, annotation member declarations are treated like regular methods, and therefore get the entity.name.function.java and meta.function-call.java scopes. We could add those scopes to annotation member declarations for backwards compatibility, but we could also add a more specific annotation member scope. Again, these alternatives doesn't have to be mutually exclusive as you can declare an array of TextMate scopes in the contribution point.

So what do you think? Should we mirror the behaviour of the default TextMate grammar (and tell users to style using semantic highlighting for more specific rules) or should we also add more specific/correct TextMate scopes?

[Eskibear]

In current behavior, "value" is highlighted as function name, because you define annotationMember as a subtype of function. And it makes some sense because of the consistency, i.e., when you go to definition of "value", it's also highligted as a function.

My point is, although default textMate grammar use "constant.other.key.java", it is not proper for the scope. constant.* is for constant values like string literals, numbers, etc. See https://macromates.com/manual/en/language_grammars#naming_conventions

And it's not picked up by the default theme.

So I prefer here to use a more specific/correct TextMate scope, maybe something looks like entity.name.annotationMember?

[0dinD]

I'm no expert on TextMate grammars and naming conventions, but I agree that entity.name.annotationMember sounds more correct than constant.other.key. I'll modify this PR to include the entity.name.annotationMember scope for annotation members then.

[fbricon]

@Eskibear @0dinD does it make sense to merge eclipse-jdtls/eclipse.jdt.ls#1539 independently from this? Or should we expect more changes upstream?

[Eskibear]

I think eclipse-jdtls/eclipse.jdt.ls#1539 is safe to merge independently.

The scope mapping is required in order for the default themes to look correct with the new modifierKeyword
-- by @0dinD

I tested eclipse-jdtls/eclipse.jdt.ls#1539 independently (without this PR), but didn't see any regression with default theme. See below, there's no textMate scope for the modifier type, and it turns out to fallback to syntax highlighting.

[0dinD]

@fbricon I initially thought that not using this PR would cause modifier keywords to lose their color, similar to what happened when we introduced the annotation and annotationMember tokens. But after testing today, I came to the same conclusion as @Eskibear did, it's safe to merge eclipse-jdtls/eclipse.jdt.ls#1539. This is because most modifier keywords are already covered by the default TextMate grammar, and don't rely on semantic highlighting.

This PR mainly adds consistency between semantic highlighting and TextMate grammars. Also, I don't think the non-sealed keyword is covered by the default TextMate grammars, which this PR + eclipse-jdtls/eclipse.jdt.ls#1539 should fix. But again, it should be safe to merge this PR a while after eclipse-jdtls/eclipse.jdt.ls#1539 if we need time to discuss it.

[0dinD]

I just updated this PR to include the entity.name.annotationMember.java TextMate scope for annotation members, as per discussion with @Eskibear. I kept the old constant.other.key.java scope as a fallback in case some old themes are still using it. Since entity.name.annotationMember.java is defined first in the contribution point, it has precedence over constant.other.key.java, if used. Do you agree with this decision?

[fbricon]

@Eskibear can we merge this one?

[Eskibear]

@fbricon LGTM, safe to merge.

[fbricon]

Thanks @0dinD and @Eskibear