Skip to content
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

Add support for labels to Kotlin lexer #1496

Merged
merged 3 commits into from
Apr 14, 2020

Conversation

goodhoko
Copy link
Contributor

Hello,
I'm trying to add support for labels to the Kotlin lexer. I managed to get it work for the label definition. Eg.

loop@ for (i in 1..100) {

but can't make it work for the rule for label reference. Eg.

if (...) break@loop 

I suspect the look behind group is the cause but I'm not really sure. I'd like to ask for help on this. .)

For reference how labels are used in Kotlin see the Break and Continue Labels.

@pyrmont pyrmont changed the title Help needed: Add support for labels to Kotlin lexer Add support for labels to Kotlin lexer Apr 12, 2020
@pyrmont pyrmont marked this pull request as draft April 12, 2020 00:39
@pyrmont pyrmont self-assigned this Apr 12, 2020
@pyrmont pyrmont added the needs-review The PR needs to be reviewed label Apr 12, 2020
@pyrmont
Copy link
Contributor

pyrmont commented Apr 12, 2020

@goodhoko Sorry about the confusion. This should have worked but unfortunately didn't because of a longstanding bug in the StringScanner library that Rouge uses. As a result of the bug, lookbehinds don't work :( That bug has been fixed but enabling the fix is opt-in and we haven't enabled that yet (see #1491 for more details).

In any event, the simplest fix is to match the terms in the lookbehind earlier in the state. That's what I've done with the latest commit. Let me know how it looks on your end.

@pyrmont pyrmont added author-action The PR has been reviewed but action by the author is needed and removed needs-review The PR needs to be reviewed labels Apr 12, 2020
@goodhoko
Copy link
Contributor Author

@pyrmont thanks for the link it all makes sense now. Also thanks for your commit. It's completely ok from my side so we can merge, possibly squashing the commits? .)

By the way I noticed that there's quite a lot of look behind groups in rouge lexers.

# not even accounting for negative look behind
goodhoko:~/Workspace/rouge/lib/rouge/lexers$ grep -rn '(?<=' .
./scala.rb:30:      idrest = %r(#{letter}(?:#{letter}|#{digits})*(?:(?<=_)#{op}+)?)x
./julia.rb:224:        rule %r/(?<=[.\w)\]])\'+/, Operator
./javascript.rb:148:        rule %r((?<=\n)(?=\s|/|<!--)), Text, :expr_start
./actionscript.rb:114:        rule %r((?<=\n)(?=\s|/|<!--)), Text, :expr_start
./viml.rb:36:        rule %r/(?<=\s)"[^-:.%#=*].*?$/, Comment
./elixir.rb:30:             (?<=[\s])!+|&(&&?|(?!\d))|\|\||\^|\*|\+|\-|/|
./awk.rb:106:        rule %r((?<=\n)(?=\s|/)), Text, :expr_start
./bsl.rb:12:      KEYWORDS = /(?<=[^\wа-яё]|^)(?:
./bsl.rb:30:      BUILTINS = /(?<=[^\wа-яё]|^)(?:
./bsl.rb:67:        rule %r/(?<=[^\wа-яё]|^)\&.*$/, Keyword::Declaration
./bsl.rb:69:        rule %r/(?<=[^\wа-яё]|^)\#.*$/, Keyword::Declaration
./bsl.rb:78:        rule %r/(?<=[^\wа-яё]|^)\|((?!\"\").)*?(\"|$)/, Literal::String
./perl.rb:98:        rule %r(((?<==~)|(?<=\())\s*/(\\\\|\\/|[^/])*/[msixpodualngc]*),
./erlang.rb:80:        rule %r{(?:^|(?<=:))(#{atom_re})(\s*)(\()} do

Does this mean they all don't work?

@goodhoko
Copy link
Contributor Author

Uh, maybe wait a little, I'll also add some tests for the labels to the lexer spec.

@goodhoko
Copy link
Contributor Author

@pyrmont OK, LGTM. Should be squashed to have feature and tests in one commit. 👍

// I come from GitLab where squashing is a single click before merging. Let me know if I need to do this manually here.

@pyrmont pyrmont marked this pull request as ready for review April 14, 2020 00:47
@pyrmont
Copy link
Contributor

pyrmont commented Apr 14, 2020

@goodhoko I think that yes, those rules probably don't match precisely as intended. That said, in most cases, I'd expect it results in syntactically incorrect code lexing without problems rather than syntactically correct code having errors. Since syntax correctness is a non-goal of Rouge that's not the biggest problem (although it is something to consider when we move to the improved StringScanner).

In any event, thank you for adding the tests. These all look good—will merge in now (our merging strategy is squash and merge when we accept PRs so nothing for you to do!).

@pyrmont pyrmont merged commit dd39655 into rouge-ruby:master Apr 14, 2020
@pyrmont pyrmont removed the author-action The PR has been reviewed but action by the author is needed label Apr 14, 2020
@pyrmont
Copy link
Contributor

pyrmont commented Apr 14, 2020

@goodhoko OK, merged in now. Thanks for the PR! This will be part of the next release of Rouge. That's scheduled for Tuesday 14 April which, depending on where you are when you read this, is either today or tomorrow 🎉

mattt pushed a commit to NSHipster/rouge that referenced this pull request May 21, 2020
This commit adds support for labels to the Kotlin lexer.
pyrmont added a commit that referenced this pull request Nov 9, 2020
Rouge will currently match identifiers in the Kotlin lexer that begin
with certain keywords (such as `super`). This is because of a change in
#1496 that added a rule to support labels. That change should have
updated the rule to end with a `\b` so as to ensure that identifiers
that begin with the listed keywords are followed by a word break.
This commit fixes that error.
mattt pushed a commit to NSHipster/rouge that referenced this pull request May 19, 2021
Rouge will currently match identifiers in the Kotlin lexer that begin
with certain keywords (such as `super`). This is because of a change in
rouge-ruby#1496 that added a rule to support labels. That change should have
updated the rule to end with a `\b` so as to ensure that identifiers
that begin with the listed keywords are followed by a word break.
This commit fixes that error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants