[syntax-errors] Multiple assignments in case pattern#16957
Conversation
Summary -- This PR detects multiple assignments to the same name in `case` patterns by recursively visiting each pattern. Test Plan -- New inline tests.
|
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
This looks good to me. I've a few smaller suggestions
| let range = if ident.range.start() > other.range.start() { | ||
| ident.range | ||
| } else { | ||
| other.range | ||
| }; |
There was a problem hiding this comment.
It seems that we only need the range comparison here because we visit the PatternMatchAs fields not in the source order. That took me a while to realise. I think I'd prefer to, instead, change visit_pattern so that it visits all fields in source order, so that we can remove the range handling here.
There was a problem hiding this comment.
Ah yes, I thought I had tried that first and got surprised when they didn't come out in source order, but I think I just had the order wrong in the MatchAs case.
| impl<'a, Ctx: SemanticSyntaxContext> MultipleCaseAssignmentVisitor<'a, Ctx> { | ||
| /// Check if `other` is present in `self.names` and emit an error if so. | ||
| fn push(&mut self, other: &'a ast::Identifier) { | ||
| for ident in &self.names { |
There was a problem hiding this comment.
This is O(n^2). Should we change the visitor to first collect all names, then sort by name (log (n)), then identify duplicate names by comparing neighboring elements (n)?
There was a problem hiding this comment.
That's closer to what I had initially, but I ran into some issues with MatchOr, where the patterns needed to be visited independently. I found a way to work around it and think this is better, thanks!
| 4 | case [y, z, *y]: ... # MatchSequence | ||
| 5 | case [y, y, y]: ... # MatchSequence multiple | ||
| 6 | case {1: x, 2: x}: ... # MatchMapping duplicate pattern | ||
| | ^ Syntax Error: multiple assignments in pattern |
There was a problem hiding this comment.
Do you think we should mention the variable name here? It wasn't clear to me from the error message what's wrong in this example: Can I only have a single variable? It might be especially hard to realize what's wrong if the use of the same identifier isn't next to each other ({ 1: x, 2: aaaa_very_long_key, ..., 4: x })
There was a problem hiding this comment.
That's a good point. I was trying to avoid putting data into the SemanticSyntaxErrorKind variants, but we can give better error messages if we do. I'll try something more like Python:
>>> match 2:
... case x as x: ...
...
File "<python-input-4>", line 2
case x as x: ...
^^^^^^
SyntaxError: multiple assignments to name 'x' in pattern
>>> match 2:
... case [x, y, *x]: ...
...
File "<python-input-5>", line 2
case [x, y, *x]: ...
^^
SyntaxError: multiple assignments to name 'x' in pattern
| } | ||
| } | ||
| Pattern::MatchOr(ast::PatternMatchOr { patterns, .. }) => { | ||
| // each of these patterns should be visited separately |
There was a problem hiding this comment.
I think it would be useful to provide the reason for why should this be done. From what I understand, I think it's because they're separate patterns and checking for duplicate names are isolated to a single pattern.
There was a problem hiding this comment.
I expanded the comment and also moved the relevant test_ok case down.
I also switched to the hash set implementation we discussed yesterday and cleaned up some of the docs and code locations.
|
@ntBre I am seeing an linting error thrown for Any idea what should be happening here? |
|
@sammorley-short Yeah that was a bug in this PR, first reported in #17181. It's been fixed but not released yet. 0.11.4 should come out with the fix today. Thanks for the report! |
4 participants
Summary
This PR detects multiple assignments to the same name in
casepatterns by recursively visiting each pattern.Test Plan
New inline tests.