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

CaseOfCase kind mismatch error fix #5923

Merged
merged 3 commits into from
Apr 26, 2024

Conversation

Unisay
Copy link
Contributor

@Unisay Unisay commented Apr 24, 2024

Closes #5922 by fixing order of type arguments as they are extracted.

@Unisay Unisay added the No Changelog Required Add this to skip the Changelog Check label Apr 24, 2024
@Unisay Unisay requested a review from effectfully April 24, 2024 18:43
@Unisay Unisay self-assigned this Apr 24, 2024
Copy link
Contributor

@effectfully effectfully left a comment

Choose a reason for hiding this comment

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

That's not nearly enough:

  • most importantly, this function is in some general module, how can you be sure you're not breaking something else by reversing the order of the result? There's no comment on what that function is supposed to return, no examples etc, maybe that's how it's supposed to work for some other use case that isn't case-of-case. I don't see any way around going through all transitive dependencies and ensuring you aren't breaking things elsewhere
  • please document the offending function, so that it's clear what it returns to reduce the possibility of us screwing things up the same way again
  • please add the test that caught the bug as a unit/golden test to make sure we never regress back

@Unisay
Copy link
Contributor Author

Unisay commented Apr 25, 2024

most importantly, this function is in some general module, how can you be sure you're not breaking something else by reversing the order of the result?
I don't see any way around going through all transitive dependencies and ensuring you aren't breaking things elsewhere

Besides CaseOfCase transform I found only one place where this function extractTyArgs is used - in the PlutusIR.Analysis.Builtins.defaultUniMatcherLike and that one is only used in the CaseOfCase.

The tests for CaseOfCase transformation are all green so this gives me the confidence.

There's no comment on what that function is supposed to return, no examples etc, maybe that's how it's supposed to work for some other use case that isn't case-of-case.

I've added comment and test cases to explain the semantics as I understood them. I'd be grateful to @michaelpj for checking those test cases making sure that I inferred the intended behavior correctly.

please document the offending function, so that it's clear what it returns to reduce the possibility of us screwing things up the same way again

Done.

please add the test that caught the bug as a unit/golden test to make sure we never regress back

It was the property test that caught the bug, and its already there. I think that unit tests for the extractTyApp are enough to make sure we never have this bug again.

@Unisay Unisay requested a review from effectfully April 25, 2024 13:09
@michaelpj
Copy link
Contributor

So the analysis is that this would only happen when we had case-of-case on a type that had two type parameters, is that right? That does indeed seem like something we might not have tripped up on until now. Hooray for the generators!

Copy link
Contributor

@effectfully effectfully left a comment

Choose a reason for hiding this comment

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

As we discussed on the call I'd prefer to record the offender to increase coverage, but I'm not gonna be an ass about it, so here's your approval.

@Unisay
Copy link
Contributor Author

Unisay commented Apr 26, 2024

As we discussed on the call I'd prefer to record the offender to increase coverage, but I'm not gonna be an ass about it, so here's your approval.

I have recorded the offender as a golden test ;-)

@Unisay Unisay enabled auto-merge (squash) April 26, 2024 06:38
@Unisay Unisay merged commit 0567ef7 into master Apr 26, 2024
12 of 13 checks passed
@Unisay Unisay deleted the yura/PLT-9407/caseOfCase-kind-mismatch branch April 26, 2024 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No Changelog Required Add this to skip the Changelog Check
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CaseOfCase kind mismatch error
3 participants