Facility to mitigate double-breakage due to Data.Version.versionTag Deprecation

[Herbert Valerio Riedel]

Hello CLC,

See discussion starting at https://phabricator.haskell.org/D395#12229
for backstory

So Edward is worrying about not being able to write warning-free code
for GHC 7.10 (which DEPRECATEs `versionTag`) and which keeps working
once `versionTag` is really removed (*probably* in 7.12, but there's no
guarantee it won't happen later due to unforeseen reasons)

The problem is mostly constructing a `Version`, as that's what's going
to be different once the tags field is removed. Pattern matching can be
done in a fully back/forward compatible way using record-syntax (or
simply using the field-accessor 'versionBranch' as a function):

foo (Version { versionBranch = ver }) = ...

I was hoping that the new bidirectional `PatternSynonyms` would help us
out here, but it requires a language extension (see http://lpaste.net/115088)


So, here's two concrete alternative suggestions on how to mitigate the
double-breakage I'd like a decision on from the core-lib-committee on
which one to use (if any) for the upcoming GHC 7.10.1:


a)

-- | Maximum monoid over `Version`s
instance Monoid Version where
mempty = Version [] []
mappend v1 v2 = Version (max v1 v2) []


There doesn't seem to be any other sensible Monoid instance you can
define over `Version` I can think of (or is there?)

b)

-- | /Empty/ `Version` value. This also represents the absolute
-- `Ord` minimum of `Version`
emptyVersion = Version [] []



Both variants, a) and b) allow us to construct a 'Version' value in a
back/fwd compat way as in (replace `XXX` by `mempty` for a), and by
`emptyVersion` for b) ) like

mkVer :: (Int,Int,Int) -> Version
#if MIN_VERSION_base(4,8,0)
mkVer (mj,mi,pl) = XXX { versionBranch = [mj,mi,pl] }
#else
mkVer (mj,mi,pl) = Version [mj,mi,pl] []
#endif




Cheers,
hvr

[Edward Kmett]

Minor tweak:

    mappend v1 v2 = Version (max v1 v2) []

should just be

   mappend = max

The Monoid instance seems particularly clean to me, if somewhat non-obvious to newcomers looking to future proof their code.

[] is the lexicographically least [Int], so this is just the (max, minBound) monoid.

It also has the benefit of not taking up a name, and it does strike me as the only sensible Monoid instance anyways.

-Edward

--
You received this message because you are subscribed to the Google Groups "haskell-core-libraries" group.
To unsubscribe from this group and stop receiving emails from it, send an email to haskell-core-libr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Herbert Valerio Riedel]

On 2014-11-26 at 08:58:19 +0100, Edward Kmett wrote:

[...]

> The Monoid instance seems particularly clean to me, if somewhat non-obvious
> to newcomers looking to future proof their code.

What I forgot to mention: I plan to add compatibility-notes wrt
future-proofing code to Haddock somewhere in Data.Version
(that, of course, still requires newcomers to actually ready the
Data.Version Haddock docs...)

[Michael Snoyman]

Just to make sure I understand: the idea here is that code written for GHC 7.8 and earlier will need to be written separately, but for 7.10 and onward the same code will work warning-free.

I think providing a Monoid instance like this makes sense, and I *do* prefer it to coming up with just another name. +1.

Michael 

[Eric Mertens]

While I agree that the instance is a fine idea on its own, I don't agree that it is a proper substitute for a properly named value for use when specifically intending to refer to an empty version.

While it might be the only obvious instance, we're talking about a much more general class method with mempty than we might be with something like a foldl where I would be inclined to support the class method existing without a proper name and haddock entry.

--
Eric

--

[Edward Kmett]

I'm not averse to doing both.

For that matter IsList could also be used, in much the same manner.

That instance probably belongs there if we're getting rid of the tags anyways.

-Edward

[Duncan Coutts]

I'd tend to agree with Eric here that ideally we want named constructor
and access functions (instances are nice too). But it's not a very
strong preference, the most important thing is that we can reduce
breakage.

