New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attempt to fix overlap of generics 2 #93

Merged
merged 10 commits into from Oct 31, 2017

Conversation

Projects
None yet
2 participants
@sadikovi
Member

sadikovi commented May 17, 2017

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

In released version of Atom 1.17.0 and in current master branch there is an issue when expression inside if (...) is parsed as generics when capitalized variable is used with < and some other combinations, see screen shots below. This is also observed when bitwise operators are used. Result is that such partial generic affects all subsequent highlighting (including other functions declarations, etc) as shown on image below. For most part it is merely minor rather annoying, out of ordinary highlighting of variables, not critical at all.

  • Examples that break highlighting: if (A < a) { ... }, if (A < a && b < a) { ... }, and if (A < a || b > a) { ... }, and t = M << x
  • Examples that work: if (a < A) { ... }, if (a < b) { ... }

I tried fixing most of the cases, currently I make sure that pattern for object types terminates when ; is found, which is basically replacement of (?<=>) with (?<!;). I am still not sure why it works, and would appreciate if someone could explain. From manual tests, it looks like this fixes situations with usage && and || in if-else statements, which my previous PR reported as not covered.

Before

before1

before2

After

after1

after2

Alternate Designs

Some of the designs were proposed in #75, but this solution looks fairly simple and does not break existing tests.

Benefits

Current solution is simple, alternative would require more code, with potentially refactoring large portion of syntax highlighting. This solution also preserves all patterns for generics, and does not have constraint of my previous pull request (generic on single line only).

Possible Drawbacks

The change might break some other untested cases.

Applicable Issues

@sadikovi

This comment has been minimized.

Show comment
Hide comment
@sadikovi

sadikovi May 17, 2017

Member

@50Wliu @noseglid could you review this pull request? This is the second take on generics after #75.

Could you also test manually some examples with this patch? I tested some cases, but I am not sure if I covered generics corner cases, or situations which this patch breaks.

Member

sadikovi commented May 17, 2017

@50Wliu @noseglid could you review this pull request? This is the second take on generics after #75.

Could you also test manually some examples with this patch? I tested some cases, but I am not sure if I covered generics corner cases, or situations which this patch breaks.

@50Wliu 50Wliu added the needs-review label May 17, 2017

@sadikovi

This comment has been minimized.

Show comment
Hide comment
@sadikovi

sadikovi Aug 9, 2017

Member

@50Wliu could you review this pull request, please? Thanks!

Member

sadikovi commented Aug 9, 2017

@50Wliu could you review this pull request, please? Thanks!

@50Wliu 50Wliu self-requested a review Aug 9, 2017

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Aug 9, 2017

Member

I must say that I'm still unhappy with this solution, however it may be the best option available. It's been a while since I last looked at this, so give me a few days to try to get reassociated with the code.

Member

50Wliu commented Aug 9, 2017

I must say that I'm still unhappy with this solution, however it may be the best option available. It's been a while since I last looked at this, so give me a few days to try to get reassociated with the code.

@sadikovi

This comment has been minimized.

Show comment
Hide comment
@sadikovi

sadikovi Aug 9, 2017

Member

When debugging this problem, I noted that generics pattern was correct, but generic termination capturing was somehow loose, therefore I decided to essentially enforce rule of terminating generic after ;. Added tests to make sure new cases were covered, and old test cases passed.

I am happy to try a different approach to this problem, if you want.

Member

sadikovi commented Aug 9, 2017

When debugging this problem, I noted that generics pattern was correct, but generic termination capturing was somehow loose, therefore I decided to essentially enforce rule of terminating generic after ;. Added tests to make sure new cases were covered, and old test cases passed.

I am happy to try a different approach to this problem, if you want.

@sadikovi

This comment has been minimized.

Show comment
Hide comment
@sadikovi

sadikovi Aug 29, 2017

Member

@50Wliu did you have a chance to look at this problem and pull request? I am not sure what the next step should be. Should I close this PR and try coming up with new solution or should we just mark it as known problem? Thanks.

Member

sadikovi commented Aug 29, 2017

@50Wliu did you have a chance to look at this problem and pull request? I am not sure what the next step should be. Should I close this PR and try coming up with new solution or should we just mark it as known problem? Thanks.

@sadikovi

This comment has been minimized.

Show comment
Hide comment
@sadikovi

sadikovi Sep 4, 2017

Member

@50Wliu I also found that this code:

class A<T> {
  private B<T>[] arr;
}

is not highlighted correctly in master branch (latest commit 652e8ba), which I might have broken.

Found that this PR fixes this problem and returns correct highlight:)

Should I add test for this issue?

Member

sadikovi commented Sep 4, 2017

@50Wliu I also found that this code:

class A<T> {
  private B<T>[] arr;
}

is not highlighted correctly in master branch (latest commit 652e8ba), which I might have broken.

Found that this PR fixes this problem and returns correct highlight:)

Should I add test for this issue?

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Sep 16, 2017

Member

Yes please, if it isn't already tested! Sorry for the late reply.

Member

50Wliu commented Sep 16, 2017

Yes please, if it isn't already tested! Sorry for the late reply.

@sadikovi sadikovi changed the title from Attempt to fix overlap of generics 2 to Attempt to fix overlap of generics 2 + generics highlight Sep 17, 2017

@sadikovi sadikovi changed the title from Attempt to fix overlap of generics 2 + generics highlight to Attempt to fix overlap of generics 2 Sep 17, 2017

Revert "add test for generic array"
This reverts commit 16c1cd3.
@sadikovi

This comment has been minimized.

Show comment
Hide comment
@sadikovi

sadikovi Sep 17, 2017

Member

@50Wliu I was wrong, this PR does not fix the problem of tokenising generic arrays. I will create a new pull request to patch that issue (and it does not break it either).

Member

sadikovi commented Sep 17, 2017

@50Wliu I was wrong, this PR does not fix the problem of tokenising generic arrays. I will create a new pull request to patch that issue (and it does not break it either).

@sadikovi

This comment has been minimized.

Show comment
Hide comment
@sadikovi

sadikovi Sep 17, 2017

Member

I created PR to fix aforementioned issue: #110

Member

sadikovi commented Sep 17, 2017

I created PR to fix aforementioned issue: #110

Ivan Sadikov
@sadikovi

This comment has been minimized.

Show comment
Hide comment
@sadikovi

sadikovi Oct 4, 2017

Member

@50Wliu I rebased to master. Passes all tests locally. Should we proceed with this fix or should we address this issue differently?

Member

sadikovi commented Oct 4, 2017

@50Wliu I rebased to master. Passes all tests locally. Should we proceed with this fix or should we address this issue differently?

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Oct 16, 2017

Member

I want to apologize for the continued delays. I really want to review this, I just haven't had enough time to do so. Sorry to keep you waiting.

Member

50Wliu commented Oct 16, 2017

I want to apologize for the continued delays. I really want to review this, I just haven't had enough time to do so. Sorry to keep you waiting.

@sadikovi

This comment has been minimized.

Show comment
Hide comment
@sadikovi

sadikovi Oct 16, 2017

Member

@50Wliu It is okay, no worries, I am just curious what we do about the issue and resolution. It does not seem to be particularly common, though examples in description are still reproducible in the latest master branch.

Member

sadikovi commented Oct 16, 2017

@50Wliu It is okay, no worries, I am just curious what we do about the issue and resolution. It does not seem to be particularly common, though examples in description are still reproducible in the latest master branch.

Show outdated Hide outdated grammars/java.cson
Show outdated Hide outdated grammars/java.cson
@sadikovi

This comment has been minimized.

Show comment
Hide comment
@sadikovi

sadikovi Oct 25, 2017

Member

@50Wliu, Thanks for the review! I rebased and addressed your comments. I left (?<!;) lookbehind, instead of using lookahead, but it looks like either works.

Member

sadikovi commented Oct 25, 2017

@50Wliu, Thanks for the review! I rebased and addressed your comments. I left (?<!;) lookbehind, instead of using lookahead, but it looks like either works.

50Wliu added some commits Oct 31, 2017

@sadikovi

This comment has been minimized.

Show comment
Hide comment
@sadikovi

sadikovi Oct 31, 2017

Member

@50Wliu Thanks for patching this. Apologies - I somehow missed your answer in comments.

Member

sadikovi commented Oct 31, 2017

@50Wliu Thanks for patching this. Apologies - I somehow missed your answer in comments.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Oct 31, 2017

Member

One last thing - would it be more efficient to use (?<=>)|(?!;)? That way we can exit the pattern earlier if a generic is detected.

Member

50Wliu commented Oct 31, 2017

One last thing - would it be more efficient to use (?<=>)|(?!;)? That way we can exit the pattern earlier if a generic is detected.

@sadikovi

This comment has been minimized.

Show comment
Hide comment
@sadikovi

sadikovi Oct 31, 2017

Member

It passes tests, and I manually ran several examples - I agree, let's use (?<=>)|(?!;) instead of just (?!;).

Member

sadikovi commented Oct 31, 2017

It passes tests, and I manually ran several examples - I agree, let's use (?<=>)|(?!;) instead of just (?!;).

@sadikovi

This comment has been minimized.

Show comment
Hide comment
@sadikovi

sadikovi Oct 31, 2017

Member

@50Wliu Are you going to update the pattern? Otherwise, I could push commit, if you want.

Member

sadikovi commented Oct 31, 2017

@50Wliu Are you going to update the pattern? Otherwise, I could push commit, if you want.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Oct 31, 2017

Member

I'll update it :).

Member

50Wliu commented Oct 31, 2017

I'll update it :).

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Oct 31, 2017

Member

Thanks for your patience with this 🙇

Member

50Wliu commented Oct 31, 2017

Thanks for your patience with this 🙇

@50Wliu 50Wliu merged commit 833c2a9 into atom:master Oct 31, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sadikovi

This comment has been minimized.

Show comment
Hide comment
@sadikovi

sadikovi Oct 31, 2017

Member

Thanks for merging!

Member

sadikovi commented Oct 31, 2017

Thanks for merging!

@sadikovi sadikovi deleted the sadikovi:generics-fix branch Oct 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment