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

Mark ClassesC as Trustworthy #151

Merged

Conversation

langston-barrett
Copy link
Contributor

@langston-barrett langston-barrett commented Feb 8, 2023

For compatibility with lens < 5. Fixes #152.

@RyanGlScott
Copy link
Contributor

GHC now warns:

[20 of 38] Compiling Data.Parameterized.ClassesC
src/Data/Parameterized/ClassesC.hs:20:14: warning: [-Wtrustworthy-safe]
Warning:     ‘Data.Parameterized.ClassesC’ is marked as Trustworthy but has been inferred as safe!
   |
20 | {-# LANGUAGE Trustworthy #-}
   |              ^^^^^^^^^^^

I think we should just globally set -Wno-trustworthy-safe in the .cabal file to avoid this class of warnings.

For compatibility with lens < 5. See GaloisInc#149.
@langston-barrett
Copy link
Contributor Author

Good catch, fixed!

@RyanGlScott
Copy link
Contributor

I think this is a sensible way to handle things. Does this work for you, @kquick?

@kquick
Copy link
Member

kquick commented Feb 8, 2023

Couldn't subterms someone something internally call unsafePerformIO and wouldn't this violate the intention of declaring this module Trustworthy?

@RyanGlScott
Copy link
Contributor

My understanding of the contract that Trustworthy offers (as documented here) is that Trustworthy code is allowed to do unsafe things internally, provided that the user-facing API still respects type safety and referential transparency. That is certainly the case with Data.Parameterized.ClassesC, and I'd wager the same applies to nearly all the code in parameterized-utils (with the possible exception of Data.Parameterized.Axiom).

@kquick
Copy link
Member

kquick commented Feb 9, 2023

OK. I have reservations about who bears the responsibility on Trustworthy, but I think that's outside the scope of what is reasonably involved here, so I'm good with this change.

@RyanGlScott
Copy link
Contributor

Are you alright with merging this, @langston-barrett?

@langston-barrett langston-barrett merged commit a581283 into GaloisInc:master Feb 16, 2023
@langston-barrett langston-barrett deleted the lb/some-trustworthy branch February 16, 2023 15:40
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.

Incorrect lower bound for lens
3 participants