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

Simplify the implementations of hasOrd and hasEq #3735

Merged
merged 1 commit into from Sep 22, 2023

Conversation

MaximilianAlgehed
Copy link
Collaborator

Description

This simplifies the implementation by relying on a generalization of the IsTypeable dict

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated
  • When applicable, versions are updated in .cabal and CHANGELOG.md files according to the
    versioning process.
  • The version bounds in .cabal files for all affected packages are updated. If you change the bounds in a cabal file, that package itself must have a version increase. (See RELEASING.md)
  • All visible changes are prepended to the latest section of a CHANGELOG.md for the affected packages. New section is never added with the code changes. (See RELEASING.md)
  • Code is formatted with fourmolu (use scripts/fourmolize.sh)
  • Cabal files are formatted (use scripts/cabal-format.sh)
  • hie.yaml has been updated (use scripts/gen-hie.sh)
  • Self-reviewed the diff

@MaximilianAlgehed
Copy link
Collaborator Author

@TimSheard another nice trick!

@MaximilianAlgehed
Copy link
Collaborator Author

@lehins there is a bug in fourmolu - it's suggested change does not have the same meaning as the original code and fourmolu knows this. However, the CI still fails.

@lehins
Copy link
Contributor

lehins commented Sep 19, 2023

there is a bug in fourmolu

@MaximilianAlgehed Can you report it as a bug?

It can either be a bug in ormolu or fourmolu, so you'd need to try applying a snippet using latest versions of both of those tools and report it on the appropriate repo, I suspect it will be ormolu, since that's the package responsible for getting the source AST and majority of the formatting decisions.

Should be as simple as downloading the ormolu binary from the repo release and applying it to the snippet that has the semantic broken by it.

@MaximilianAlgehed
Copy link
Collaborator Author

This is the specific error that I got on commit 897a6dd:

@@ -443,6 +443,6 @@
    MaybeR (repHasInstances -> ia) -> requireInstances ia
    GenR (repHasInstances -> IsTypeable) -> IsTypeable

- lubIs :: ((c a, c b) => c (f a b)) => Is c a -> Is c b -> Is c (f a b)
+ lubIs :: (c a, c b) => c (f a b) => Is c a -> Is c b -> Is c (f a b)
  lubIs Is Is = Is
  lubIs _ _ = Isn't
@@ -458,6 +458,6 @@
  lubInstances (Type eq_a ord_a) (Type eq_b ord_b) =
    Type (lubIs eq_a eq_b) (lubIs ord_a ord_b)

- requireIs :: (c a => c (f a)) => Is c a -> Is c (f a)
+ requireIs :: c a => c (f a) => Is c a -> Is c (f a)
  requireIs Is = Is
  requireIs _ = Isn't

  AST of input and AST of formatted code differ.
    at libs/cardano-ledger-test/src/Test/Cardano/Ledger/Constrained/TypeRep.hs:446:11-33
    at libs/cardano-ledger-test/src/Test/Cardano/Ledger/Constrained/TypeRep.hs:446:39-[7](https://github.com/input-output-hk/cardano-ledger/actions/runs/6236465538/job/16927825935#step:4:8)0
    at libs/cardano-ledger-test/src/Test/Cardano/Ledger/Constrained/TypeRep.hs:461:15-2[8](https://github.com/input-output-hk/cardano-ledger/actions/runs/6236465538/job/16927825935#step:4:9)
    at libs/cardano-ledger-test/src/Test/Cardano/Ledger/Constrained/TypeRep.hs:461:34-53
  Please, consider reporting the bug.
  To format anyway, use --unsafe.

@MaximilianAlgehed
Copy link
Collaborator Author

MaximilianAlgehed commented Sep 19, 2023

I checked the issue tracker over at the fourmolu repo and a very similar issue has been marked as resolved fourmolu/fourmolu#340. Before I go digging around with this - are we sure we are using the latest version of fourmolu in this repo or are we on a version from before this fix?

@lehins
Copy link
Contributor

lehins commented Sep 19, 2023

are we sure we are using the latest version of fourmolu

Good point. We are not. I'll submit a PR in a moment that updates it. So, you might be able to remove this commit d2daee7

@MaximilianAlgehed MaximilianAlgehed force-pushed the PR-simplify-ord-eq branch 2 times, most recently from 8017d04 to fd52383 Compare September 21, 2023 14:36
@lehins
Copy link
Contributor

lehins commented Sep 21, 2023

Good point. We are not. I'll submit a PR in a moment that updates it. So, you might be able to remove this commit d2daee7

@MaximilianAlgehed Just in case you didn't notice #3742 fixed the issue with outdated version of fourmolu

Copy link
Contributor

@lehins lehins left a comment

Choose a reason for hiding this comment

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

This is way too cool! I love it ❤️

@MaximilianAlgehed
Copy link
Collaborator Author

@lehins that updated version of fourmolu doesn't appear to have fixed the issue. Perhaps the fix hasn't been released yet? I can put back the work-around for now with a note to follow this up in the future.

@lehins
Copy link
Contributor

lehins commented Sep 21, 2023

I can put back the work-around for now with a note to follow this up in the future.

Before putting workarounds, I'd suggest verifying the problem is not fix and submitting an issue with a reproducer case on fourmolu or ourmolu repo, depending who's responsible for the problem

@MaximilianAlgehed
Copy link
Collaborator Author

@lehins fourmolu/fourmolu#374

@MaximilianAlgehed MaximilianAlgehed force-pushed the PR-simplify-ord-eq branch 2 times, most recently from 55c06bb to c7afed5 Compare September 22, 2023 12:08
@lehins
Copy link
Contributor

lehins commented Sep 22, 2023

@MaximilianAlgehed could you rebase this on master? We've had a PR merged.

FYI, now that you have write permissions to the repo, if you create branches on this repo, instead of a fork, I'd be able to do rebases for you right in the github UI, which would remove this inconvenient friction.

@MaximilianAlgehed
Copy link
Collaborator Author

Yes, I'll build my future PRs on this repos origin

@lehins lehins merged commit 3e5e7f4 into IntersectMBO:master Sep 22, 2023
12 checks passed
@MaximilianAlgehed MaximilianAlgehed deleted the PR-simplify-ord-eq branch September 25, 2023 09:38
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

2 participants