-
-
Notifications
You must be signed in to change notification settings - Fork 337
Implement Occurs Check #7811
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
Implement Occurs Check #7811
Conversation
2d6d773
to
c03d11e
Compare
86e6e50
to
ba1b414
Compare
/// _only_ modifies a variable's `Mark`. Before returning, all visited nodes' | ||
/// `Mark`s will be reset to `none`. | ||
/// | ||
/// TODO: See if there's a way to represent this ^ in the type system? If we |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe so! I think calling .items(.mark)
on the MultiArrayList
should get you a slice of just the marks, which would mean you could:
- continue to take
types_store: *Store
- pass a
*const Store
and then a mutable slice of just theMark
s to a helper function - now we know the helper only mutates the
Mark
s
} else if (self.scratch.hasSeenVar(root.var_)) { | ||
// Recursion point! We've already seen this var during traversal. | ||
if (ctx.recursion_allowed and ctx.seen_nominal) { | ||
// If recursion is allowed (we've passed through a Box, List or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Box, List
😍 😍 😍
} | ||
|
||
fn appendSeen(self: *Self, var_: Var) void { | ||
_ = self.seen.append(self.gpa, var_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In working on unify
I've actually personally been liking having things do std.mem.Allocation.Error!void
and actually use try
(instead of the exit on oom thing) - what do you think of doing more of that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm into that, imo the more explicit the points of failure are the better. And tracking the error in the type sigs would make it easier to understand what's allocating and what's not, at a glance
@@ -5,5 +5,8 @@ test { | |||
testing.refAllDeclsRecursive(@import("main.zig")); | |||
testing.refAllDeclsRecursive(@import("snapshot.zig")); | |||
testing.refAllDeclsRecursive(@import("builtins/main.zig")); | |||
|
|||
// TODO: Remove after hooking up |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be removed now? 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this module isn't yet used in any codepath, it'll be DCE'd. But i'll remove in a subsequent PR that hooks occurs
up into unify
!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jaredramirez this looks SWEET!!! 🤩
I left a few suggestions, but nothing blocking!
This change switches back from the "new" way to keep track of visited node in the occurs check to set Marks, like the rust compiler does.
ba1b414
to
12e3ec5
Compare
Overview
This PR implements the
occurs
check for type variables. This means, for a given type variable:redirect
s until we get a concrete type)This is the 1st step in the larger project of supporting recursive types in the type system, but only for nominal types – as outlined here. This check is not yet used anywhere, that will come in a follow-up PR.
Memory Management
During the occurs check, there are things we need to write down:
seen
so far (pushed & popped for each recursive branch)visited
and confirmed are not recursive (to avoid redundant work)To capture this, we use a
Scratch
struct that allocates some array lists that can be re-used across many occurs check. This is the same pattern thatunify
uses to hold intermediate data.Differences from the Rust Compiler
This implementation has not significant difference from the Rust compiler version. Originally, I did things slightly differently, but ultimately decided that it probably made the most sense to match the semantics of the Rust version. That said, if those reviewing like the original approach, we can easily revert.
Original "Differences from the Rust Compiler"
The Rust implementation of occurs takes a different approach to avoiding redundant work
Mark
field to track visited state during the checkThis requires:
Mark
values on each visited descriptorThe Zig version does not use
Mark
at all. Instead:This results in the same number of allocations, but with two key differences:
Mark
fields on descriptorsvisited
arrayseen
variables, though the total number ofseen
vars for any given recursive branch would be less than the total visited nodes.That said, I'm unsure what the average number of variables in a type is, but this approach assumes it would be relatively small. (We could also use a Map to check for seen/visited vars, which may perform better than an array)