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

fix tc step types #1333

Merged
merged 1 commit into from
Feb 13, 2024
Merged

fix tc step types #1333

merged 1 commit into from
Feb 13, 2024

Conversation

rsoeldner
Copy link
Member

@rsoeldner rsoeldner commented Jan 18, 2024

This PR enhances the typechecking for defpacts by ensuring consistency in return types across all steps. Previously, varying return types were permissible for each step. This update mandates that the return type for all steps aligns with the defpact's defined result type.

PR checklist:

  • Test coverage for the proposed changes
  • PR description contains example output from repl interaction or a snippet from unit test output
  • Documentation has been updated if new natives or FV properties have been added. To generate new documentation, issue cabal run tests. If they pass locally, docs are generated.
  • Any changes that could be relevant to users have been recorded in the changelog
  • In case of changes to the Pact trace output (pact -t), make sure pact-lsp is in sync.

Additionally, please justify why you should or should not do the following:

  • Confirm replay/back compat
  • Benchmark regressions
  • (For Kadena engineers) Run integration-tests against a Chainweb built with this version of Pact

@rsoeldner rsoeldner marked this pull request as ready for review January 18, 2024 17:31
Copy link
Member

@emilypi emilypi left a comment

Choose a reason for hiding this comment

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

I'd like to see a regression that addresses nested defpacts in here.

src/Pact/Typechecker.hs Outdated Show resolved Hide resolved
@rsoeldner
Copy link
Member Author

I'd like to see a regression that addresses nested defpacts in here.

This PR concentrates solely on ensuring that each step's result type is consistent with the overall defpact type. While Stuart's nested defpact example remains a separate challenge, it is regarded as an independent issue and will be addressed in a future PR. This current update is dedicated to aligning step result types with the defpact's defined type.

@@ -60,7 +60,7 @@

(defpact test-pact-guards (id:string)
(step (step1 id))
(step (step2 (read-msg "id"))))
(step (let ((s2 (step2 (read-msg "id")))) "step2")))
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this binding do, and how is it extending the test? Is this suggesting that the return type of test-pact-guards should be :string?

Copy link
Member Author

Choose a reason for hiding this comment

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

step has return type string, with this change, we enforce that all steps have the same return type. Hence, the change will return the "step2" string. This is a fix to a pre-existing test (on caps).

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add some comments to the test describing what's going on here and what exactly is being tested by these odd constructions? (Namely, binding to a name that is never used). Everything else looks good!

@jmcardon jmcardon merged commit 45c8eca into master Feb 13, 2024
8 checks passed
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.

None yet

5 participants