Opened 11 years ago

Last modified 2 years ago

#2496 new bug

Invalid Eq/Ord instances in Data.Version

Reported by: guest Owned by:
Priority: normal Milestone:
Component: Core Libraries Version: 6.8.3
Keywords: Cc: hvr, ekmett, core-libraries-committee@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D395
Wiki Page:

Description

(From Adrian Hey)

In Data.Version we have:

data Version = 
  Version {versionBranch :: [Int]
          ,versionTags :: [String]
          }

instance Eq Version where
  v1 == v2  =  versionBranch v1 == versionBranch v2 
                && sort (versionTags v1) == sort (versionTags v2)
                -- tags may be in any order

instance Ord Version where
  v1 `compare` v2 = versionBranch v1 `compare` versionBranch v2

The "laws" for valid Eq/Ord instances were argued about recently but the H98 report seems reasonably clear that (==) is supposed to test for equality and the compare method is supposed to define a total ordering. There is also an implied but not explicitly stated law that:

 (x == y = True)  <-> (x `compare` y = EQ)

and also this I guess..

 (x == y = False)  <-> ~(x `compare` y = EQ)

This law is implied by the Eq constraint on the Ord class (which seems to serve no purpose otherwise).

See also: http://www.haskell.org/pipermail/haskell-prime/2008-March/002330.html

Change History (30)

comment:1 Changed 11 years ago by duncan

See also the thread on cabal-devel starting with: http://haskell.org/pipermail/cabal-devel/2008-May/002818.html

The conclusion of the discussion was that we should either remove the tags completely, or at minimum change the Eq instance to consider only the versionBranch. I was going to put a proposal to removing them through the libraries submission process. I suppose I should get round to doing that.

comment:2 Changed 10 years ago by igloo

difficulty: Unknown
Milestone: _|_
Owner: set to duncan.coutts@…

comment:3 Changed 10 years ago by simonmar

Architecture: MultipleUnknown/Multiple

comment:4 Changed 10 years ago by simonmar

Operating System: MultipleUnknown/Multiple

comment:5 Changed 6 years ago by morabbin

Type of failure: None/Unknown

Data.Version in base-4.6.0.1 still has the quoted instance for Eq. Not clear if there's been a library proposal.

comment:6 Changed 6 years ago by igloo

Owner: changed from duncan.coutts@… to duncan

comment:8 Changed 5 years ago by duncan

Note that that is a different can of worms, trailing zeros.

My suggestion is to remove the unused (and useless) tag field, then the derived Eq and Ord will be fine.

comment:9 Changed 4 years ago by thomie

Cc: hvr ekmett added

comment:10 Changed 4 years ago by thoughtpolice

Component: libraries/baseCore Libraries

Moving over to new owning component 'Core Libraries'.

comment:11 Changed 4 years ago by thomie

Cc: core-libraries-committee@… added
Differential Rev(s): Phab:D395
Status: newpatch

Library proposal accepted.

comment:12 in reply to:  11 Changed 4 years ago by hvr

Replying to thomie:

Library proposal accepted.

Is there a summary of the library submission?

comment:13 Changed 4 years ago by ekmett

@hvr See the link from thomie above ~5 weeks ago.

Makes sense to me.

So is the plan then to deprecate the tags now and remove them later?

Last edited 4 years ago by ekmett (previous) (diff)

comment:14 in reply to:  13 Changed 4 years ago by hvr

Replying to ekmett:

@hvr See the link from thomie above ~5 weeks ago.

I saw no summary of the voting/discussing there; I meant a summary in the sense of

At the end of the discussion period, summarise your understanding of the consensus (or lack thereof), including a link to the thread in the mailing list archives, and send the summary to the maintainer for decision.

Here's an example of what I mean: http://www.haskell.org/pipermail/libraries/2014-August/023658.html

comment:15 Changed 4 years ago by thomie

From http://www.haskell.org/pipermail/libraries/2014-October/024033.html:

This proposal is now accepted, with support from Duncan, Herbert and myself. No objections were raised.

If all goes well, versionTags will be deprecated in GHC 7.10, and removed in GHC 7.12.

I'll try to write a bit nicer summary next time.

comment:16 Changed 4 years ago by Herbert Valerio Riedel <hvr@…>

In 137b33133f49a994e5d147c5b30a8fcfc610eada/ghc:

