Skip to content
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

Fix a variable capture during pattern desugaring #5562

Merged
merged 3 commits into from
Jan 31, 2025
Merged

Conversation

dolio
Copy link
Contributor

@dolio dolio commented Jan 30, 2025

This fixes a variable capture issue that @stew found. I think it would only occur when you have code that is internally a case inside the scrutinee of a case.

I didn't try to find the exact spot that the variable capture occurs, and add calls to freshen variables. The capture was occurring on variables that are introduced by the pattern compiler. Before each stage of compilation would make up new variables like this starting from 0. Instead, I threaded the 'fresh number' through the whole process, so that variables made up are unique to the whole definition, and there's no need to worry about capture and re-freshening. This was pretty easy because it just involved tweaking how the visit function is used to rewrite the expression.

I added Stew's example as a transcript test case as well.

dolio added 2 commits January 30, 2025 15:12
Pattern compilation was causing variable captures in some
particular corner cases. Rather than try to figure out exactly what
situation was causing the capture, and how to avoid it, I've just
changed the way the recursive splitting functions works to thread
the fresh ids through the entire process. This means that
sub-expressions never make up the same variables as parent
expressions, and should mean that no further processing is needed
to avoid captures.
@dolio dolio requested review from pchiusano and stew January 30, 2025 22:37
Copy link
Member

@pchiusano pchiusano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

Looks like you just have to refresh transcript output.

@dolio
Copy link
Contributor Author

dolio commented Jan 30, 2025

Yeah. I think Vim deleted the trailing whitespace that the transcript runner put in the file.

@dolio
Copy link
Contributor Author

dolio commented Jan 30, 2025

Oh yeah. api-list-projects-branches seems to have random output order, so it only sometimes passes. One time I saw the output in a different order locally, too, so it's not just windows. Not sure who should look at that.

Or, what I mean is, it only sometimes passes CI. It runs successfully, but sometimes with different output than expected.

@pchiusano pchiusano merged commit e9d2a2a into trunk Jan 31, 2025
32 checks passed
@pchiusano pchiusano deleted the fix/pattern-capture branch January 31, 2025 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants