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

Raise blame error when trying to access fields in the sealed tail of a polymorphic record #1296

Merged
merged 4 commits into from May 5, 2023

Conversation

matthew-healy
Copy link
Contributor

This updates SealedTail to store a HashSet of the identifiers which the tail contains. Then, when we hit a point where we'd otherwise raise a FieldMissing error, we check whether the field is in the sealed tail. If so, we instead raise a blame error.

This closes #952.

@matthew-healy matthew-healy self-assigned this May 5, 2023
@github-actions github-actions bot temporarily deployed to pull request May 5, 2023 12:25 Inactive
@matthew-healy matthew-healy marked this pull request as ready for review May 5, 2023 13:02
Copy link
Member

@vkleen vkleen left a comment

Choose a reason for hiding this comment

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

👍 I was wondering whether it might be better to not duplicate the information which fields are contained in the tail. But I think, your solution is better; finding the contained fields from a RichTerm would be quite annoying. Even though the sealing code path might be rather hot if a statically typed function using polymorphic tails is used, constructing the HashSet hopefully isn't a big deal.

@matthew-healy
Copy link
Contributor Author

@vkleen Yeah, iirc when I discussed this with @yannham last year we decided this was likely to be a reasonable compromise initially.

Though, to be honest, I was happier with this solution before it involved allocating a new String for every Ident in the tail. I can't really see any way to avoid doing that and still get O(1) containment checking, but maybe taking linear time to check if an element is in the tail would be okay since we only hit that codepath when we already know we're going to raise some sort of error... 🤔

If so, then we could just store a Vec<Ident>, which avoids the extra string allocations.

@vkleen
Copy link
Member

vkleen commented May 5, 2023

Storing a Vec<Ident> actually sounds pretty reasonable. Generally, we don't care about the performance of the error path (within limits). Being able to avoid the String allocation should be worth that cost, I think.

@matthew-healy
Copy link
Contributor Author

Alright, sounds good. Thanks!

@github-actions github-actions bot temporarily deployed to pull request May 5, 2023 14:02 Inactive
@matthew-healy matthew-healy added this pull request to the merge queue May 5, 2023
Merged via the queue into master with commit 119dc4f May 5, 2023
5 checks passed
@matthew-healy matthew-healy deleted the feature/sealed-tail-access-blame-error branch May 5, 2023 15:16
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.

Correctly assign blame when accessing a field in the tail of a polymorphically-sealed record
2 participants