Do not imply NoStarIsType by TypeOperators/TypeInType
ClosedPublic

Authored by int-index on Jun 18 2018, 1:17 AM.

Details

Summary

Implementation of the "Embrace TypeInType" proposal was done according to the
spec, which specified that TypeOperators must imply NoStarIsType. This
implication was meant to prevent breakage and to be removed in 2 releases.
However, compiling head.hackage has shown that this implication only magnified
the breakage, so there is no reason to have it in the first place.

To remain in compliance with the three-release policy, we add a workaround
to define the (*) type operator even when -XStarIsType is on.

Test Plan

./validate

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
int-index created this revision.Jun 18 2018, 1:17 AM
goldfire added inline comments.Mon, Jun 18, 8:35 PM
compiler/basicTypes/RdrName.hs
1314–1317

I suppose this isn't new, but I'd prefer avoiding using a gendered pronoun here. How about "The user might unknowingly be working on a module..."?

compiler/main/DynFlags.hs
4368

This change surprises me. I see no discussion of this in the github proposal.

4410

I don't think we need to include the tortuous history here. End the sentence at "on Hackage".

compiler/parser/RdrHsSyn.hs
1759

I think "desugaring" is a technical term. Perhaps use "Declaring"?

docs/users_guide/glasgow_exts.rst
8716

If you undo the removal of the TypeInType ==> NoStarIsType implication, this should be updated, too.

9169

I think this is out of sync with other changes.

int-index added inline comments.Mon, Jun 18, 9:53 PM
compiler/basicTypes/RdrName.hs
1314–1317

Sorry, in my native language even inanimate objects are gendered, so I fail to notice such things sometimes. I didn't mean to imply that users are all male. Will surely fix.

compiler/main/DynFlags.hs
4368

Well, making TypeInType imply NoStarIsType in the first place wasn't in any proposal either. I thought about it and couldn't come up with a reason to keep the implication. Until the "Remove the * kind syntax" proposal gets accepted, it shouldn't StarIsType be the default?

compiler/parser/RdrHsSyn.hs
1759

I believe many programmers are familiar with the idea of syntactic sugar and "declaring" isn't really what the warning is about.

I think "desugaring" is a technical only in GHC sources and means "translating to Core", and this is not the sense of the word in which I used it here.

I also happen to use the word "desugaring" in the user guide to describe what StarIsType does, so this is consistent.

docs/users_guide/glasgow_exts.rst
9169

What do you mean? With this patch StarIsType is no longer disabled by TypeOperators or TypeInType, so I deleted this sentence.

int-index updated this revision to Diff 17008.Mon, Jun 18, 10:05 PM

Comments and rebase

goldfire added inline comments.Tue, Jun 19, 11:46 AM
compiler/main/DynFlags.hs
4368

I guess you're right.

compiler/parser/RdrHsSyn.hs
1759

Can we say "Treating" then instead of desugaring? One of the ways in which syntactic sugar works is that users aren't supposed to care that it's sugar.

docs/users_guide/glasgow_exts.rst
9169

You're right. I think I got the polarity of the diff wrong.

A few more things needed here.

Also, have you checked what issues remain when building head.hackage?

compiler/main/DynFlags.hs
3861

Can you add this to the users guide (using-warnings.rst)?

compiler/parser/RdrHsSyn.hs
1759

I agree that "desugaring" isn't terribly appropriate here. Perhaps,

Found binding occurrence of * yet StarIsType is enabled. N.B. To use (or export) this operator in modules with StarIsType enable you must qualify it.

bgamari requested changes to this revision.Sun, Jun 24, 2:12 PM
This revision now requires changes to proceed.Sun, Jun 24, 2:12 PM

Also, have you checked what issues remain when building head.hackage?

I was curious about this myself, so I took this patch for a spin and built a cross section of large Hackage packages. I noticed five breakages. Four of them were due to GHC.TypeLits.* being misinterpreted as Type, while one (reflection) was due to trouble parsing the Template Haskell name ''(*). Here is how I patched each one:

  • basement
