atom/language-java

Highlight `var` keyword

Closed this issue · 8 comments

mtman commented

Java 10 var keyword is not highlighted, which makes declarations visually ambiguous with calls. That makes code significantly harder to navigate.

sadikovi commented

Can you post screenshots/snippets of what it looks like in Java 10?

mtman commented

image

In case of second declaration, var is not recognized as storage.type and launchSpecification as variable.other.

sadikovi commented

Relates to #136. There are some valid arguments for and against having var.

This is a code snippet for different combinations of using var that we would have to consider (I am not sure if var can be a local variable name in Java 10):

class Test {
  // should not be highlighted
  var d = new D();
  
  void func() {
    A a = new A();
    var b = new B();
    // should be scoped as a variable name
    C var = new C();
    // should be scoped as a variable name
    var = new C();
  }
}

Does not seem like a top priority at the moment. @50Wliu do you think we should add var support? Seems like it could work by adding variables-local, see diff below (there is a bit of code duplication and we would have to add variables-local to method body separately):

diff --git a/grammars/java.cson b/grammars/java.cson
index bf91a33..bd6fe58 100644
--- a/grammars/java.cson
+++ b/grammars/java.cson
@@ -835,6 +835,9 @@
           {
             'include': '#code'
           }
+          {
+            'include': '#variables-local'
+          }
         ]
       }
       {
@@ -1375,3 +1378,22 @@
         'include': '#code'
       }
     ]
+  'variables-local':
+    'begin': '(?=\\b(var)\\b\\s+[A-Za-z_$][\\w$]*\\s*(=|;))'
+    'end': '(?=\\=|;)'
+    'name': 'meta.definition.variable.local.java'
+    'patterns': [
+      {
+        'match': '\\bvar\\b'
+        'name': 'storage.type.local.java'
+      }
+      {
+        'match': '([A-Za-z$_][\\w$]*)(?=\\s*(\\[\\])*\\s*(;|=))'
+        'captures':
+          '1':
+            'name': 'variable.other.definition.java'
+      }
+      {
+        'include': '#code'
+      }
+    ]
mtman commented

Another scenario is for (var c : listOfC) { }. The following regex covers all declarations: \\b(var)\\b\\s+[A-Za-z_$][\\w$]*\\b\\s*[=:]

There are some valid arguments for and against having var.

var is intrinsically expected to be highlighted alongside void and real types. Both Eclipse and Intellij work that way. Here's a more on point discussion: https://bugs.eclipse.org/bugs/show_bug.cgi?id=532313

One improvement would be highlighting void,var and the primitive types as keywords, while class C and new X(), except the primitive types as complex types. Currently new X() is incorrectly highlighted as a method for unlisted types, but that should be a new issue.

sadikovi commented

Thanks @mtman! That is very useful, I forgot about for loops. Let me know if you would like to open a PR to add support for var (which I would encourage you to do, if you have time), otherwise I will try fixing it this weekend:).

@50Wliu how would you go about adding var support? Would love to hear your opinion on the issue.

@mtman I did not get part with new X(). I am pretty sure it is highlighted correctly in the context of X x = new X();. Would you mind to clarify that statement?

mtman commented

@sadikovi
I've moved back to Intellij because of all the bugs in a plugin from Red Hat, so I don't have the motivation to learn the Atom language system anymore, but it looks like you know what you are doing.

The issue is that new C1() is blue like the method name, but it should be yellow since it's a type and not a method:
image

sadikovi commented
mtman commented

@sadikovi The plugin is for VS Code, but it apparently uses this package, see redhat-developer/vscode-java#604 It has issues like random delays for auto completion to show up, or sometimes errors that were e.g. commented out prevent the build to succeed.

Thanks for your effort, it's always useful to have multiple editors that are competitive.