atom / language-java

Java package for Atom
Other
62 stars 58 forks source link

Field has different pattern if "this" is omitted #224

Closed rafaelrenanpacheco closed 4 years ago

rafaelrenanpacheco commented 4 years ago

Description

I'm trying to clone IntelliJ's Darcula theme, and there's an issue when trying to set the variable.other.object.property.java foreground color. In IntelliJ fields have different colors, and this is good because it allow us to avoid the this keyword and still be easy to identify fields inside a method. In VS Code this wasn't possible to do because a field without the this keyword receives the variable.other.object.java pattern, which conflicts with objects defined inside the method. If the this keyword is used, the field receives the variable.other.object.property.java pattern, which defines a field inside a method body.

I think this is a grammar bug because in both cases we are talking about the same object, so with or without the this keyword it should be a variable.other.object.property.java.

Steps to Reproduce

Token identification without this keyword: image

Token identification with this keyword: image

Expected behavior:

Both cases (with and without this) be recognized as variable.other.object.property.java.

Actual behavior:

Field without this receives variable.other.object.java

Field with this receives variable.other.object.property.java

Reproduces how often:

Always.

Versions

VS Code: 1.43.2 OS: Linux x64 5.4.28-1-MANJARO

Additional Information

I saw a grammar bug report on VS Code repository being transferred to this repository, so I opened this issue here as well.

Also when creating a code snippet for this issue, I saw that when using the field without method invocation still results in different patterns with and without the this keyword:

public class Test {

    private String field = "string";

    public String getFieldWithThis() {
        // this.field: variable.other.object.property.java
        this.field.toCharArray();

        // this.field: variable.other.property.java
        return this.field;
    }

    public String getFieldWithoutThis() {
        // field: variable.other.object.java
        field.toCharArray();

        // field: meta.method.body.java (doesn't even have a specific pattern)
        return field;
    }
}
sadikovi commented 4 years ago

Technically, issue seems to be "trailing property" vs "non-trailing property", but I agree - we should not distinguish between properties here, there is a fairly small gain to do that. I can see the current syntax being useful, for example, if you want to differentiate the last property in the chain.

This seems to be a legacy change that was copied from an another repository and has been around for a long time. Hopefully, no one relies on this difference in scopes.

I will open a pull request to update the scope, thanks.