diff -ru basement-0.0.7.orig/Basement/Nat.hs basement-0.0.7/Basement/Nat.hs
--- basement-0.0.7.orig/Basement/Nat.hs 2017-11-11 04:52:31.000000000 -0500
+++ basement-0.0.7/Basement/Nat.hs      2018-06-24 17:54:55.038235612 -0400
@@ -8,6 +8,9 @@
 {-# LANGUAGE ScopedTypeVariables       #-}
 {-# LANGUAGE UndecidableInstances      #-}
 {-# LANGUAGE ConstraintKinds           #-}
+#if __GLASGOW_HASKELL__ >= 805
+{-# LANGUAGE NoStarIsType #-}
+#endif
 module Basement.Nat
     ( Nat
     , KnownNat
diff -ru basement-0.0.7.orig/Basement/Sized/Block.hs basement-0.0.7/Basement/Sized/Block.hs
--- basement-0.0.7.orig/Basement/Sized/Block.hs 2018-02-12 09:24:12.000000000 -0500
+++ basement-0.0.7/Basement/Sized/Block.hs      2018-06-24 17:56:02.506237311 -0400
@@ -5,13 +5,16 @@
 --
 -- A Nat-sized version of Block
 {-# LANGUAGE AllowAmbiguousTypes        #-}
+{-# LANGUAGE CPP                        #-}
 {-# LANGUAGE DataKinds                  #-}
 {-# LANGUAGE TypeOperators              #-}
 {-# LANGUAGE TypeApplications           #-}
 {-# LANGUAGE ScopedTypeVariables        #-}
 {-# LANGUAGE GeneralizedNewtypeDeriving #-}
 {-# LANGUAGE ConstraintKinds            #-}
-
+#if __GLASGOW_HASKELL__ >= 805
+{-# LANGUAGE NoStarIsType #-}
+#endif
 module Basement.Sized.Block
     ( BlockN
     , MutableBlockN
  • constraints
diff -ru constraints-0.10.orig/src/Data/Constraint/Nat.hs constraints-0.10/src/Data/Constraint/Nat.hs
--- constraints-0.10.orig/src/Data/Constraint/Nat.hs    2018-01-18 14:26:31.000000000 -0500
+++ constraints-0.10/src/Data/Constraint/Nat.hs 2018-06-24 18:17:26.938269657 -0400
@@ -10,6 +10,9 @@
 {-# LANGUAGE ScopedTypeVariables #-}
 {-# LANGUAGE AllowAmbiguousTypes #-}
 {-# LANGUAGE Trustworthy #-}
+#if __GLASGOW_HASKELL__ >= 805
+{-# LANGUAGE NoStarIsType #-}
+#endif
 -- | Utilities for working with 'KnownNat' constraints.
 --
 -- This module is only available on GHC 8.0 or later.
  • memory
diff -ru memory-0.14.16.orig/Data/ByteArray/Sized.hs memory-0.14.16/Data/ByteArray/Sized.hs
--- memory-0.14.16.orig/Data/ByteArray/Sized.hs 2018-02-26 05:46:08.000000000 -0500
+++ memory-0.14.16/Data/ByteArray/Sized.hs      2018-06-24 17:59:32.450242598 -0400
@@ -21,7 +21,9 @@
 {-# LANGUAGE MultiParamTypeClasses #-}
 {-# LANGUAGE FunctionalDependencies #-}
 {-# LANGUAGE UndecidableInstances #-}
-
+#if __GLASGOW_HASKELL__ >= 805
+{-# LANGUAGE NoStarIsType #-}
+#endif
 module Data.ByteArray.Sized
     ( ByteArrayN(..)
     , SizedByteArray
  • reflection
diff -ru reflection-2.1.3.orig/fast/Data/Reflection.hs reflection-2.1.3/fast/Data/Reflection.hs
--- reflection-2.1.3.orig/fast/Data/Reflection.hs       2018-01-18 19:35:38.000000000 -0500
+++ reflection-2.1.3/fast/Data/Reflection.hs    2018-06-24 17:43:32.206218416 -0400
@@ -330,7 +330,7 @@
   a + b = AppT (AppT (VarT ''(+)) a) b

   LitT (NumTyLit a) * LitT (NumTyLit b) = LitT (NumTyLit (a*b))
-  (*) a b = AppT (AppT (VarT ''(*)) a) b
+  (*) a b = AppT (AppT (VarT ''(GHC.TypeLits.*)) a) b
 #if MIN_VERSION_base(4,8,0)
   a - b = AppT (AppT (VarT ''(-)) a) b
 #else
  • singletons
diff -ru singletons-2.4.1.orig/src/Data/Singletons/Prelude/Num.hs singletons-2.4.1/src/Data/Singletons/Prelude/Num.hs
--- singletons-2.4.1.orig/src/Data/Singletons/Prelude/Num.hs   2018-01-08 11:09:19.000000000 -0500
+++ singletons-2.4.1/src/Data/Singletons/Prelude/Num.hs        2018-06-24 18:10:38.266259365 -0400
@@ -1,7 +1,10 @@
 {-# LANGUAGE TemplateHaskell, PolyKinds, DataKinds, TypeFamilies, TypeInType,
              TypeOperators, GADTs, ScopedTypeVariables, UndecidableInstances,
-             DefaultSignatures, FlexibleContexts
+             DefaultSignatures, FlexibleContexts, CPP
   #-}
+#if __GLASGOW_HASKELL__ >= 805
+{-# LANGUAGE NoStarIsType #-}
+#endif

 -----------------------------------------------------------------------------
 -- |

Also, have you checked what issues remain when building head.hackage?

I was curious about this myself, so I took this patch for a spin and built a cross section of large Hackage packages. I noticed five breakages. Four of them were due to GHC.TypeLits.* being misinterpreted as Type, while one (reflection) was due to trouble parsing the Template Haskell name ''(*). Here is how I patched each one:

Mmm, so it looks to me like there's little chance that these will be doable in a three-release-policy-compliant manner. That being said, I believe the three-release-policy strictly speaking only covers library changes, so I suppose that is okay, especially given the small number of affected packages.

RyanGlScott added a comment.EditedMon, Jun 25, 3:40 PM

Mmm, so it looks to me like there's little chance that these will be doable in a three-release-policy-compliant manner.

To be clear, I was aiming for the smallest changes possible when I wrote those patches, not for strict compliance with the three-release policy. You actually could maintain three-release policy compliance by just prefixing every use of * with an explicit GHC.TypeLits. module prefix, but I opted not to do that in some of those patches since it would took fewer keystrokes to simply use CPP.

Ahh yes, of course! Lovely, I suppose there is no problem here then.

int-index updated this revision to Diff 17095.Mon, Jun 25, 10:27 PM

Update documentation and warning message

What is the status of this? It looks like this patch landed in the ghc-8.6 branch in abd6622324733c67b05e0cbd0c8c3d12c6332f61, but it is not present on master.

I pulled the patch into ghc-8.6 for the sake of getting the alpha out but haven't yet merged it to master since the proposal hasn't yet been formally accepted by the commitee or gone through a thorough code review. Once this happens I will revert the patch and reapply the final version to both master and ghc-8.6.

RyanGlScott accepted this revision.Wed, Jul 11, 11:36 AM

The GHC steering committee just approved https://github.com/ghc-proposals/ghc-proposals/pull/146, so I suppose it's time to land this in master proper.

bgamari accepted this revision.Mon, Jul 16, 5:40 PM
This revision is now accepted and ready to land.Mon, Jul 16, 5:40 PM
This revision was automatically updated to reflect the committed changes.