Implement partial support for Ruby 2.7 pattern matching #2186

tomstuart · 2020-12-11T15:26:24Z

TruffleRuby needs support for case … in pattern matching in order to be fully compatible with Ruby 2.7 (#2004). This PR includes an initial implementation of parsing and matching simple patterns. Specifically we now support:

parsing case … in syntax
matching Arrays, Hashes and Structs against array and hash patterns (#deconstruct and #deconstruct_keys)
matching array literals against nested array patterns
assigning existing local variables in top-level patterns and inside nested arrays

Before these changes, 2% (2/89) of the relevant ruby/spec examples (spec/ruby/core/**/deconstruct* + spec/ruby/language/pattern_matching_spec.rb) were passing; now, 37% (35/94) are passing. Further parser work is required to pass all of the remaining examples.

Shopify#1

eregon

Looks good so far.

This actually reminded me I implemented pattern matching in Mozart-Graal, maybe there is some similarity: https://github.com/eregon/mozart-graal/blob/ca762ad12d505647a8a872b4993ff76b92ea643a/vm/src/org/mozartoz/truffle/translator/Translator.java#L458

Do you intend the implement the rest of pattern matching in this PR, or would you rather merge this part, and the rest later?
If the latter, I think for users it would be safer to keep raising a SyntaxError (in visitCase3Node()) until it's complete. We could have a temporary option to enable pattern matching to let specs run and to help working on it (i.e., setting the option to true would not throw the SyntaxError).

spec/ruby/core/struct/deconstruct_keys_spec.rb

src/main/java/org/truffleruby/parser/ast/Case3ParseNode.java

src/main/java/org/truffleruby/parser/ast/InParseNode.java

src/main/ruby/truffleruby/core/struct.rb

src/main/java/org/truffleruby/parser/BodyTranslator.java

tomstuart · 2020-12-11T17:41:16Z

How do I get the linting check to pass? I’ve tried several times but can’t get it to stop flip-flopping.

eregon · 2020-12-11T17:42:26Z

@tomstuart Are you using jt format? That should work fine.

eregon · 2020-12-11T17:44:44Z

IMHO the best way to deal with formatting and lint checks is to enable this hook as pre-commit.
That in my experience saves far more time than it takes to run it.
We should probably document it in https://github.com/oracle/truffleruby/blob/master/doc/contributor/workflow.md

tomstuart · 2020-12-11T18:05:33Z

Thanks, that worked. I was trying to eliminate the diff output by CI but that obviously wasn’t right because it kept changing its mind.

chrisseaton · 2020-12-11T22:19:36Z

Do you intend the implement the rest of pattern matching in this PR, or would you rather merge this part, and the rest later?

I think we'd like to merge this. It's the result of a team hack-week. We'll come back and do more pattern matching work later - probably starting with doing the parser properly.

eregon · 2020-12-14T11:35:37Z

OK, please add an expert experimental option for pattern matching then, as described in #2186 (review)

chrisseaton · 2020-12-15T19:01:49Z

Option done

chrisseaton · 2020-12-15T19:03:11Z

Rebased to merge parser changes.

chrisseaton · 2020-12-16T09:29:24Z

We added

% ruby --experimental-options --pattern-matching spec/ruby/language/pattern_matching_spec.rb --excl-tag fails

to our test matrix.

eregon · 2020-12-16T11:19:21Z

I think we can automatically set the option when running specs, e.g., in jt's test_specs, so we just run them alongside other specs.
The command above won't test anything because of the fails tags.

Could you remove the last commit, and I'll find a way to run the specs for them automatically?

tomstuart · 2020-12-16T11:22:50Z

Could you remove the last commit, and I'll find a way to run the specs for them automatically?

Done. Thank you — it’s better for us if these specs run in this repo’s CI, cos then they won’t get accidentally broken.

We don’t intend to implement Ruby 3.0’s `#deconstruct` cache yet, but we’re including the example here for future reference.

This is how MRI behaves as of Ruby 2.7.2 & 3.0.0-preview2, so it’s important that we continue to do the same when implementing the rest of pattern matching.

… they are Co-authored-by: Lillian Zhang <[email protected]> Co-authored-by: Maple Ong <[email protected]>

Co-authored-by: Lillian Zhang <[email protected]> Co-authored-by: Maple Ong <[email protected]>

We can't untag any existing tests because the simplest binding variable spec is in an array, but this now works: ``` >> a = "FIRSTINSTANCE" => "FIRSTINSTANCE" >> case 1 in a p "s" in 1 p "o" end "s" => "s" >> a => 1 ```

This will make it easier to combine the two switch statements.

This isn’t always needed, so we shouldn’t do it unconditionally.

We’re switching twice on the node type, so we should just do it once instead.

We can't untag any existing tests because we currently depend upon the local variable having already been declared, but this now works: ``` >> a = "FIRSTINSTANCE" => "FIRSTINSTANCE" >> case [[1,1,1],[6,6,6]] in [[a,1,1],[Integer,Integer,Integer,]] >> end => nil >> a => 1 ```

This branch didn’t introduce these violations, but they’re preventing its build from going green.

By adding it to `src/options.yml`, then running `ruby tool/generate-options.rb` to regenerate `LanguageOptions.java` and `OptionsCatalog.java`.

The message is “syntax error, unexpected keyword_in” because that’s what TruffleRuby (without pattern matching support) currently raises. # Conflicts: # src/main/java/org/truffleruby/parser/BodyTranslator.java

norswap · 2020-12-16T14:57:29Z

src/main/java/org/truffleruby/parser/parser/ParserSupport.java

+     *
+     * @param expression of the case node (e.g. case foo)
+     * @param firstInNode first in (which could also be the else)
+     * @return a new case node */


For what it's worth, I don't think this is ugly at all. It keeps the grammar clean and all the logic in a single place.

The alternative would be to use the pattern used with ArrayParser in the grammar with left-recursion, so something roughly like this:

p_case_body : keyword_in args then compstmt { $$ = new SomeKindOfCaseBodyThing(); // or $$ = support.storedCaseInNode; $$.add(support.newInNode($1, $2, $4)); } | p_case_body keyword_in args then compstmt { $$ = $1.add(support.newInNode($2, $3, $5)); } | p_case_body keyword_else compstmt { $$ = $1.set_else($3); }

Is this problematic with the grammar?

Note that this comment was copied from newCaseNode(), which was @enebo’s work in d7361b8.

Okay! Probably worth changing that later if we decide that's not how we feel about it.

…2186) PullRequest: truffleruby/2263

graalvmbot added the oca-signed label Dec 11, 2020

tomstuart force-pushed the partial-pattern-matching branch 2 times, most recently from 104c107 to bcd1b0b Compare December 11, 2020 16:54

eregon reviewed Dec 11, 2020

View reviewed changes

tomstuart force-pushed the partial-pattern-matching branch 2 times, most recently from 7595b77 to 05d84e6 Compare December 11, 2020 17:31

tomstuart force-pushed the partial-pattern-matching branch from 05d84e6 to 27971da Compare December 11, 2020 17:54

tomstuart force-pushed the partial-pattern-matching branch 5 times, most recently from 585eb17 to cf6c011 Compare December 15, 2020 14:44

chrisseaton force-pushed the partial-pattern-matching branch from 53d9647 to 6989305 Compare December 15, 2020 19:03

tomstuart force-pushed the partial-pattern-matching branch from 6989305 to c6dc29d Compare December 16, 2020 11:21

chrisseaton and others added 5 commits December 16, 2020 11:23

Super-basic support for parsing case-in

73f5cbb

Add Array#deconstruct

eda877e

Implement Struct#deconstruct

da1b8f4

Add implementation for Hash#deconstruct_keys

5256f4f

Implement Struct#deconstruct_keys

397b2d1

chrisseaton and others added 21 commits December 16, 2020 11:23

Specify when deconstruct and deconstruct_keys is called

2b1ceb8

We don’t intend to implement Ruby 3.0’s `#deconstruct` cache yet, but we’re including the example here for future reference.

Specify case expression caching behaviour

3c1a82d

This is how MRI behaves as of Ruby 2.7.2 & 3.0.0-preview2, so it’s important that we continue to do the same when implementing the rest of pattern matching.

Rename expressionNode and rubyExpression to make it more obvious what…

3870050

… they are Co-authored-by: Lillian Zhang <[email protected]> Co-authored-by: Maple Ong <[email protected]>

Call #deconstruct on the case expression when the pattern is an array

5e1d3f2

Co-authored-by: Lillian Zhang <[email protected]> Co-authored-by: Maple Ong <[email protected]>

Match elements of array pattern against elements of case expression

434618d

Co-authored-by: Lillian Zhang <[email protected]> Co-authored-by: Maple Ong <[email protected]>

Use #deconstruct_keys and match hash pattern with case expression

49e3594

Support binding a single variable in a pattern match

76e0962

We can't untag any existing tests because the simplest binding variable spec is in an array, but this now works: ``` >> a = "FIRSTINSTANCE" => "FIRSTINSTANCE" >> case 1 in a p "s" in 1 p "o" end "s" => "s" >> a => 1 ```

Extract pattern matching helper method

4cef9ae

Inline local variables in helper method to avoid name clashes

71d106c

Combine local variables for call parameters in helper method

2729080

This will make it easier to combine the two switch statements.

Convert patternNode into a RubyNode inline

96e02ad

This isn’t always needed, so we shouldn’t do it unconditionally.

Combine similar switch statements in helper method

a7b267c

We’re switching twice on the node type, so we should just do it once instead.

Update changelog

6a69150

Fix old linting errors

311eee3

This branch didn’t introduce these violations, but they’re preventing its build from going green.

Rename deconstruct spec

51a64e3

NodeType.INNODE

3db440d

Struct#deconstruct_keys

72edeba

Comment on in 1, 2 parsing bug

b41a3b1

Introduce expert experimental --pattern-matching option

1314c85

By adding it to `src/options.yml`, then running `ruby tool/generate-options.rb` to regenerate `LanguageOptions.java` and `OptionsCatalog.java`.

Raise SyntaxError on pattern match if the feature isn’t enabled

4ca026c

The message is “syntax error, unexpected keyword_in” because that’s what TruffleRuby (without pattern matching support) currently raises. # Conflicts: # src/main/java/org/truffleruby/parser/BodyTranslator.java

eregon approved these changes Dec 16, 2020

View reviewed changes

tomstuart force-pushed the partial-pattern-matching branch from c6dc29d to 4ca026c Compare December 16, 2020 11:24

eregon self-assigned this Dec 16, 2020

eregon added the in-ci The PR is being tested in CI. Do not push new commits. label Dec 16, 2020

norswap reviewed Dec 16, 2020

View reviewed changes

graalvmbot pushed a commit that referenced this pull request Dec 18, 2020

[GR-19220] Implement partial support for Ruby 2.7 pattern matching (#…

eaeafce

…2186) PullRequest: truffleruby/2263

graalvmbot merged commit 4ca026c into oracle:master Dec 18, 2020

tomstuart deleted the partial-pattern-matching branch January 5, 2021 13:16

chrisseaton added the shopify label Feb 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement partial support for Ruby 2.7 pattern matching #2186

Implement partial support for Ruby 2.7 pattern matching #2186

tomstuart commented Dec 11, 2020 •

edited

Loading

eregon left a comment •

edited

Loading

tomstuart commented Dec 11, 2020

eregon commented Dec 11, 2020

eregon commented Dec 11, 2020

tomstuart commented Dec 11, 2020

chrisseaton commented Dec 11, 2020

eregon commented Dec 14, 2020

chrisseaton commented Dec 15, 2020

chrisseaton commented Dec 15, 2020

chrisseaton commented Dec 16, 2020

eregon commented Dec 16, 2020

tomstuart commented Dec 16, 2020

norswap Dec 16, 2020

tomstuart Dec 16, 2020

norswap Dec 16, 2020

Implement partial support for Ruby 2.7 pattern matching #2186

Implement partial support for Ruby 2.7 pattern matching #2186

Conversation

tomstuart commented Dec 11, 2020 • edited Loading

eregon left a comment • edited Loading

Choose a reason for hiding this comment

tomstuart commented Dec 11, 2020

eregon commented Dec 11, 2020

eregon commented Dec 11, 2020

tomstuart commented Dec 11, 2020

chrisseaton commented Dec 11, 2020

eregon commented Dec 14, 2020

chrisseaton commented Dec 15, 2020

chrisseaton commented Dec 15, 2020

chrisseaton commented Dec 16, 2020

eregon commented Dec 16, 2020

tomstuart commented Dec 16, 2020

norswap Dec 16, 2020

Choose a reason for hiding this comment

tomstuart Dec 16, 2020

Choose a reason for hiding this comment

norswap Dec 16, 2020

Choose a reason for hiding this comment

tomstuart commented Dec 11, 2020 •

edited

Loading

eregon left a comment •

edited

Loading