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

Improve grammar after the introduction of case-in pattern matching #197

Merged
merged 7 commits into from
Nov 22, 2021

Conversation

aibaars
Copy link
Contributor

@aibaars aibaars commented Nov 22, 2021

This pull request implements some simplifications and minor fixes .

  • The "value" expression of case value in ... is no longer optional
  • The spurious singleton 'choice' in the case_match rule is removed
  • Constants in patterns now use the same node names as normal constants (ie constant and scope_expression)
  • The various forms of * and ** in array and hash patterns now use the same node names as the ones used in formal parameter lists (ie splat_parameter, hash_splat_parameter)
  • The **nil token is now named hash_splat_nil and is used in patterns as well as formal parameter lists (used to be missing)
  • pattern_pair is renamed to keyword_pattern to match the naming of keyword_parameter
  • pattern_variable was removed, it is a simple identifier and there is no real need for an additional wrapper node .
  • Ranges in patterns now use the same node names as normal range (ie range)

Checklist:

  • All tests pass in CI.
  • There are sufficient tests for the new fix/feature.
  • Grammar rules have not been renamed unless absolutely necessary.
  • The conflicts section hasn't grown too much.
  • The parser size hasn't grown too much (check the value of STATE_COUNT in src/parser.c).

* The value is not optional
* Remove singleton 'choice' from rule
@aibaars aibaars marked this pull request as draft November 22, 2021 11:45
* alias 'pattern_constant_resolution' as 'scope_resolution'
* replace 'pattern_constant' with just 'constant'
@@ -347,11 +347,9 @@ module.exports = grammar({

case_match: $ => seq(
'case',
field('value', optional($._statement)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this change is correct, since the value does appear to be optional. MRI accepts code like this:

def f(a, b)
  case
  when a > 1
    puts 'foo'
  when b == 7
    puts 'bar'
  else
    puts 'baz'
  end
end

Here is an example I found in the wild.

Compare parse.y line 3134 with line 3118.

Copy link
Contributor Author

@aibaars aibaars Nov 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That case is covered by the case rule. The expression case in pattern then ... is not valid. The missing value is only allowed in combination with when branches.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry, that makes sense.

* array_pattern_rest => splat_parameter
* hash_pattern_rest => hash_splat_parameter
* hash_pattern_norest => hash_split_nil
This is more consistent with the naming of  keyword_parameter
A pattern_variable is simply an identifier, no need for an additional
named node.
@aibaars aibaars marked this pull request as ready for review November 22, 2021 14:46
@aibaars aibaars changed the title Improve case_match rule Improve grammar after the introduction of case-in pattern matching Nov 22, 2021
Copy link
Contributor

@nickrolfe nickrolfe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went though each commit one-by-one, and they all look sensible to me.

@aibaars aibaars merged commit 951799c into tree-sitter:master Nov 22, 2021
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