Another good reason for named constructor and access functions is that
we might later be able to make the data type abstract. My motivation for
that is that we could use a more compact representation for the common
forms. You'd be surprised how much memory in cabal and hackage etc is
dedicated to version numbers. They're very low density. Most versions
would fit in a Word64 (as 4 16-bit fields), which would cost 3 words
total, where as current 4-place versions use 21 words!

Duncan (who has been planning for some time to make the Cabal data types
more compact)

--
Duncan Coutts, Haskell Consultant
Well-Typed LLP, http://www.well-typed.com/

[Herbert Valerio Riedel]

On 2014-11-26 at 13:15:35 +0100, Duncan Coutts wrote:

[...]

> Another good reason for named constructor and access functions is that
> we might later be able to make the data type abstract. My motivation for
> that is that we could use a more compact representation for the common
> forms.

...why didn't you mention that earlier? :)

I previously didn't see any point in suggesting the

makeVersion :: [Int] -> Version

variant (and maybe discourage users from directly pattern matching on
`Version`), as I felt it to be rather redundant given a future (post
GHC-7.12+)

Version :: [Int] -> Version

constructor...



...too bad bidirectional PatternSynonyms require -XPatternSynonyms to be
enabled in code using them as value constructors...

otherwise we could put them to use to smooth a transition to
an opaque `Version`... as on the outside it would behave just as if it
was really just

data Version = Version [Int]

but the pattern-synonyms would translate that back/forth into the
internal opaque representation...




PS: As a side-note, I'm somewhat unhappy about [Int] allowing for
negative version components suchs as [-1,2,3]... but let's leave it
at that -- don't want to cause additional bikeshedding =)

[Herbert Valerio Riedel]

On 2014-11-26 at 13:31:28 +0100, Herbert Valerio Riedel wrote:

[...]

> ...too bad bidirectional PatternSynonyms require -XPatternSynonyms to be
> enabled in code using them as value constructors...
>
> otherwise we could put them to use to smooth a transition to
> an opaque `Version`... as on the outside it would behave just as if it
> was really just
>
> data Version = Version [Int]
>
> but the pattern-synonyms would translate that back/forth into the
> internal opaque representation...

I got that one backward; it's when you actually use a pattern-syn for
pattern-matching that you need `-XPatternSynonyms` (see GHCi example below)

...to follow-up on this idea, here's some proof-of-concept code:


{-# LANGUAGE PatternSynonyms, ViewPatterns #-}

module Version (Version, pattern Version, versionBranch) where

-- import Data.Version

data VersionImpl = V0
| V1 !Int
| V2 !Int !Int
| V3p !Int !Int !Int [Int]
deriving (Show,Eq)
-- TODO: proper 'Show' instance & others

makeVersion :: [Int] -> Version
makeVersion [] = V0
makeVersion [a] = V1 a
makeVersion [a,b] = V2 a b
makeVersion (a:b:c:ds) = V3p a b c ds

-- | Backward compat field-accessor function
versionBranch :: Version -> [Int]
versionBranch V0 = []
versionBranch (V1 a) = [a]
versionBranch (V2 a b) = [a,b]
versionBranch (V3p a b c ds) = (a:b:c:ds)

type Version = VersionImpl

pattern Version :: [Int] -> Version
pattern Version v <- (versionBranch -> v) where
Version v = makeVersion v


when compiled, this mostly like if it was merely defined as

data Version = Version [Int]

deriving ...

makeVersion :: [Int] -> Version
makeVersion = Version

e.g.:

GHCi, version 7.9.20141124: http://www.haskell.org/ghc/ :? for help
Ok, modules loaded: Version.
λ:2> :browse
pattern Version :: [Int] -> Version
type Version = Version.VersionImpl
versionBranch :: Version -> [Int]

λ:3> Version [1]
V1 1
it :: Version

λ:4> Version [1,2,3,4]
V3p 1 2 3 [4]
it :: Version

λ:5> let v23 = Version [2,3]
v23 :: Version

λ:6> case v23 of Version x -> x
<interactive>:6:13:
A pattern match on a pattern synonym requires PatternSynonyms
In the pattern: Version x
In a case alternative: Version x -> x
In the expression: case v23 of { Version x -> x }

λ:7> :set -XPatternSynonyms

λ:8> case v23 of Version x -> x
[2,3]
it :: [Int]


So the only thing that's unfortunate is the need to enable
-XPatternSynonyms at use-sites, otherwise this would almost seem perfect
for Duncan's use-case...


Cheers,
hvr

[Dr. ERDI Gergo]

On Wed, 26 Nov 2014, Herbert Valerio Riedel wrote:

> So the only thing that's unfortunate is the need to enable
> -XPatternSynonyms at use-sites, otherwise this would almost seem perfect
> for Duncan's use-case...

I'm not against removing this restriction, if pattern synonyms are now
wide-spread enough that they're not causing surprise when used without
explicitly meaning to.

--

.--= ULLA! =-----------------.
\ http://gergo.erdi.hu \
`---= ge...@erdi.hu =-------'
'Two beer or not two beer?'-Shakesbeer

[Edward Kmett]

FWIW-

I'm okay with adding makeVersion.

I'm okay with adding emptyVersion.

I'm okay with adding the (max,minBound) Monoid.

I'm okay with adding IsList.

I'm even okay with a pattern synonym.

In general I'm okay with adding any non-empty subset of the above functionality, or all of it.

I'm happy to yield to anyone who has a grievance against any one of those things. I just want the functionality, the form factor it comes packaged in is largely irrelevant to me. =)

-Edward

[Herbert Valerio Riedel]

On 2014-11-26 at 15:38:26 +0100, Dr. ERDI Gergo wrote:
> On Wed, 26 Nov 2014, Herbert Valerio Riedel wrote:
>
>> So the only thing that's unfortunate is the need to enable
>> -XPatternSynonyms at use-sites, otherwise this would almost seem perfect
>> for Duncan's use-case...
>
> I'm not against removing this restriction, if pattern synonyms are now
> wide-spread enough that they're not causing surprise when used without
> explicitly meaning to.

I think the restriction should be removed
(to increase -XPatternSynonyms adoption).

There are precedents of other GHC language extensions which only require
the language-pragma to be enabled at the definition site, but not at the
use-site if the use-site code doesn't require necessarily any extended
grammar/syntax (to name a few: fundeps, associated types, default-signatures)

And -XPatternSynonyms is such a case as well imho, where the code making
use of a defined pattern-synonyms can be plain Haskell2010 code.

Cheers,
hvr

[Edward Kmett]

See https://ghc.haskell.org/trac/ghc/ticket/9838

Cheers,
  hvr

[sefe...@gmail.com]

Herbert Valerio Riedel wrote:
>> So the only thing that's unfortunate is the need to enable
>> -XPatternSynonyms at use-sites

Edward Kmett wrote:
> See https://ghc.haskell.org/trac/ghc/ticket/9838

Already implemented, great! But… It *still* won't be widely
adopted unless Haddock supports it. No one wants to publish
an API that can't be understood by looking at the Haddocks.
Has this been done yet?

Thanks,
Yitz

[Edward Kmett]

Pattern synonyms work in haddock. 

There are some bugs with haddock and 7.8.3 around pattern synonyms caused by an internal GHC issue, but that issue is fixed in 7.8.4 and only affects haddock generation for a module that re-exports a pattern synonym from another module.

Most usecases "just work" though.

-Edward

[Edward Kmett]

Ok, we're coming down to the wire on this and need to make a decision if we're not going to have breakage.

To get this down to a 'yes/no', since nobody seems to have any further comments / objections:

I propose:

Lets just add makeVersion :: [Int] -> Version and an instance IsList Version and call it good.

These are the most obvious solutions, they don't require arbitrary choices, and they are enough to allow anyone who wants to fix the warnings caused by the use of versionTags to be able to solve them now rather than have to do half now, half in a year.

Any objections?

-Edward

[Eric Mertens]

That seems perfectly reasonable to me.