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

Handle contextual functions in PushTopAmbiguityUp #3925

Merged
merged 2 commits into from
Jan 26, 2024

Conversation

Scott-Guest
Copy link
Contributor

@Scott-Guest Scott-Guest commented Jan 25, 2024

Part of #3880

PushTopAmbiguityUp ensures that the LHS of any top-level rewrite is not an Ambiguity by pushing any such Ambiguity above the containing #RuleContent. This simplifies sort inference by making the declared sort of the LHS readily available, as needed for function, macro, etc. rules.

This PR fixes some issues in PushTopAmbiguityUp, namely

  • Correctly handle contextual functions. This was partly responsible for [K-Bug] Incorrect parsing and crash with ambiguous contextual function #3880, but is not the only issue there.
  • Ensure that we don't introduce nested ambiguities. I couldn't come up with a contrived enough test case to prove this is necessary, but conceptually this would be an issue if the #RuleBody is itself an Ambiguity containing a top-level rewrite whose LHS is an Ambiguity.
  • General code clean-up - documentation, better naming, factoring out some duplication

@Scott-Guest Scott-Guest self-assigned this Jan 25, 2024
@Scott-Guest Scott-Guest marked this pull request as ready for review January 25, 2024 03:38
Copy link
Contributor

@Baltoli Baltoli left a comment

Choose a reason for hiding this comment

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

Looks good!

@rv-jenkins rv-jenkins merged commit 08134dd into develop Jan 26, 2024
8 checks passed
@rv-jenkins rv-jenkins deleted the push-ambiguities-upward branch January 26, 2024 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants