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

Fix scope of generics within parenthesis #148

Closed
wants to merge 1 commit into from
Closed

Fix scope of generics within parenthesis #148

wants to merge 1 commit into from

Conversation

brennenberkley
Copy link
Contributor

@brennenberkley brennenberkley commented Jul 24, 2018

The angle brackets in value = (B<C>) obj; were being treated as comparison operators instead of punctuation.

Description of the Change

Generics within parenthesis were not being recognized, so I added a pattern to match them before anything else.

Alternate Designs

None

Benefits

Syntax highlighting will be more accurate, all tests pass

Possible Drawbacks

none

Applicable Issues

Fixes #132

The angle brackets in "value = (B<C>) obj;" were being treated as
operators instead of punctuation.
@brennenberkley
Copy link
Contributor Author

Before:
screen shot 2018-07-24 at 10 32 19 am

After:
screen shot 2018-07-24 at 10 32 01 am

@sadikovi
Copy link
Contributor

Thanks for opening a PR.

I have a couple of minor concerns with this approach; plus, it does not seem to work on the following cases:

value = (B<T<C>>) a;
value = (B<T<? extends C>>) a;

And potentially other weird generic combinations. Did you look to see if there is anything we do already for it?

@brennenberkley
Copy link
Contributor Author

I didn’t see anything else that that handles generics within parenthesis, only generics that are directly in classes and functions.

Do you have any suggestions for a better approach?

@sadikovi
Copy link
Contributor

Your approach seems reasonable, we just need to make it work for a wider set of generic expressions:). I will also look into the code closely and will let you know if there is anything we are missing in this approach.

Well done with the initial fix!

@brennenberkley
Copy link
Contributor Author

I realize that my first solution was a bit hacky, so I've been trying to come up with something else. The way we are currently matching generics every else (but not in parenthesis) is using a begin/end block. The problem with this is there is no way to control what characters are allowed other than what's in begin and end tags, so in this block

if (a < b && c > d) {
     return;
  }

  if (a < b) {
     return;
  }
  c = "b>a";

it ends up matching

< b && c >

and

< b) {
     return;
  }
  c = "b>

An alternative I tried was to use a pattern like (<)(.*?)(>) to prevent it from matching newlines, but then nested generics get terminated too early:

A<B<C>> obj = obj;

Only <B<C> get's matched, not the last >

Do you have any ideas @sadikovi?

@sadikovi
Copy link
Contributor

sadikovi commented Jul 26, 2018

I would not really throw away your initial approach, I think it can work with some minor updates. So let's keep it as a fallback plan, in case there is no general enough solution.

We had a discussion some time ago, and one of the approaches was solving from the perspective of <, >, or <<, >> operators in parenthesis. We know that one cannot use < >, it must be separated by && or ||, etc. We also know that generics should not match in this example a << b >> c.

My initial pull request was around fixing those angle brackets in parenthesis that had been originally parsed as generics, which could have been causing this issue as well (#93)

It could be useful to create such patterns of matching < and > operators in parenthesis and then falling back to generics, etc., sort of going the other way of trying to eliminate an option of angle brackets before generics. I have not tried that, may give it a go this weekend. You are very welcome to also try it and see if it makes sense at all.

Cheers!

@sadikovi
Copy link
Contributor

@brennenberkley any progress on this issue? I was thinking that we could merge your fix for now and open a follow-up issue. Let me know if you are happy with this approach. I will also try poking around a bit more and see if there is anything else we can do.

@brennenberkley
Copy link
Contributor Author

brennenberkley commented Aug 21, 2018

@sadikovi I have not worked on it recently. We can merge it for now

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

Successfully merging this pull request may close these issues.

2 participants