New Singleton enum for PatternMatchSingleton node#8063
Conversation
|
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
dhruvmanila
force-pushed
the
dhruv/tuple-constant
branch
from
71c6d5a to
2d4925c
Compare
dhruvmanila
force-pushed
the
dhruv/match-pattern-singleton
branch
2 times, most recently
from
07dce4c to
db9d767
Compare
| #[inline] | ||
| fn visit_singleton(&mut self, _singleton: &'a Singleton) {} | ||
|
|
There was a problem hiding this comment.
I don't think this should be defined 🤔
PR Check ResultsEcosystem✅ ecosystem check detected no linter changes. ✅ ecosystem check detected no format changes. |
| } | ||
|
|
||
| #[derive(Debug, PartialEq, Eq, Hash)] | ||
| pub enum ComparableSingleton { |
There was a problem hiding this comment.
Nit: I find Singleton a confusing name. I assume it comes from Python's AST definition?
Or should this match the literal case? https://peps.python.org/pep-0622/#literal-patterns
There was a problem hiding this comment.
I assume it comes from Python's AST definition?
Yes, and in turn ours as both uses the "Singleton" suffix.
It is for the literal pattern but only a subset of it - None, True, False. The string and number literal are represented using PatternMatchValue while the singletons are using PatternMatchSingleton.
| { | ||
| let ast::PatternMatchSingleton { value, range: _ } = self; | ||
| visitor.visit_constant(value); | ||
| visitor.visit_singleton(value); |
There was a problem hiding this comment.
What about constant's in other positions? e.g. do we call this function when visiting a ExprConstant too?
There was a problem hiding this comment.
Yes, we do:
ruff/crates/ruff_python_ast/src/node.rs
Lines 2681 to 2687 in db9d767
I'm not sure if there should be a visit_singleton or not. Do we define visitor methods for the enum values as well? Wouldn't they be accounted for when visiting the enclosing node? Looking at Constant, we do.
There was a problem hiding this comment.
I'm not sure. I guess it can be useful when the same non-nodes may have different parent nodes but I would be okay only having visitors for nodes. @charliermarsh should have more context on why visit_constant exists.
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
I'm not a huge fan of the Singleton terminology because it only makes me thing about the singleton pattern but it is in line with Python's terminology. It also seem to make sense from a technical perspective (I didn't know that) because None, True, and False are indeed all singleton values 🤯
The one alternative I could think of is a KeywordLiteral.
dhruvmanila
force-pushed
the
dhruv/match-pattern-singleton
branch
from
db9d767 to
666eb09
Compare
Ah yes, I should've written that in the PR description.
This might be confusing as other keywords are not part of it. |
dhruvmanila
force-pushed
the
dhruv/match-pattern-singleton
branch
from
666eb09 to
9321710
Compare
2 participants
Summary
This PR adds a new
Singletonenum for thePatternMatchSingletonnode.Earlier the node was using the
Constantenum but the value for this pattern can only be eitherNone,TrueorFalse. With the coming PR to remove theConstant, this node required a new type to fill in.This also has the benefit of narrowing the type down to only the possible values for the node as evident by the removal of
unreachable.Test Plan
Update the AST snapshots and run
cargo test.