Deprecate Data.Version.versionTags (#2496)

The library submission was accepted:

  http://www.haskell.org/pipermail/libraries/2014-September/023777.html

The T5892ab testcases were changed to use `Data.Tree` instead of `Data.Version`

Reviewed By: ekmett

Differential Revision: https://phabricator.haskell.org/D395

comment:17 Changed 4 years ago by thoughtpolice

Milestone: 7.12.1
Owner: duncan deleted
Status: patchnew

Since the patch from Thomas was merged, I'm moving this to 7.12 so we can close it then.

comment:18 Changed 4 years ago by Herbert Valerio Riedel <hvr@…>

In 5b8fa46ca37caa9ec83b217a697628135da34506/ghc:

Add Data.Version.makeVersion & `IsList Version`

These two facilities provide some means to avoid the double-breakage caused by
first by the deprecation (see #2496), and then again by the actual future
field-removal.

See also

  https://groups.google.com/d/msg/haskell-core-libraries/q9H-QlL_gnE/4lbb_mBjre8J

for details about this library addition.

Reviewed By: ekmett

Differential Revision: https://phabricator.haskell.org/D577

comment:19 Changed 4 years ago by zilinc

I feel it's too late to object, so I'll ask a question instead. I'm developing a software, in which I use the VersionTags field to store the git commit hash in order to know under which git revision my software is compiled. It's implemented by a hook in Setup.hs. So that the showVersion function can print something like MySoftware 2.3.0.0-ce78942aed, which is really handy for development. With the new changes in GHC, is there anywhere that I can put this piece of info? Or are you going to create another field in PackageDescription to keep extra notes?

comment:20 Changed 4 years ago by duncan

My suggestion would be that this kind of extra info should go into either the existing generated Paths module, or another generated module. You can do this now with custom code in Setup.hs, but I'm sure we'd welcome a patch to Cabal to make this easier.

comment:21 in reply to:  19 Changed 4 years ago by rodlogic

Replying to zilinc:

I feel it's too late to object, so I'll ask a question instead. I'm developing a software, in which I use the VersionTags field to store the git commit hash in order to know under which git revision my software is compiled. It's implemented by a hook in Setup.hs. So that the showVersion function can print something like MySoftware 2.3.0.0-ce78942aed, which is really handy for development. With the new changes in GHC, is there anywhere that I can put this piece of info? Or are you going to create another field in PackageDescription to keep extra notes?

I also share the same feeling and sorry for being late to comment here. This is quite a common pattern out there. Removing versionTags to avoid the Eq/Ord inconsistency seems like the easiest approach but not the best one, imho. Why not stick to http://semver.org: MAJOR.MINOR.PATCH(-PRERELEASE)?(+tag)*, where:

  • "Pre-release versions have a lower precedence than the associated normal version"
  • "Build metadata SHOULD be ignored when determining version precedence"
Last edited 4 years ago by rodlogic (previous) (diff)

comment:22 Changed 4 years ago by ekmett

One reason for not following semver exactly is that the PVP doesn't line up exactly with semver. Both of our first two digits are "major", and we allow as many subsequent digits as we want. We also don't have an observable prerelease tag for better or worse.

As for version tags being ignored by the Eq instance. That would have been a viable alternative, but it would have meant that Version still didn't line up with what we use it for in ghc, cabal, and elsewhere. IIRC the main use of the tags currently is to talk about a version "*" in some internals in places.

A "structural" Eq has the benefit that x == y implies f x == f y, and doesn't require the user to track the exception to the semantics they expect for equality, whereas under your suggestion we'd lose that.

It'd just be 'some place to shove extra stuff' bolted onto a data type that could be fundamentally simpler. Then if users want to work with a tagged version they can build it by one of several means, for whatever notions of tags they want to allow, using the existing Version type as a primitive component in their own tagged version type with tags being relevant or not for equality as they choose.

comment:23 Changed 4 years ago by zilinc

At least from my own perspective, I think removing versionTags to retain some properties is rational (well to be honest in this particular case I don't even care). And, exactly as Edward said, I just need a replacement. So I simply did as little change as in https://github.com/haskell/cabal/pull/2584 to accomplish that.

comment:24 Changed 3 years ago by thoughtpolice

Milestone: 7.12.18.0.1

Milestone renamed

comment:25 Changed 3 years ago by bgamari

While cleaning up things for the 8.0 release I noticed a TODO in Data.Version mentioning this ticket. My read of this discussion is that the original plan was to remove versionTags (at least from the Eq instance, if not the field itself) in GHC 7.12 (now 8.0). This has not yet happened. It would be nice to have some guidance from the Core Libraries Committee regarding what is slated to happen on this front in this release.

comment:26 Changed 3 years ago by ekmett

Let's leave it alone for 8.0 and fix it in the next release. That will give the cabal folks plenty of time to make sure it works for them.

comment:27 Changed 3 years ago by bgamari

Milestone: 8.0.18.2.1

Sounds good to me. Perhaps this should be added to the Core Library Committee's roadmap (which I seem to recall seeing at one point but am now having bit of trouble finding; perhaps we could add a few more inward-links to this page?)

comment:28 Changed 3 years ago by ekmett

https://prime.haskell.org/wiki/Libraries/Proposals contains the current set of actively worked on proposals.

It currently focuses on things that affect Prelude at the moment, but we can broaden the mandate of the page to cover more API changes.

comment:29 Changed 3 years ago by bgamari

It would be great if at least this particular item could make it on to that list.

In general it would be great to have a comprehensive summary of the committee's future plans.

comment:30 Changed 2 years ago by bgamari

Milestone: 8.2.1

De-milestoning due to lack of progress. If you want this to happen please feel free to pick it up!

Note: See TracTickets for help on using tickets.