WIP: GHC 8.2 revamp #182

RyanGlScott · 2017-04-22T22:10:28Z

Fixes #180 while I'm in town.

RyanGlScott

Some concerns inline.

RyanGlScott · 2017-04-22T22:11:29Z

src/Data/Singletons/Promote.hs

@@ -552,6 +554,7 @@ promotePat (DBangPa pat) = do
  qReportWarning "Strict pattern converted into regular pattern in promotion"
  (ty, pat') <- promotePat pat
  return (ty, DBangPa pat')
+promotePat (DSigPa _pat _ty) = error "TODO: Handle SigPa"


To be honest, I'm not quite sure how one would promote a DSigPa. Suggestions welcome.

RyanGlScott · 2017-04-22T22:11:56Z

src/Data/Singletons/Single.hs

@@ -499,6 +501,7 @@ singPat var_proms patCxt (DTildePa pat) = do
 singPat var_proms patCxt (DBangPa pat) = do
  (pat', ty) <- singPat var_proms patCxt pat
  return (DBangPa pat', ty)
+singPat _var_proms _patCxt (DSigPa _pat _ty) = error "TODO: Handle SigPa"


Similarly, how do we single DSigPas?

RyanGlScott · 2017-04-22T22:13:27Z

src/Data/Singletons/Partition.hs

+          stock = case strat of
+                    Nothing            -> True
+                    Just StockStrategy -> True
+                    Just _             -> False


I opted to only choose deriving clauses with an explicit strategy when that strategy is stock. We could make it more permissive and ignore the strategy, however.

I agree with this choice.

RyanGlScott · 2017-04-22T22:14:07Z

src/Data/Singletons/Prelude/List.hs

+              -- implementation of permutations in base. However, it is needed
+              -- here, since (at least in GHC 8.2.1) the singletonized version
+              -- will fail to typecheck without it. See #13549 for the full story.
+              interleave' :: ([a] -> b) -> [a] -> [b] -> ([a], [b])


It turns out adding one type signature is enough to work around GHC Trac #13549. Hooray!

See my comment to GHC's #13459. This type signature is indeed necessary: a type signature is required on a local function if its type is inferred differently depending on the presence of MonoLocalBinds. This is the case here.

RyanGlScott · 2017-04-22T22:15:10Z

tests/compile-and-dump/Singletons/BadBoundedDeriving.ghc82.template

@@ -1,3 +1,6 @@

 Singletons/BadBoundedDeriving.hs:0:0: error:
    Can't derive Bounded instance for Foo_0 a_1.
+  |
+6 | $(singletons [d|
+  |   ^^^^^^^^^^^^^^...


Do you think we should include these error message locations in golden files? They do add some degree of bloat.

RyanGlScott · 2017-04-22T22:39:50Z

Another TODO: the Travis build is failing because Cabal-2.0 is inclined to build the test suite before the library. Unfortunately, this breaks the assumption that "../dist/build/autogen/cabal_macros.h" will exist by the time the test suite is built (see this Travis error for an example).

Perhaps we should just make the test suite depend on singletons in singletons.cabal? Then again, there's this Note:

-- Note [-this-unit-id hack]
-- ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
--
-- We want to avoid installing singletons package before running the
-- testsuite, because in this way we prevent double compilation of the
-- library.

I don't think making the test suite depend on singletons in singletons.cabal would result in double compilation, but then again, I am quite new to the inner machinations of this test suite.

goldfirere

This is a large patch. I will review when I have a bit more time than I do at the moment -- perhaps tomorrow. Many thanks for doing this!

goldfirere · 2017-04-23T03:06:04Z

src/Data/Singletons/Prelude/List.hs

+              -- implementation of permutations in base. However, it is needed
+              -- here, since (at least in GHC 8.2.1) the singletonized version
+              -- will fail to typecheck without it. See #13549 for the full story.
+              interleave' :: ([a] -> b) -> [a] -> [b] -> ([a], [b])


See my comment to GHC's #13459. This type signature is indeed necessary: a type signature is required on a local function if its type is inferred differently depending on the presence of MonoLocalBinds. This is the case here.

goldfirere · 2017-04-23T13:01:13Z

Somehow just saw your last comment, about Cabal and -this-unit-id. It's quite possible that modern Cabals won't cause double-compilation.... or simply that we had configured it wrongly when setting this up. I don't think it would severely break things to add the singletons dependency.

RyanGlScott · 2017-04-23T13:58:53Z

OK, adding singletons as a test suite dependency seemed to fix that particular problem. But now the tests themselves are failing, apparently because of different unique values. But we enable -dsuppress-uniques... hm. I need to stare at this more closely.

RyanGlScott · 2017-04-23T14:11:47Z

Ah, here's a hunch. In the test suite, we do normalization with sed:

-- Note [Normalization with sed]
-- ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
--
-- Output file is normalized with sed. Line numbers generated in splices:
--
--   Foo:(40,3)-(42,4)
--   Foo.hs:7:3:
--   Equals_1235967303
--
-- are turned into:
--
--   Foo:(0,0)-(0,0)
--   Foo.hs:0:0:
--   Equals_0123456789

In particular, we normalize this pattern:

"-e", "'s/[0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9]/0123456789/g'"

This works assuming that whenever you have something like Equals_1235967303, it will always have exactly 10 numbers following it. But in GHC 8.2, uniques appear to have 19 numbers (perhaps due to this commit?). Maybe adding a pattern for 19 numbers will do the trick.

RyanGlScott · 2017-04-23T15:17:12Z

Luckily, adding more patterns for sed was precisely the secret sauce needed to cook up a green Travis build.

RyanGlScott · 2017-05-31T02:00:37Z

Should this be closed now that 637fb4d has been committed?

The real work is in #182.

goldfirere · 2017-05-31T03:27:06Z

Don't close quite yet. I did merge (it passes the testsuite) but I haven't reviewed carefully yet. Will do so soon.

goldfirere

Reviewed. Looks great. Thanks.

goldfirere · 2017-05-31T14:00:37Z

src/Data/Singletons/Partition.hs

+          stock = case strat of
+                    Nothing            -> True
+                    Just StockStrategy -> True
+                    Just _             -> False


I agree with this choice.

goldfirere · 2017-05-31T14:07:27Z

This has been merged.

RyanGlScott commented Apr 22, 2017

View reviewed changes

goldfirere reviewed Apr 23, 2017

View reviewed changes

RyanGlScott added 2 commits April 23, 2017 10:40

GHC 8.2 revamp

9633fcb

Fix extra-source-files

784257f

goldfirere pushed a commit that referenced this pull request May 31, 2017

Fix #180 by adding regression test.

17f4703

The real work is in #182.

goldfirere reviewed May 31, 2017

View reviewed changes

goldfirere closed this May 31, 2017

RyanGlScott mentioned this pull request Jul 27, 2017

Do doctests test suites always need to depend on the library? ulidtko/cabal-doctest#19

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: GHC 8.2 revamp #182

WIP: GHC 8.2 revamp #182

RyanGlScott commented Apr 22, 2017

RyanGlScott left a comment

RyanGlScott Apr 22, 2017

RyanGlScott Apr 22, 2017

RyanGlScott Apr 22, 2017

goldfirere May 31, 2017

RyanGlScott Apr 22, 2017

goldfirere Apr 23, 2017

RyanGlScott Apr 22, 2017

RyanGlScott commented Apr 22, 2017

goldfirere left a comment

goldfirere Apr 23, 2017

goldfirere commented Apr 23, 2017

RyanGlScott commented Apr 23, 2017

RyanGlScott commented Apr 23, 2017

RyanGlScott commented Apr 23, 2017

RyanGlScott commented May 31, 2017

goldfirere commented May 31, 2017

goldfirere left a comment

goldfirere May 31, 2017

goldfirere commented May 31, 2017

WIP: GHC 8.2 revamp #182

WIP: GHC 8.2 revamp #182

Conversation

RyanGlScott commented Apr 22, 2017

RyanGlScott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanGlScott commented Apr 22, 2017

goldfirere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goldfirere commented Apr 23, 2017

RyanGlScott commented Apr 23, 2017

RyanGlScott commented Apr 23, 2017

RyanGlScott commented Apr 23, 2017

RyanGlScott commented May 31, 2017

goldfirere commented May 31, 2017

goldfirere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goldfirere commented May 31, 2017