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

[UPLC] [Test] Add scoping tests #6773

Merged

Conversation

effectfully
Copy link
Contributor

@effectfully effectfully commented Jan 6, 2025

This adds scoping tests for UPLC the same way we have them for PIR. It also tweaks the way the scoping machinery handles case-expressions for PIR.

I didn't stumble upon any unexpected behavior.

@effectfully effectfully self-assigned this Jan 6, 2025
@effectfully effectfully added the No Changelog Required Add this to skip the Changelog Check label Jan 6, 2025
@effectfully effectfully force-pushed the effectfully/untyped-plutus-core/test/add-scoping branch from 31e82d6 to 95bbea6 Compare January 6, 2025 10:00
@effectfully effectfully force-pushed the effectfully/untyped-plutus-core/test/add-scoping branch from 95bbea6 to cf7c231 Compare January 6, 2025 10:42
@effectfully effectfully requested review from Unisay and zliu41 January 6, 2025 11:12
Copy link
Member

@zliu41 zliu41 left a comment

Choose a reason for hiding this comment

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

This looks good but we have a Pass data type (though currently only for PIR) where a Pass is associated with pre- and post-conditions. Would it be better to express the scoping properties in Pass instead of using this more ad-hoc way of testing?

@effectfully
Copy link
Contributor Author

This looks good but we have a Pass data type (though currently only for PIR) where a Pass is associated with pre- and post-conditions. Would it be better to express the scoping properties in Pass instead of using this more ad-hoc way of testing?

The scoping tests are already rather complex, I'd rather not add any more complexity unless it's needed for something. Currently we only distinguish between

  • transformations that require renaming and those that don't
  • transformations that remove bindings and those that don't

I don't feel like adding an abstraction on top of this will make anything simpler or more readable. Particularly given that the handling logic is deep within the scoping tests machinery, so abstracting it out of there won't be nice.

@effectfully effectfully enabled auto-merge (squash) January 20, 2025 20:47
@effectfully effectfully disabled auto-merge January 20, 2025 20:47
@effectfully effectfully force-pushed the effectfully/untyped-plutus-core/test/add-scoping branch from cf7c231 to e500ffd Compare January 20, 2025 22:51
@effectfully effectfully enabled auto-merge (squash) January 20, 2025 23:13
@effectfully effectfully force-pushed the effectfully/untyped-plutus-core/test/add-scoping branch from e500ffd to ffc06c9 Compare January 22, 2025 14:26
@effectfully effectfully merged commit 5458fe5 into master Jan 22, 2025
8 of 10 checks passed
@effectfully effectfully deleted the effectfully/untyped-plutus-core/test/add-scoping branch January 22, 2025 15:24
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.

2 participants