Fix a parser crash with empty match vars #8911
Conversation
b51668f to
063c087
Compare
063c087 to
740819c
Compare
740819c to
fb49bd2
Compare
| @@ -1206,6 +1206,10 @@ class Builder::Impl { | |||
| // Label key is a symbol `sym: val` | |||
| return match_var_h 8000 ash(label->loc, key->val.show(gs_)); | |||
| } else if (auto *key = parser::cast_node<DSymbol>(pair->key.get())) { | |||
| if (key->nodes.empty()) { | |||
There was a problem hiding this comment.
For what it's worth, it looks like all string-quoted keys are not allowed in the punned hash key/val pairs
❯ ruby -e '{"x":}'
-e:1: syntax error, unexpected '}'
{"x":}not just empty strings.
| @@ -78,6 +78,7 @@ tuple<string, string> MESSAGES[] = { | |||
| {"NoAnonymousBlockArg", "no anonymous block parameter"}, | |||
| {"NoAnonymousRestArg", "no anonymous rest parameter"}, | |||
| {"NoAnonymousKwrestArg", "no anonymous keyword rest parameter"}, | |||
| {"InvalidKey", "key must be valid as local variables"}, | |||
There was a problem hiding this comment.
I don't know how important this is but both the whitequark parser and the ruby parser seem to report an unexpected token error:
❯ ruby-parse -e '{"x":}'
warning: parser/current is loading parser/ruby31, which recognizes 3.1.4-compliant syntax, but you are running 3.1.2.
Please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
(fragment:0):1:6: error: unexpected token tRCURLY
(fragment:0):1: {"x":}
(fragment:0):1: ^
~/sorbet (jez-package-namespaces-with-owner)
❯ ruby -e '{"x":}'
-e:1: syntax error, unexpected '}'
{"x":}
all else being equal it's probably better to have our own custom error for this if it means that we can provide better parse error recovery for it, so I'm fine with the new dclass but I'm curious to know what's different about our parser vs those parsers that is going through a different code path.
There was a problem hiding this comment.
Sorry it seems like this is actually specific to pattern matching, and the error message is the same as upstream.
~/sorbet (jez-package-namespaces-with-owner)
❯ ruby -e 'case {}; in {"":} then true; end'
-e:1: key must be valid as local variables
case {}; in {"":} then true; end
-e:1: identifier is not valid to set
~/sorbet (jez-package-namespaces-with-owner)
❯ ruby -e 'case {}; in {"x":} then true; end'
-e:1:in `<main>': {}: key not found: :x (NoMatchingPatternKeyError)
~/sorbet (jez-package-namespaces-with-owner)
❯ ruby -e 'case {}; in {"x y":} then true; end'
-e:1: key must be valid as local variables
case {}; in {"x y":} then true; end
-e:1: identifier x y is not valid to set
but in any case, it's not just empty string that's invalid
There was a problem hiding this comment.
Ah, true. I don't think that we can handle the second case, as it's a dynamic property of the value being examined, but the third case is definitely something we could raise an error for.
There was a problem hiding this comment.
yep, the second case I was just trying to show that the logic is not "are there quotes around the name" which I suspected might be the case, like maybe {x:} was fine but {"x":} was not fine because it had quotes (which would be easier to detect in Builder.cc than trying to check valid variable names)
There was a problem hiding this comment.
Is the plan to merge this and handle the non-empty "key must be valid as local variable" in a follow up change / an open issue?
There was a problem hiding this comment.
I'll timebox investigating the local variable name validation, and if I run out I'll ping you for an approval and make an issue to track it.
There was a problem hiding this comment.
I don't know whether there's an easy helper for this. Might be best to just over-eagerly allow things—I don't think that having {"x y":} causes problems for Sorbet.
Might want to add a test case and defer solving in Prism.
There was a problem hiding this comment.
My timebox ran out, and I agree that this will be easier to handle when we upgrade to prism 👍
Sorbet is crashing when encountering an unterminated string symbol in a match expression. This PR fixes the problem by catching the empty case, and entering it manually.
This introduces a new parser error, with text that mirrors the error from the ruby parser:
and from Sorbet after this change:
Motivation
Fixes #8903
Test plan
See included automated tests.