G/C unused Monoid and Semigroup instances in crucible-mir #1260
Conversation
|
Thanks for working on this, @sauclovian-g! This does change the API of |
|
Good point, hadn't thought about that. I'm trying that now. Looks like we'll need to bump what4 in saw, but that shouldn't hurt anything... |
|
ok, draft saw-script pull request here: GaloisInc/saw-script#2133 |
crucible-mir/CHANGELOG.md
Outdated
| * The calling sequence of ```translateMIR``` has changed: the first argument, | ||
| which should always have been passed as ```mempty```, has been removed. | ||
| This will probably require adjustments in downstream callers. |
There was a problem hiding this comment.
We already assigned a date (2024-08-30) to the 0.3 release, so let's put this bullet point under a new section (with the head # next -- TBA) above this one.
There was a problem hiding this comment.
Also, make sure to advertise the removal of the Semigroup and Monoid instances, since those are also part of the public API.
There was a problem hiding this comment.
I've moved the entry in the right place and added the instances (but there are not expected to be any downstream users -- are these part of the public API because you can't hide instances, or were they intended? (And is there any better way to determine what's in the public API than wade through the export lists of every source file? Is the Pass type in Mir.Pass, which has one fewer implicit argument now, also part of the public API because it's apparently exported and all the source files are exposed?)
There was a problem hiding this comment.
are these part of the public API because you can't hide instances, or were they intended?
Instances are always exported, so they're always part of the public API. More specifically...
And is there any better way to determine what's in the public API than wade through the export lists of every source file?
My measuring stick for what constitutes a publicly-facing API change is to use the Haskell Package Versioning Policy (PVP). According to the PVP, removing an instance is always a breaking change.
Is the
Passtype in Mir.Pass, which has one fewer implicit argument now, also part of the public API because it's apparently exported and all the source files are exposed?
While the PVP doesn't say anything about type synonyms per se, since we are changing the definition of Pass, I think one could argue that that is a breaking change per the PVP. We could mention this in the changelog to be on the safe side.
crucible-mir/CHANGELOG.md
Outdated
| * The calling sequence of ```translateMIR``` has changed: the first argument, | ||
| which should always have been passed as ```mempty```, has been removed. | ||
| This will probably require adjustments in downstream callers. |
There was a problem hiding this comment.
More than just "probably": it will require adjustment in downstream callers!
There was a problem hiding this comment.
Well, yes, I think I meant to write "downstream code" that might or might not actually call it (although I suppose it would be pretty odd if something used this and didn't call that...)
crucible-mir/src/Mir/Mir.hs
Outdated
| instance Semigroup Collection where | ||
| (Collection f1 a1 a1' t1 s1 v1 n1 tys1 r1) <> (Collection f2 a2 a2' t2 s2 v2 n2 tys2 r2) = | ||
| Collection (f1 <> f2) (a1 <> a2) (a1' <> a2') (t1 <> t2) (s1 <> s2) (v1 <> v2) (n1 <> n2) (tys1 <> tys2) (r1 <> r2) |
There was a problem hiding this comment.
Thinking about this some more, I think we should remove the Semigroup instances for Collection and friends as well. The reasoning is similar: when we add a schema version number to Collection, it's not clear how one would implement (<>) for two version numbers. (The only uses of (<>) in crucible-mir have already been removed in this PR.)
There was a problem hiding this comment.
You compose data blobs of different schema versions by promoting/converting the old one to the new version (or demoting the new one to the old version if it can be encoded in the old version, depending on requirements) and then you have a single version again.
But we aren't set up to do that and it's probably not worthwhile to try...
|
Do you think I should squash this one before merging it? |
|
I think squashing makes sense here, yes. |
|
Yeah, me too. |
That they're unused wasn't entirely obvious since there was a bunch of plumbing using them and you had to non-locally recognize that the only thing ever sent down that plumbing was mempty.
sauclovian-g
force-pushed
the
1259-remove-unused-monoids
branch
from
992a432 to
cb53598
Compare
sauclovian-g
changed the title
Closes #1259.