Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Resources part of the try-with-resources statement does falsely require a trailing semicolon #88

Closed
MoritzKn opened this issue Apr 27, 2017 · 6 comments · Fixed by #146

Comments

@MoritzKn
Copy link
Contributor

MoritzKn commented Apr 27, 2017

Description

The resources part of the try-with-resources statement does falsely require a trailing semicolon.

Steps to Reproduce

  1. Open new file
  2. Enter:
class Main {
    public static void main() {
        try (Foo foo = bar) {
            
        }
    }
}

Expected behavior:
The meta.try.resources.java scope ends at the closing parenthesis and the try body gets the scope meta.try.body.java.

Actual behavior:
The meta.try.resources.java scope does never end and the try body still has the scopes meta.try.resources.java and meta.definition.variable.java.

Reproduces how often:
100%

Versions

Reproduced in:

$atom --version 
Atom    : 1.16.0
Electron: 1.3.13
Chrome  : 52.0.2743.82
Node    : 6.5.0

(build from source)

With language-java at 0.27.0 and in master (i.e. 7b4c32a).

$ uname -a
Linux arch-pc 4.10.11-1-ARCH #1 SMP PREEMPT Tue Apr 18 08:39:42 CEST 2017 x86_64 GNU/Linux

Other issues

Support for try-with-resources was originally added in #80 after being reported in #79.

Additional Information

The Java Spec defines the syntax of this language element as:

TryWithResourcesStatement:
    try ResourceSpecification Block [Catches] [Finally]
ResourceSpecification:
    ( ResourceList [;] )
ResourceList:
    Resource {; Resource}
Resource:
    {VariableModifier} UnannType VariableDeclaratorId = Expression

See: http://docs.oracle.com/javase/specs/jls/se8/html/jls-14.html#jls-14.20.3

Therefore the semicolon after the "ResourceList" is optional. Furthermore the resources in the try-with-resources can't be arbitrarily code (link it is currently the case), but have to be a special kind of assignment.

Relevant source code: grammars/java.cson L532

I would fix it myself but I have no time at the moment + few experience with atom grammars, but maybe i have some time to look into it in the future.

@50Wliu
Copy link
Contributor

50Wliu commented Apr 28, 2017

The best way to implement this is to create a separate rule for try-with-resources, because currently the variable assignment rule waits for the ending semicolon before terminating.

@50Wliu 50Wliu added the bug label Apr 28, 2017
@MoritzKn
Copy link
Contributor Author

Yeah, it isn't a normal assignment any way.
Normal assignment looks like this:

AssignmentExpression:
  ConditionalExpression 
  Assignment
Assignment:
  LeftHandSide AssignmentOperator Expression
LeftHandSide:
  ExpressionName 
  FieldAccess 
  ArrayAccess
AssignmentOperator:
  (one of)  =  *=  /=  %=  +=  -=  <<=  >>=  >>>=  &=  ^=  |=

The "Resource" from the try-with-resources statement dosen't support komplex assignments (+= etc.) for example.

@MFKoelmans
Copy link

MFKoelmans commented Sep 28, 2017

I found a hacky workaround: adding a semicolon before the closing parenthesis of the try. It is redundant there, but it fixes the highlighting of the following code.

@sadikovi
Copy link
Contributor

sadikovi commented Sep 29, 2017

Thanks for workaround! I managed to patch code for it to work:

diff --git a/grammars/java.cson b/grammars/java.cson
index 7c20d99..75bc3bf 100644
--- a/grammars/java.cson
+++ b/grammars/java.cson
@@ -1199,7 +1199,7 @@
         \\s*(=|;)
       )
     '''
-    'end': '(?=;)'
+    'end': '(?=;|\\))'
     'name': 'meta.definition.variable.java'
     'patterns': [
       {
@@ -1216,7 +1216,7 @@
         'beginCaptures':
           '0':
             'name': 'keyword.operator.assignment.java'
-        'end': '(?=;)'
+        'end': '(?=;|\\))'
         'patterns': [
           {
             'include': '#code'

This is hacky as well, however, passes all tests in master branch.

@garyrussell
Copy link

Are there any plans to merge this or fix the problem some other way?

@sadikovi
Copy link
Contributor

There is an open pull request to fix this issue, but it needs to be revisited and rebased. Will get back to it soon.

sadikovi added a commit that referenced this issue Jul 14, 2018
…n behaviour (#146)

### Requirements

* Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
* All new code requires tests to ensure against regressions

### Description of the Change

This PR updates `variables` and `member-variables` patterns and changes the behaviour of the `meta.definition.variable.java` scope. This is done to fix #88 (try-with-resources issue) to handle `;`.

I propose to define `meta.definition.variable.java` to be just declaration part without the following expression to initialise the variable, e.g. `[int a] = 5;` or `[int a];`, where `[...]` is `meta.definition.variable.java`. This results in the right hand side not having the scope, but I could potentially define and assign another one, like `meta.initialization.expression.java` or something similar.

After patch (also passes test case mentioned in the issue):
```java
class A {
  void func() {
    try (Buffer a = new Buffer()) {
      str = "str";
    }

    try (
      Buffer a = new Buffer();
      Buffer b = new Buffer();
      Buffer c = new Buffer()
    ) {
      str = "str";
    }
  }
}
```

<img width="294" alt="after-patch" src="https://user-images.githubusercontent.com/7788766/35179953-3043e56e-fe0a-11e7-8757-98f06bc0ddf2.png">

I updated the existing tests to remove `meta.definition.variable.java` wherever it is not needed, and added more tests to check the correct behaviour.

### Alternate Designs
Alternative design was considered where we apply a hack to `end` by listing `)` as another terminating character. This is a very small fix (4-8 characters change), but could potentially result in other issues, such as triggering close on closing paren (see #88 (comment)).

Another alternative design was proposed in #124, but it is more intrusive and results in duplicating variables pattern code and increases maintenance cost, since both patterns must be updated in case of any changes.

### Benefits
Fixes issue when not using `;` (which is allowed by specification) would break the highlighting.
Potentially helps to fix other problems related to variable highlighting.

### Possible Drawbacks
We remove `meta.definition.variable.java` scope from the expression that follows `=`. The left hand side is okay. Arguably, the right hand side should not be scoped as variable definition.

### Applicable Issues
Fixes #88
Relates to PR #124
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants