Skip to content

Add free_variables_list and refactor free_vars* #1519

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

Closed
wants to merge 5 commits into from

Conversation

rtjoa
Copy link
Contributor

@rtjoa rtjoa commented Jun 21, 2023

(Both #1518 and #1519 attempt to fix the same problem. See the last paragraph for how the behavior of this refactor differs)

The motivation for this change is similar to #1503's: we sometimes collect all free variables in a list of types tyl by passing a fake TTuple to Ctype.free_variables, as in Ctype.free_variables (Btype.newgenty (Ttuple tyl)).

However, this is fragile to changes in tuples, and in particular, becomes messy when adding labeled tuples: the above would become Ctype.free_variables (Btype.newgenty (Ttuple (List.map (fun t -> None, t) tyl))).

This refactor also allows Ctype.closed_type to return earlier, raising Non_closed for the first free variable found rather than the last (this should be the only externally-noticeable change in behavior - if we want, we could just re-collect the entire list and raise the last instead).

@ccasin
Copy link
Contributor

ccasin commented Jun 22, 2023

I discussed this with @rtjoa. Although the new version is cleaner, merging it would create maintenance burden when we rebase on upstream, so I'm going to close it for now. We may make an upstream PR that combines a few of his small improvements instead.

@ccasin ccasin closed this Jun 22, 2023
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