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

- Fix locations for alias / undef nodes with internal symbols #715

Merged
merged 2 commits into from Jun 23, 2020

Conversation

@marcandre
Copy link
Contributor

@marcandre marcandre commented Jun 23, 2020

$ ruby-parse -L -e "alias foo x"
s(:alias,
  s(:sym, :foo),
  s(:sym, :x))
alias foo x
~~~~~ keyword         
~~~~~~~~~~~ expression
s(:sym, :foo)
alias foo x
      ~ begin       # <<< error
      ~~~ expression
s(:sym, :x)
alias foo x
          ~ begin     # <<< error
          ~ expression

I couldn't find a (simple) way to add a test, but it looks like tests aren't necessarily precise to that level. For example I temporarily changed %i[foo] to erroneously generate a begin and no test fail. Since this hasn't been noticed in so many years, clearly it's not super important. I noticed it by pure chance.

Let me know if it's necessary to add a test (which would probably involve changing assert_parses to accept some kind of annotation like ! begin or accept a block to run extra checks...

@iliabylich
Copy link
Collaborator

@iliabylich iliabylich commented Jun 23, 2020

Good catch. So you propose changing .begin to nil for such nodes, right? (Just like for symbol nodes inside %i[]). I agree, but it definitely requires some changes to assertions.

There are a few tests that verify expression of all kinds of arguments given to alias, but there are no tests asserting that begin is empty. And we have no tests asserting that individual parts of source maps are empty.

Adding something like ! begin seems to be a good idea to me. Do you want to include it here? If not I can add it by myself in a separate PR.

Also undef foo is affected:

bin/ruby-parse --28 -L -e 'undef a'
s(:undef,
  s(:sym, :a))
undef a
~~~~~ keyword
~~~~~~~ expression
s(:sym, :a)
undef a
      ~ begin
      ~ expression
@marcandre
Copy link
Contributor Author

@marcandre marcandre commented Jun 23, 2020

Yes, I mentionned undef in the title but not in the body...

I'll handle the test (and indeed I forgot ruby_motion / mac_ruby...)

@marcandre marcandre marked this pull request as draft Jun 23, 2020
@marcandre marcandre force-pushed the marcandre:fix_symbol_internal branch from 3e43194 to c231b92 Jun 23, 2020
@marcandre marcandre marked this pull request as ready for review Jun 23, 2020
@marcandre
Copy link
Contributor Author

@marcandre marcandre commented Jun 23, 2020

PR amended.

test/parse_helper.rb Outdated Show resolved Hide resolved
@marcandre
Copy link
Contributor Author

@marcandre marcandre commented Jun 23, 2020

Ok, I refactored things a bit then. Improves failures a bit, e.g.:

TestParser#test_heredoc [test/test_parser.rb:261]:
(1.8) range of "        ~~~~~~~~ heredoc_body".
Expected: 8...16
  Actual: 7...15

Instead of comparing begin_pos and then end_pos.

@marcandre marcandre force-pushed the marcandre:fix_symbol_internal branch from 9f1485f to 0473203 Jun 23, 2020
@iliabylich iliabylich changed the title Fix locations for alias / undef with internal symbols - Fix locations for alias / undef nodes with internal symbols Jun 23, 2020
@iliabylich iliabylich merged commit c3fc057 into whitequark:master Jun 23, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@iliabylich
Copy link
Collaborator

@iliabylich iliabylich commented Jun 23, 2020

@marcandre Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.