-
Notifications
You must be signed in to change notification settings - Fork 89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Refactor] Pattern matching #1799
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yannham
force-pushed
the
refactor/pattern-matching
branch
from
February 6, 2024 17:01
7bc531d
to
f35b5a1
Compare
yannham
force-pushed
the
refactor/pattern-matching
branch
from
February 6, 2024 20:02
f35b5a1
to
70f6eb9
Compare
yannham
force-pushed
the
refactor/pattern-matching
branch
from
February 8, 2024 14:59
70f6eb9
to
0982a62
Compare
vkleen
approved these changes
Feb 9, 2024
This commit is a preliminary work for the upcoming ADTs (and more generally the introduction of pattern matching, instead of just having destructuring). The refactoring aims at installing a consistent naming, simplify the representation of patterns and their associated method, and pave the way for other patterns that just records. Indeed, the previous implementation often made the implicit assumptions that patterns were only record patterns.
Record pattern was previously using a tuple `(open: bool, rest: Option<LocIdent>)` to represent the presence of either no tail, an non-capturing tail `..`, or a capturing tail `..rest`. This representation allows the illegal state `(false, Some("x"))`: indeed, if the tail is capturing, the record contract is necessarily open. This commit flattens this representation to a single enum that correctly represents those three different cases, instead of the 4 allowed by the previous representation.
The AST of patterns had a special node for an aliased pattern, which was a variant containing the alias and a potentital nested pattern. However, this doesn't model correctly patterns: usually, it doesn't make sense to stack aliases (and the parser won't accept it), but the previous representation accepted ASTs for things like `x @ y @ z @ <pat>`, which incurs additional burden to handle, although it can actually never happen. Additionally, the alias of the top pattern was duplicated as an optional field in the `LetPattern` and `FunPattern` nodes of the `Term` AST. This commit makes things simpler by storing `alias` as an optional field directly in the `Pattern` struct, which makes it accessible without having to pattern match on an enum variant, and forbids nested aliases. Doing so, we remove the duplication from the `LetPattern` and `FunPattern`, which now only takes a pattern instead of an optional identifier and a pattern, leading to code simplification.
The refactoring of patterns has introduced a slightly different algorithm for typechecking patterns, which isn't entirely backward-compatible, although it's more consistent. We'll probably rule out (i.e. depreacte) the offending special cases, but until then, this commit restores the previous behavior, which fixes a previously failing test.
This commit only applies pure renaming of several symbols of the destructuring module for improved clarity. The whole module is also moved to `term::pattern`, as patterns are just syntactic component of the term AST.
Co-authored-by: Viktor Kleen <[email protected]>
yannham
force-pushed
the
refactor/pattern-matching
branch
from
February 9, 2024 14:25
b744381
to
e024740
Compare
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is preliminary work for the introduction of elimination form for ADTs. It is just a pure refactoring, which shouldn't incur any user-facing change.
This refactoring was motivated by the fact that the existing structure of the codebase used names that weren't very consistent, and relied heavily on the assumption that the only possible patterns are record patterns, which would make the addition of new patterns (ADTs, but not limited to them - constants patterns as well for e.g. strings, array patterns, regular expressions, etc.) impossible.
Content
destructuring
is now the submoduleterm::pattern
(and same for other destructuring submodules, which are now namedpattern
), and the strange distinction between matches and destructuring is ditched: everything is a pattern.do_stuff_record_pattern
,do_stuff_field_pattern
, etc.open = false, rest = Some("captured_var")
LetPattern
andFunPattern
to not take an additional top-level alias as an argument, and simply store this data inside the pattern itselfReview
Unfortunately, the git diff doesn't seem to understand that
destructuring.rs
was renamed toterm/pattern.rs
(and similarly fortypecheck::destructuring -> typecheck::pattern
,nickel_lang_lsp::destructuring -> nickel_lang_lsp::pattern
). That being said, the changes are quite heavy, so maybe it's not a bad idea to just review as new code (but take a look at the deleted file to have an idea of what was the previous state).