On Sun, Oct 16, 2016 at 12:04 AM, Bogdan Sinitsyn [email protected]
wrote:

It's useless when compared with using just any string as map key because
two different modules could have wrapper with the same name

If typeOf is not distinguishing those, then either you have a fairly old
ghc or you have found an unsafeCoerce-level bug. (Or possibly the Show
instance is broken, which is still pretty bad but I suspect the ghc devs
would care less. It would be better if we could use the TypeRep directly,
but we need to be able to serialize the ones representing PersistentState.)

You do get one benefit, provided the String we get is in fact correct:
compile time verification that two extensions are not using the same key
and therefore conflicting with each other. Maybe you consider that useless;
I don't.

And now I wonder if it should be using something other than Show, if that
is producing an incomplete representation. The TypeRep should have enough
information to make an unambiguous String.

brandon s allbery kf8nh sine nomine associates
[email protected] [email protected]
unix, openafs, kerberos, infrastructure, xmonad http://sinenomine.net

compile time verification that two extensions are not using the same key

The point is there is no compile time verification. At least I don't notice it(show (typeOf Test) == "Test" in different modules).

you have a fairly old ghc

No, I have 8.0.1

Or possibly the Show instance is broken

I checked for it: yup, that is broken Show instance(or maybe it's not broken and that behaviour is expected; I haven't found anything that tells that Show instance supports equality like TypeRep)

I'm agree that compile-time check of key difference is important but in does not present now.

It's only Show instance issue, and TypeRep itself checks for equality better. So, the best option I can offer is to switch to using TypeRep as key(AFAIK nothing uses extensible state without contrib module and TypeRep guarantees good compile-time equality check).

On Sun, Oct 16, 2016 at 1:12 AM, Bogdan Sinitsyn [email protected]
wrote:

It's only Show instance issue, and TypeRep itself checks for equality
better. So, the best option I can offer is to switch to using TypeRep as
key(AFAIK nothing uses extensible state without contrib module and TypeRep
guarantees good compile-time equality check).

Did you read what I said? It has to be serializable to pass persistent
state on to the next xmonad instance on mod-q. TypeRep is not directly
serializable, and we'd end up using show to do it just like everything else
we serialize, which we have already determined is broken.

We should stop using the Show instance, though. This may not be possible
with older ghcs, unfortunately. I wonder if typeRepFingerprint is stable
between compiles/between ghc versions... if not, we'll have to disassemble
the TypeRep manually, and that will be compiler version specific to the
extent that at some point in the past TypeRep was simpler and at some point
in the future (8.2.1 is currently planned, I think) it will change again.
(Come to think of it, typeRepFingerprint is also not going to be available
in older versions.)

I'm going to guess compatibility with older ghcs are why this works the way
it does. (Likely related to the deprecated tyConString, which I bet the
Show instance is using.)

brandon s) allbery kf8nh sine nomine
associates
[email protected] [email protected]
unix, openafs, kerberos, infrastructure, xmonad http://sinenomine.net

I wonder if typeRepFingerprint is stable between compiles/between ghc versions...

From https://ghc.haskell.org/trac/ghc/wiki/TypeableT:

Currently, all fingerprints match between the old, base, TypeRep, the new TypeRep and the compatability TypeRep710

And I've checked that it's the same across compiles(haven't found any info on this).

So should we switch to using fingerprints?

We need to fix. However, we also need a way to migrate from the old Show scheme.

As far as GHC goes, I don't think we should officially support anything before 7.6.3. I suppose it would be nice to know which versions of Debian are using older versions of GHC but at some point it's not practical to continue supporting older versions.

I suppose it would be nice to know which versions of Debian are using older versions of GHC

Debian stable (jessie): GHC 7.6.3
Debian testing (stretch): GHC 8.0.1
Debian unstable: GHC 8.0.1

(Debian oldstable (wheezy) has GHC 7.4.1, but no one is expecting anyone to support that.)

Source: packages.debian.org

@asjo thanks!

@f1u77y I've updated the issue title. What do you think?

Also, do you have any suggestions for how to update this without breaking a restart after upgrading?

Transitionally, a Maybe Fingerprint which is filled with Nothing if not present; use the current mechanism when it is found to be Nothing.

The real problem is that we'd be relying on a ghc internal mechanism, which as I tried to hint earlier strikes me as a risky notion regardless of how convenient it would be. (I don't even know if the new type-indexed Typeable in 8.2 still uses something similar, or in fact how that affects this mechanism at all.)

I agree. I'd rather use a stable library that provides what we need instead of a compiler feature. I don't know if anything like that exists though.

Worst case, we have to make it a manual process in the instance definition and go through the code and fix everything. That's not a terrible proposition.

@pjones
Couln't we use TH for that? It generates Module.Name.TypeName, so there shouldn't be any collisions when using it.

Also, do you have any suggestions for how to update this without breaking a restart after upgrading?

Use an commandline option to indicate new format of ExtensibleState serialisation and use old code for deserializing if we need.

@f1u77y I suppose that depends if we want to introduce TH has a dependency. I'm not convinced that's the right way to go.

What's wrong with TH as dependency?

TemplateHaskell introduces portability issues. I may be wrong so please correct me, but I don't think TH works on ARM architectures and it completely breaks cross compiling. It seems to me that this is a really silly use of TH to cause so much breakage.

Also there's a hidden issue with just using the type name instead of a fingerprint, assuming fingerprints are reproducibly generated from the actual type (not just its name): we should not attempt to deserialize if the type changed.

I think this could be hacked up using only Typeable that we already require for it. It'd be ugly at best though; if we can rely on a fingerprint generated by the compiler, we should. I just don't trust doing so in the absence of a public API. :/

What about this?

> (\t -> tyConModule (typeRepTyCon t) ++ "." ++ show t) (typeOf (1::Integer))
"GHC.Integer.Type.Integer"

Seems a good quick fix. Doesn't fix the deserialization issue, though.

Turns out fingerprints aren't stable. They may be stable across GHC versions, but they're definitely not stable across xmonad versions:

*Main Data.Typeable XMonad> (\t -> tyConPackage (typeRepTyCon t)) $ typeOf (undefined :: XConf)
"xmonad-0.16.9999-Ld1LH0Co1h93slbLPYTq5r"

(the fingerprint is constructed from the package name, module name and type name)

So I guess we go ahead with my hack ((\t -> tyConModule (typeRepTyCon t) ++ "." ++ show t)), right? Shall we try to include backwards compat fallbacks for smooth upgrades of running xmonad instances?

I guess we're stuck with your hack. And probably yes to the fallback or everyone loses upon mod-q into the new version (I'm sure I'm not the only one who has done that… probably everyone who runs git xmonad has done so at least once).

I have some draft PRs: xmonad/xmonad#326, #600
I don't like the code so I hope you guys come up with some other ideas :-)

This isn't critical for the release so I'm moving this and the associated PRs to the v0.18 milestone.