ExtensibleState map keys are not unique

[f1u77y]

It's useless when compared with using just any string as map key because two different modules could have wrapper with the same name, and checking for that is not easier then checking for same string as key. It does also require these annoying wrappers and unwrappers for no benefit. Should we migrate to using any string as keys or is it useless or even wrong?

[geekosaur]

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

[f1u77y]

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).

[geekosaur]

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

[f1u77y]

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?

[pjones]

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.

[ghost]

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

[pjones]

@asjo thanks!

[pjones]

@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?

[geekosaur]

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.)

[pjones]

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.

[f1u77y]

@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.

[pjones]

@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.

[f1u77y]

What's wrong with TH as dependency?

[pjones]

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.

[geekosaur]

Both of those are being worked on in ghc8, but neither is quite stable yet --- and only ghc8 can even pretend to deal with it. (Cross compiling is still known to not work reliably, because a splice may run a previously compiled function and that function will have been compiled for either the host or the target, not "for the host but generating code for the target".) And yes, there's been at least one report of someone cross-compiling xmonad for a RasPi.

On Thu, Apr 13, 2017 at 11:08 AM, Peter J. Jones [email protected] wrote:

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.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/xmonad/xmonad-contrib/issues/94#issuecomment-293923244, or mute the thread https://github.com/notifications/unsubscribe-auth/AB8SoAkcPGDUNubk2APLcm_hG56E8ksQks5rvjqBgaJpZM4KX4f4 .

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

[geekosaur]

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. :/

[liskin]

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.

[liskin]

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?

[geekosaur]

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).

[liskin]

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

[liskin]

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