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

Allow type synonyms inside other type synonyms #1986

Merged
merged 4 commits into from Dec 5, 2023
Merged

Allow type synonyms inside other type synonyms #1986

merged 4 commits into from Dec 5, 2023

Conversation

qsctr
Copy link
Contributor

@qsctr qsctr commented Nov 18, 2023

Instantiate the body of a type synonym declaration with the existing type synonym map before adding it to the map, so that the map never contains any uninstantiated type synonyms.

Fixes #1985.

Instantiate the body of a type synonym declaration with the existing
type synonym map before adding it to the map, so that the map never
contains any uninstantiated type synonyms.
@qsctr qsctr self-assigned this Nov 18, 2023
@qsctr qsctr marked this pull request as ready for review November 18, 2023 11:15
@@ -382,10 +387,13 @@ instance Instantiate Type where
TySkolemVar _ _ -> ty
LType pos ty' -> LType pos (instantiate nts ty')

instantiateType :: Instantiate t => Map Name Type -> t -> t
instantiateType tenv = instantiate (M.assocs tenv)
Copy link
Contributor

Choose a reason for hiding this comment

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

This does almost the exact same thing as the instantiate method of the Instantiate class, except that this function uses a Map Name Type instead of [(Name, Type)]. I think using a Map makes more sense here—any objection to simply changing the type signature of instantiate to use a Map instead?

Either way, it would be worth leaving a comment saying precisely what we are instantiating. (Particularly, we're instantiating typedefs.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I've changed instantiate to use Map. It seems like it's not only used with typedefs, so I wrote a more general description.

@qsctr qsctr added the ready-to-merge If at least one person approves this PR, automatically merge it when CI finishes successfully. label Dec 5, 2023
@mergify mergify bot merged commit 2adef40 into master Dec 5, 2023
40 checks passed
@mergify mergify bot deleted the T1985 branch December 5, 2023 18:54
RyanGlScott added a commit that referenced this pull request Feb 1, 2024
RyanGlScott added a commit that referenced this pull request Feb 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge If at least one person approves this PR, automatically merge it when CI finishes successfully.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transitive SAWScript type synonyms don't work
2 participants