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

bugfix: make namespace directive not churn unique types #5509

Merged
merged 5 commits into from
Dec 18, 2024

Conversation

mitchellwrosen
Copy link
Member

Overview

Fixes #5489

This PR tweaks decl parsing to delay assigning a unique type guid until after the namespace pragma is applied. Previously (as seen in first transcript commit), one could accidentally be assigned a new unique type guid in the presence of a namespace directive.

Test coverage

I've added a transcript that demonstrates the fix.

@aryairani
Copy link
Contributor

@mitchellwrosen

💥 annotations.refs.Test annotations for types with effects.typechecked file

@mitchellwrosen mitchellwrosen marked this pull request as draft December 16, 2024 19:56
@@ -58,7 +58,7 @@ structural ability X where
``` ucm :added-by-ucm
Loading changes detected in scratch.u.

I found two types called X:
Copy link
Member Author

Choose a reason for hiding this comment

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

This change is due to the fact that our parser has two different error constructors that represent the duplicate type names. This PR performed only minimal unification (switching one over to the other), but there's a little bit more cleanup to do, namely delete the other one entirely.

@mitchellwrosen mitchellwrosen marked this pull request as ready for review December 17, 2024 21:58
@mitchellwrosen
Copy link
Member Author

@aryairani Out of expedience I decided to leave the UniqueName generation alone, since I realized we could reuse the namespace field in the parser state that was already there for a different purpose, but I can make a separate ticket for refactoring it to be the gen -> (Bytes, gen) that we talked about if you'd like.

@aryairani
Copy link
Contributor

@aryairani Out of expedience I decided to leave the UniqueName generation alone, since I realized we could reuse the namespace field in the parser state that was already there for a different purpose, but I can make a separate ticket for refactoring it to be the gen -> (Bytes, gen) that we talked about if you'd like.

I read over the code but I didn't get the new strategy. If you're convinced it will work, that's fine; if you want to discuss it let me know.

@mitchellwrosen
Copy link
Member Author

@aryairani Yeah I'm confident it will work.

When we query the parser environment for whether a unique type already exists for a particular name, we prefix the given name with the current namespace directive, if any.

It's really that simple, I didn't think of this solution right away though. :(

@aryairani aryairani merged commit 1d177b7 into trunk Dec 18, 2024
32 checks passed
@aryairani aryairani deleted the 24-12-09-fix-5489 branch December 18, 2024 16:33
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.

Namespace directive breaks idempotency check for unique types
2 participants