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

Fix array initializer highlighting #215

Merged
merged 3 commits into from
Mar 25, 2020
Merged

Fix array initializer highlighting #215

merged 3 commits into from
Mar 25, 2020

Conversation

Vigilans
Copy link
Contributor

@Vigilans Vigilans commented Nov 27, 2019

Description of the Change

Similar to #212, it uses a two-scope pattern to distinguish inner-class block and array-initializer block. it is based on #214 to provide correct end regex for inner-class and array-initializer.

Detailed description

Conside how a syntax scope terminates:

new Class() <end of new expression>
            ^~~~~ terminate here
new Class[] <end of new expression>
            ^~~~~ terminate here
new Class() {} <end of new expression>
               ^~~~~ terminate here
new Class[] {} <end of new expression>
               ^~~~~ terminate here

Previous new-expr grammar handles following ways of termination:

  • inner-class: (?<!\\])\\s*({)...(})
  • parens (square): [...]
  • parens (round): (...)
  • parens (square + curly): [...]...{...}

The usage of \s* suffers from new-line and comment-in-between pattern, and this should be relaxed using a two-scope model.

Besides, inner-class and array-initializer do not share same syntax (class-body and code), so they should fall in different scope.

So, this PR:

  • Differentiate object-new and array-declaration using two different scopes.
  • Put inner-class and array-initializer in different scopes, with similar names.

Alternate Designs

This PR repetitively uses the end regex of new expression:

'end': '(?=;|\\)|\\]|\\.|,|\\?|:|}|\\+|\\-|\\*|\\/(?!\\/|\\*)|%|!|&|\\||\\^|=)'

to determine when a scope terminates. It would be better if we could find a way to reduce its usage.

One possible alternative is: match when inner end token reached (), ] or }), not the outer token that ends it (the complex end regex).

But just like #214, it is hard to tell when ) or ] is met, whether there will be a {} block next to it. I (maybe) tried such silly regexes (cannot remember them correctly):

  • outer - (?<=\\)...}, inner: {...(?=}
    The problem with it: if there's no } after ), it won't ends, causing the whole new-expr scope won't ends.
    e.g. A[] arr = new A[]{ new A(), new A() }; - the comma won't ends first new A() because inner scope eats until } is met.

Benefits

  • Can correctly highlight array-initializer when there's any distance of spaces between ']' and '{'.
  • Can correctly highlight array-initializer when '{' is placed at new line or with a comment in between.
  • Introduced a new scope array-initializer, in correspondence with inner-class.
  • Patterns are more neat and uniform.

Possible Drawbacks

  • Undetected possible regression issues.

Applicable Issues

Fix #172
Fix #199
Fix redhat-developer/vscode-java#728

@Vigilans
Copy link
Contributor Author

Demo

Before

image

After

image

@sadikovi
Copy link
Contributor

sadikovi commented Dec 7, 2019

Can you describe what alternatives were considered?

@Vigilans
Copy link
Contributor Author

Vigilans commented Dec 7, 2019

Hi @sadikovi, I updated how the modification of this PR is formed, and the alternative I tried. Some other alternatives led to similar results.

@sadikovi
Copy link
Contributor

sadikovi commented Dec 7, 2019

Workaround is removing whitespace between ] {.

@Vigilans
Copy link
Contributor Author

Vigilans commented Dec 7, 2019

@sadikovi

Workaround is removing whitespace between ] {.

It reminds me of another alternative I tried...

(?<!\\])\\s*({) uses negate look back pattern, so although it will not match ]{, it will match <space>{ in ]<space>{.

#172 proposed another regex, using look back without negation: (?<=\\))\\s*({), but it will cause the tests to fail since the \s* prevents inner-class's left bracket { from being placed in new line.

So either array-init or inner-class with suffer from new line or comment in between as long as \s* is used, this is how another scope is introduced.

@Eskibear
Copy link
Contributor

Eskibear commented Mar 3, 2020

Good to know that someone is fixing it now! Is this going to be merged?

@sadikovi
Copy link
Contributor

sadikovi commented Mar 3, 2020

@Eskibear hopefully :)

@Vigilans Can you rebase? Also, if I remember correctly it fixes only part of the problem with array initialisers. Could you clarify? Thanks!

@Vigilans
Copy link
Contributor Author

Vigilans commented Mar 25, 2020

Hi, I've rebased my branch and removed a redundant commit.

if I remember correctly it fixes only part of the problem with array initialisers.

This PR fixes full of the problem with the choice of end regex '(?=;|\\)|\\]|\\.|,|\\?|:|}|\\+|\\-|\\*|\\/(?!\\/|\\*)|%|!|&|\\||\\^|=)'. What falls short are the alternate designs I mentioned, in compare with the repetitively used end regex they fixes only part of the problem.

@sadikovi
Copy link
Contributor

Thanks! Merging to master.

@sadikovi sadikovi merged commit eb6d555 into atom:master Mar 25, 2020
@Eskibear
Copy link
Contributor

Great appreciation! So when is it supposed to be shipped?

@sadikovi
Copy link
Contributor

I will try preparing release this weekend if I have time.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants