Restricted pattern kinds allowed for a variable definition #1355
Conversation
| choice Ints { | ||
| None, | ||
| One(i32), | ||
| Two(i32, i32) | ||
| } | ||
|
|
||
| fn Main() -> i32 { | ||
| let (var Ints.Two(a1: auto, var a2: auto), | ||
| ((b: auto, var c: auto), var (d: auto, e: auto))) = | ||
| (Ints.Two(1, 10), ((2, 3), (4, 5))); | ||
| a1 = 0; | ||
| a2 = 0; |
There was a problem hiding this comment.
This test looks at least mostly correct to me (in particular, I don't think let (var Ints.Two(a1: auto, var a2: auto)) is right to just stop testing). Given it's being removed, @geoffromer can you give a look at this?
There was a problem hiding this comment.
I don't think this code is correct, because patterns like Ints.Two(a1: auto, var a2: auto) are refutable, but the pattern in a variable declaration needs to be irrefutable. So I think this code needs to be changed, but I can't speak to whether this change preserves the intent of the test (this is @DarshalShetty's code, not mine).
| @@ -11,7 +11,7 @@ | |||
| package ExplorerTest api; | |||
|
|
|||
| fn Main() -> i32 { | |||
| // CHECK: COMPILATION ERROR: {{.*}}/explorer/testdata/basic_syntax/fail_nested_binding.carbon:[[@LINE+1]]: The type of a binding pattern cannot contain bindings. | |||
| // CHECK: COMPILATION ERROR: {{.*}}/explorer/testdata/basic_syntax/fail_nested_binding.carbon:[[@LINE+1]]: binding pattern is not allowed on the right hand side of a variable definition | |||
There was a problem hiding this comment.
This error message confuses me; I would assume that "right hand side" and "left hand side" are relative to the =, but by that definition there is no binding pattern on the right hand side of this variable definition. This is an example of why I think we need separate error message logic for each rule that we're enforcing.
| @@ -183,6 +183,54 @@ static auto IsType(Nonnull<const Value*> value, bool concrete = false) -> bool { | |||
| } | |||
| } | |||
|
|
|||
| // Checks variable definition pattern structure. | |||
| static auto CheckVariableDefinitionPattern(const Pattern& p, const bool lhs) | |||
There was a problem hiding this comment.
This logic is merging several different checks, in a way that I don't think is helpful:
- The pattern in a variable definition must be irrefutable (meaning it's guaranteed to match any value of the appropriate type). This is what
var 1 = 2;andvar Ch.Alt(1) = Ch.Alt(2);violate, because expression and alternative patterns are never irrefutable. autocan only appear in the type part of a binding pattern.- The type part of a binding pattern cannot contain a binding pattern. Unlike the above two rules, I don't think this is an inherent rule of the Carbon language, it's just a limitation of Explorer.
Notice that only the first rule is specific to variable definitions; the other two apply to all patterns.
I'd suggest making this a function that applies to all patterns (not just patterns in variable definitions), with a parameter that indicates whether p is in a context that permits refutable patterns, and another that indicates whether p is in the type part of a binding pattern? Or, actually, it might be better to make those members of a PatternContext struct, because bool parameters tend to be very accident-prone, especially when there's more than one of them. I also suggest having separate error message logic for each of the three rules. Does that make sense?
| choice Ints { | ||
| None, | ||
| One(i32), | ||
| Two(i32, i32) | ||
| } | ||
|
|
||
| fn Main() -> i32 { | ||
| let (var Ints.Two(a1: auto, var a2: auto), | ||
| ((b: auto, var c: auto), var (d: auto, e: auto))) = | ||
| (Ints.Two(1, 10), ((2, 3), (4, 5))); | ||
| a1 = 0; | ||
| a2 = 0; |
There was a problem hiding this comment.
I don't think this code is correct, because patterns like Ints.Two(a1: auto, var a2: auto) are refutable, but the pattern in a variable declaration needs to be irrefutable. So I think this code needs to be changed, but I can't speak to whether this change preserves the intent of the test (this is @DarshalShetty's code, not mine).
|
We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active, please comment or remove the |
github-actions
bot
added
the
inactive
|
We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active or becomes active again, please reopen it. |
Fixes #1301