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

Handle shorthand syntax in explanations #116

Merged
merged 1 commit into from
Nov 17, 2021
Merged

Handle shorthand syntax in explanations #116

merged 1 commit into from
Nov 17, 2021

Conversation

schneems
Copy link
Collaborator

When performing a performance test on https://github.com/kddnewton/syntax_tree/blob/50f9fc1b815156bdde53f13d4f250d9522f15e64/test/syntax_tree_test.rb I noticed a strange output. The explain syntax indicated that it was missing a bracket when I had only deleted an end:

$ time DEAD_END_TIMEOUT=10 ./exe/dead_end spec/fixtures/syntax_tree.rb.txt
--> /Users/rschneeman/Documents/projects/dead_end/spec/fixtures/syntax_tree.rb.txt
Unmatched `]', missing `[' ?
Unmatched keyword, missing `end' ?
     6  class SyntaxTree < Ripper
   170    def self.parse(source)
   174    end
   176    private
❯  754    def on_args_add(arguments, argument)
❯  776    class ArgsAddBlock
❯  810    end
  9233  end
DEAD_END_TIMEOUT=10 ./exe/dead_end spec/fixtures/syntax_tree.rb.txt  1.63s user 0.07s system 99% cpu 1.703 total

The cause was this line:

        node.is_a?(Op) && %w[| ||].include?(node.value) &&

With the %w[] syntax the lexer tokenizes %w[ which (since it doesn't match [ is not counted.

This commit corrects that logic by explicitly looking for the last character of these events.

@schneems schneems force-pushed the schneems/qwords branch 2 times, most recently from 81f0c33 to 22f5b36 Compare November 17, 2021 17:30
When performing a performance test on https://github.com/kddnewton/syntax_tree/blob/50f9fc1b815156bdde53f13d4f250d9522f15e64/test/syntax_tree_test.rb I noticed a strange output. The explain syntax indicated that it was missing a bracket when I had only deleted an `end`: 

```
$ time DEAD_END_TIMEOUT=10 ./exe/dead_end spec/fixtures/syntax_tree.rb.txt
--> /Users/rschneeman/Documents/projects/dead_end/spec/fixtures/syntax_tree.rb.txt
Unmatched `]', missing `[' ?
Unmatched keyword, missing `end' ?
     6  class SyntaxTree < Ripper
   170    def self.parse(source)
   174    end
   176    private
❯  754    def on_args_add(arguments, argument)
❯  776    class ArgsAddBlock
❯  810    end
  9233  end
DEAD_END_TIMEOUT=10 ./exe/dead_end spec/fixtures/syntax_tree.rb.txt  1.63s user 0.07s system 99% cpu 1.703 total
```

The cause was this line:

```
        node.is_a?(Op) && %w[| ||].include?(node.value) &&
```

With the `%w[]` syntax the lexer tokenizes `%w[` which *since it doesn't match `[`) is not counted.

This commit corrects that logic by explicitly looking for the last character of these events.
@schneems schneems merged commit c4d3f78 into main Nov 17, 2021
@schneems schneems deleted the schneems/qwords branch November 17, 2021 17:33
schneems added a commit that referenced this pull request Nov 17, 2021
While investigating #116 I realized that the shorthand syntax does not have to be limited to our existing "pairs" characters that we already track. While coming up with additional test cases I realized that the existing `on_parse_error` does not emit an error for the code: `%Q* lol` (missing a `*` to close the string).

This commit adds the missing check (coming from `compile_error`) as well as several others coming from the event table:

```
irb(main):008:0> Ripper::PARSER_EVENT_TABLE.select {|(k,v)| k.to_s.end_with?("error") }.keys
=> [:alias_error, :assign_error, :class_name_error, :param_error, :irb(main):008:0> Ripper::PARSER_EVENT_TABLE.select {|(k,v)| k.to_s.end_with?("error") }.keys
=> [:alias_error, :assign_error, :class_name_error, :param_error, :parse_error]
```
schneems added a commit that referenced this pull request Nov 17, 2021
While investigating #116 I realized that the shorthand syntax does not have to be limited to our existing "pairs" characters that we already track. While coming up with additional test cases I realized that the existing `on_parse_error` does not emit an error for the code: `%Q* lol` (missing a `*` to close the string).

This commit adds the missing check (coming from `compile_error`) as well as several others coming from the event table:

```
irb(main):008:0> Ripper::PARSER_EVENT_TABLE.select {|(k,v)| k.to_s.end_with?("error") }.keys
=> [:alias_error, :assign_error, :class_name_error, :param_error, :irb(main):008:0> Ripper::PARSER_EVENT_TABLE.select {|(k,v)| k.to_s.end_with?("error") }.keys
=> [:alias_error, :assign_error, :class_name_error, :param_error, :parse_error]
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant