[Java] More fixes! #727

djspiewak · 2016-12-10T08:55:05Z

Lots of stuff. In no particular order:

Removed all of the lookbehind (updates Remove Incompatible Regex Patterns #481)
Added support for lambdas (I'm pretty sure anyway…)
Fixed scoping of ?, extends and super inside of generics
Removed a metric boatload of storage.type
Fixed some random punctuation scoping that was incorrect (including .)
Added kitchy invalid scoping on primitive-instantiated generics (it's pretty safe to say these will always be invalid, given how generics are implemented on the JVM)
Implemented full-state field declaration parsing

There's a bit of a question here though, and I don't know the answer. Specifically:

private String memberString = "Hello";

What should the scope of String be? Clearly, if we replace String with int, the answer is storage.type.primitive.java with some additional meta scoping for fun. But it's a lot less clear what we do if it's a non-primitive type.

The C++ mode says "no scoping at all", which is certainly a perspective I can rationalize. The Scala mode says either support.class.java or support.type.java (probably the former, since Java is super class-oriented). The strict reading of the scoping guidelines says storage.type.java.

Initially, I tried storage.type, but then I ran into the following signature:

private Map<Foo, Bar> map = null;

Should the storage.type just cover the Map? Or should it cover the whole type? Both look very weird. And what if the type contains a wildcard, resulting in some keyword.operator scoping in the middle of our storage.type? That doesn't make any sense.

By the same token, extending the Scala support.class/support.type regime to Java also has some problems. < is just a more syntactically heavyweight token than [, which means that Java's generics just seem more visually intrusive when the operators are not colored uniformly with the surrounding types. Or maybe I'm just used to reading Scala. Either way, it seems pretty darn weird.

I left the support.type and support.class scopes commented out, just so I can remember where they need to be rematerialized. For the time being, we're just going to go with the C++ mode's minimalist scoping, rather than Scala's rich (and complicated) scoping. Java types are a lot closer to C++ types (in terms of average complexity) than Scala types, so I think that's somewhat justifiable, even if it does make code look a little plain.

Anyway, I think the only remaining item for the Java mode is to fix annotations, which you could argue is pending a resolution on #709.

Disclaimer I do not use Java on a regular basis. Or at all, really. So unlike the Scala mode, I'm not dogfooding any of this. I'm happy to fix bugs and corner cases, but I'm probably not going to find them myself.

…lass

keith-hall · 2016-12-10T11:06:15Z

With private Map<Foo, Bar> map = null;, I would personally apply a meta scope to the whole Map<Foo, Bar>, and scope Map, Foo and Bar separately as types, and give punctuation scopes to the <, , and >. I think this is how the C# PR does it.

djspiewak · 2016-12-11T00:43:15Z

Would you scope them as support.class?

…yping lookahead

djspiewak · 2016-12-11T01:06:18Z

Restored scoping and employed support.class and support.type.

wbond

Thanks for all of this work. This looks like it will be a nice step forward for the Java syntax!

I think most of the changes are missing scopes or some slight tweaks.

wbond · 2016-12-13T19:32:12Z

Java/Java.sublime-syntax

+    - match: '{{primitives}}'
+      scope: storage.type.primitive.java
+      set: instantiation-arrays
+    - match: '{{qualified_id}}'


For this I'd like to use a look-ahead and them push into a scope where we match . as punctuation.accessor.

Missed this in my initial scan of your review…

This would be a trickier change, since we're nesting {{qualified_id}} matches within other lookahead contexts (for field matching). It's not impossible, and I agree it's the right thing to do, it's just probably going to be very messy. If it's alright with you, I'd rather tilt that windmill in a separate PR, since it's going to require some deeper changes.

wbond · 2016-12-13T19:43:51Z

Java/Java.sublime-syntax

  keywords:
+    - match: '::'


This should probably be either punctuation.accesor (that would be for more "color" consistency at the expense of semantic correctness), or keyword.operator for semantic correctness, correct?

I think keyword.operator.method-reference might make sense. punctuation.accessor would also be sort of accurate. This is a deference operator which returns a pointer rather than invoking the method. I don't have a strong feeling about it, but I will say that my color scheme (which colors keyword.operator and not punctuation) makes the test example look nicer with a keyword-based scope than a punctuation one.

From reading about this it seemed as though most authors considered it more of an operator, in that it transforms something, rather than a basic accessor. So let's go with keyword.

keyword.operator.method-reference.java it is!

but I will say that my color scheme (which colors keyword.operator and not punctuation) makes the test example look nicer with a keyword-based scope than a punctuation one.

That's totally subjective though. I'm fine with my Python code not showing dots as blue (for keyword.operator).

Either way, this is apparently a discussion about whether or not accessors are in fact "operators". I vote for a keyword.operator.accessor scope for all of them, if we were to decide on them being operators.
By the way, semantically this has the same scope as dot access for attributes like this.field_name, but I don't think those are scoped as keyword.operator, are they?

If all these were punctuation.accessor.<character-identifier> (for example punctuation.accessor.double-colon), a color scheme could simply highlight all accessors that use two colons for some kind of member access with a simple selector.

By the way, semantically this has the same scope as dot access for attributes

It doesn't really though. The best analogue is honestly the -> vs . operators in C, except sort of inverted. The . operator in C directly accesses a member, while the -> operator dereferences a pointer and then accesses the member. The . operator in Java accesses a member function and applies it, while the :: operator accesses a member function and creates a pointer to it (without application).

foo::bar

In C syntax:

&(foo->bar)

In the C/C++ mode, both . and -> are given scope punctuation.accessor. In Java, . is (now) given punctuation.accessor while :: is given keyword.operator.method-reference. That's certainly a bit inconsistent.

By another perspective though, the :: operator in Java is analogous to the & operator in C (creating a pointer), which is scoped as keyword.operator.c.

I agree with both of you, and as you can see from my original comment I didn't have a strong opinion either way.

For consistency with other punctuation operators, including the :: in PHP, let's go with punctuation.accessor. That was my first instinct, however the docs I had been reading about it made it sound as if it was doing more. I think the visual consistency and style-ability wins here over pure semantic correctness. Kind of like how we scope docstrings in Python as comments because they are used as comments, but technically are just strings.

@wbond I'll make the change. I preferred keyword.operator before we were scoping the identifier on the right (since it offered a strong visual distinction), but now that we're scoping the id, there's no real reason to avoid the punctuation scope.

Either way, I don't have much of a horse in the race since I don't use Java at all. :-D

Well, I certainly appreciate all of these fixes you are doing considering you never really use it!

wbond · 2016-12-13T19:44:22Z

Java/Java.sublime-syntax

+    - match: '::'
+      scope: keyword.control.method-reference.java
+      push:
+        - match: '{{id}}'


Is there a scope we could add here that would be useful?

Perhaps variable.function?

Yeah, now that you confirmed this is for static references to methods, I believe that would be the correct scope.

wbond · 2016-12-13T19:45:05Z

Java/Java.sublime-syntax

@@ -270,8 +390,17 @@ contexts:
          push:


Add a scope for the (?

wbond · 2016-12-13T19:45:28Z

Java/Java.sublime-syntax

@@ -270,8 +390,17 @@ contexts:
          push:
            - meta_scope: meta.method.identifier.java
            - match: \)
+              scope: meta.method.identifier.java


Probably should add a punctuation scope for the ).

wbond · 2016-12-13T19:46:15Z

Java/Java.sublime-syntax

+          set:
+            - meta_scope: meta.generic.java
+            - match: '>'
+              scope: punctuation.definition.generic.end.java
              pop: true


Should we be including a context here to scope the types in the generic list?

Most definitely. This is a bug.

Oh, correction: it's already included on line 429. I was puzzled when I attempted to add a failing test and it passed. :-)

wbond · 2016-12-13T19:48:10Z

Java/Java.sublime-syntax

+      set: method-parameter-after
+    - match: ','
+      scope: punctuation.separator.java
+    - match: \)


Let's add a punctuation scope here

wbond · 2016-12-13T19:49:36Z

Java/Java.sublime-syntax

+            - match: '<'
+              scope: punctuation.definition.generic.begin.java
+              set:
+                - match: '>'


We should add meta.generic.java to this, right?

djspiewak · 2016-12-13T22:55:43Z

@wbond I believe everything except the qualified_id bit is now addressed. As I said, due to the way this interacts with the other lookahead contexts, I would prefer to address that in another PR where I can fix some of the other structural issues with the mode.

FichteFoll · 2016-12-14T15:22:12Z

Note that #481 will be closed when this PR is merged becuase it includes "fixes #481" in the OP. I highly suggest editing that to read "updates #481" instead.

djspiewak · 2016-12-14T18:19:37Z

@FichteFoll Done.

…tor here

djspiewak · 2016-12-14T18:36:03Z

@wbond Done. If you approve the deferral (to a subsequent PR) on the qualified_id issue, this should be good to go.

FichteFoll · 2016-12-14T20:30:07Z

Java/Java.sublime-syntax

  keywords:
+    - match: '::'
+      scope: punctuation.accessor.java


Would you mind changing this to punctuation.accessor.double-colon.java?

FichteFoll · 2016-12-14T20:30:19Z

Java/Java.sublime-syntax

-    - match: (?<=\S)\.(?=\S)
-      scope: keyword.operator.dereference.java
+    - match: \.
+      scope: punctuation.accessor.java


And this to punctuation.accessor.dot.

I think this seems like a reasonable idea @FichteFoll. I'll add it to the official guidelines, and then we can tweak existing syntaxes as they are worked on.

Great. Some suggestions from my side.

. → dot
: → colon
:: → double-colon
-> → arrow
=> → fat-arrow ? (probably not used as an accessor anywhere but whatever)
\ → backslash (yes, Moonscript uses this...)

PHP uses \ for namespace "accessor", in the way that C++ uses ::, although right now I believe in PHP it is scoped as a punctuation.separator.namespace.

I suppose it would make sense to standardize to accessor since most other syntaxes use accessor for namespace resolution.

Additionally, : is used by Lua. Anyway, we can probably continue any further conversation about this on a new issue rather than flooding this PR.

djspiewak · 2016-12-14T23:54:09Z

@FichteFoll Revised to use dot and double-colon. I agree on your proposed guidelines, btw. Using the syntactic name for these operators rather than a language-specific semantic term seems the most sane.

wbond · 2016-12-15T11:08:58Z

Thanks!

djspiewak added 8 commits December 9, 2016 23:49

[Java] Removed excess storage.type from parameters

4829ebd

[Java] Comments should be in prototype (and main to workaround bug)

be404cc

[Java] Reimplemented support for instantiation and field matching

4bc3956

[Java] Fixed broken accessor scoping

42ac420

[Java] Added scoping for lambdas

4dc6869

[Java] Fixed up the tests; biased in favor of C++ mode

184bd56

[Java] Revised function generic instantiation to be consistent with c…

a864e51

…lass

[Java] Fixed generic instantiation in inheritance to be consistent

192544d

djspiewak added 2 commits December 10, 2016 17:59

[Java] Restored field, parameter and generic scoping

8c1f880

[Java] Made generic definition sites a bit more uniform; fixed some t…

9e37c69

…yping lookahead

wbond requested changes Dec 13, 2016

View reviewed changes

djspiewak added 4 commits December 13, 2016 15:36

[Java] Adjusted scoping on method reference stuff

d2bd72b

[Java] Fixed overflow on test issue

21826fa

[Java] Added punctuation scoping for method parameter groups

ab082df

[Java] Added meta.generic scope for throwables

de009bc

[Java] punctuation.accessor seems more appropriate than keyword.opera…

cc4f8f5

…tor here

FichteFoll reviewed Dec 14, 2016

View reviewed changes

[Java] Revised punctuation.accessor to be slightly more specific

2cbe1a7

FichteFoll mentioned this pull request Dec 15, 2016

[Scala] add support for annotations #605

Merged

wbond merged commit c5b019e into sublimehq:master Dec 15, 2016

djspiewak deleted the feature/java-fixes branch December 24, 2016 07:55

[Java] More fixes! #727

[Java] More fixes! #727

Conversation

djspiewak commented Dec 10, 2016 • edited Loading

keith-hall commented Dec 10, 2016 • edited Loading

djspiewak commented Dec 11, 2016

djspiewak commented Dec 11, 2016

wbond left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FichteFoll Dec 14, 2016 • edited Loading

Choose a reason for hiding this comment

djspiewak Dec 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djspiewak commented Dec 13, 2016

FichteFoll commented Dec 14, 2016 • edited Loading

djspiewak commented Dec 14, 2016

djspiewak commented Dec 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FichteFoll Dec 14, 2016 • edited Loading

Choose a reason for hiding this comment

wbond Dec 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djspiewak commented Dec 14, 2016

wbond commented Dec 15, 2016

djspiewak commented Dec 10, 2016 •

edited

Loading

keith-hall commented Dec 10, 2016 •

edited

Loading

FichteFoll Dec 14, 2016 •

edited

Loading

djspiewak Dec 14, 2016 •

edited

Loading

FichteFoll commented Dec 14, 2016 •

edited

Loading

FichteFoll Dec 14, 2016 •

edited

Loading

wbond Dec 14, 2016 •

edited

Loading