Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate to ghc-lib-parser #423

Merged
merged 1 commit into from
Nov 1, 2019
Merged

Migrate to ghc-lib-parser #423

merged 1 commit into from
Nov 1, 2019

Conversation

neongreen
Copy link
Collaborator

@neongreen neongreen commented Oct 27, 2019

Fixes #422, fixes #344.

Note to the reviewer

This can be reviewed commit-by-commit.

TODO

  • Fix the way I work with XPat.

  • Run nixpkgs tests.

  • Check that the executable is buildable.

  • Fix warnings.

  • Format with Ormolu.

  • Add a test for:

    type family G a b where
      forall x y. G [x] (Proxy y) = Double
      forall z.   G z   z         = Bool
    
  • Implement support for the point above.

  • Investigate:

    {-# LANGUAGE TypeFamilies, TypeOperators, NoStarIsType #-}
    
    instance PNum Nat where
      type a * b = ()
    
  • Go through singletons and look for more regressions.

  • Add a test and implementation for:

    {-# RULES "example" forall a. forall (x :: a). id x = x #-}
    
  • Add tests for:

    type family a ! b
    type family a . b
    
  • Add paren tests:

    class a + b
    data D1 = forall a b. (a + b) => D1 a b
    data D2 = forall a b.  a + b  => D2 a b -- now allowed
    
    data T = MkT1 { a :: {-# UNPACK #-} (Maybe Int && Bool) }
           | MkT2 { a :: {-# UNPACK #-}  Maybe Int && Bool  } -- now allowed
    
    data G where
      MkG1 :: {-# UNPACK #-} (Maybe Int && Bool) -> G
      MkG2 :: {-# UNPACK #-}  Maybe Int && Bool  -> G  -- now allowed
    
    Proxy '(a :: A, b :: B)
    
  • Try out a rule with two levels of multiline foralls.

  • Fix regression: the forall should not be broken across several lines:

    sEither ::
      forall a b c (l :: a ~> c) (r :: b ~> c) (e :: Either a b).
      (forall n. Proxy l -> Sing n -> Sing (Apply l n)) ->
      (forall n. Proxy r -> Sing n -> Sing (Apply r n)) ->
      Sing e ->
      Sing (Either_ l r e)
    sEither l _ (SLeft x) = l Proxy x
    sEither _ r (SRight x) = r Proxy x
  • Add a test for an (accidental?) improvement: the :<>: declaration should remain on the same line:

    data ErrorMessage' s
      = -- | Show the text as is.
        Text s
      | -- | Pretty print the type.
        -- @ShowType :: k -> ErrorMessage@
        forall t. ShowType t
      | -- | Put two pieces of error message next
        -- to each other.
        ErrorMessage' s :<>: ErrorMessage' s
      | -- | Stack two pieces of error message on top
        -- of each other.
        ErrorMessage' s :$$: ErrorMessage' s

-- located 'Pat's. However, sometimes we want to use the location to render
-- something other than the given 'Pat'.
--
-- If given 'Pat' does not contain a location, we error out.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm rather disappointed with GHC here.

Of course, an alternative option is swallowing the bug and doing p -> ($ p) if we get something other than an XPat. I'm not sure what is the best course of action here.

@@ -103,8 +103,9 @@ p_funDep (before, after) = do
defltEqnToInstDecl :: TyFamDefltEqn GhcPs -> TyFamInstDecl GhcPs
defltEqnToInstDecl FamEqn {..} = TyFamInstDecl {..}
where
eqn = FamEqn {feqn_pats = tyVarsToTypes feqn_pats, ..}
eqn = FamEqn {feqn_pats = map HsValArg (tyVarsToTypes feqn_pats), ..}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is questionable. I don't have enough experience with GHC internals to say whether it's the right thing to do.

src/Ormolu/Utils.hs Outdated Show resolved Hide resolved
typeArgToType = \case
HsValArg tm -> tm
HsTypeArg _ ty -> ty
HsArgPar _ -> notImplemented "HsArgPar"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{-
Note [HsArgPar]
A HsArgPar indicates that everything to the left of this in the argument list is
enclosed in parentheses together with the function itself. It is necessary so
that we can recreate the parenthesis structure in the original source after
typechecking the arguments.

The SrcSpan is the span of the original HsPar

((f arg1) arg2 arg3) results in an input argument list of
[HsValArg arg1, HsArgPar span1, HsValArg arg2, HsValArg arg3, HsArgPar span2]

-}

This is weird. And we probably don't need to handle it because currying?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not know. I'm OK with this as long as everything works in practice.

@neongreen neongreen changed the title [WIP] Migrate to ghc-lib-parser Migrate to ghc-lib-parser Oct 29, 2019
@neongreen neongreen requested a review from mrkkrp October 29, 2019 10:26
@mrkkrp
Copy link
Member

mrkkrp commented Oct 29, 2019

What is the status of this one?

@neongreen
Copy link
Collaborator Author

All tests pass and it's ready to be reviewed.

@neongreen
Copy link
Collaborator Author

I don't know why CI fails, might be a timeout.

@mrkkrp mrkkrp force-pushed the ghc-lib-parser branch 2 times, most recently from d2c3313 to e296e11 Compare November 1, 2019 10:51
default.nix Show resolved Hide resolved
@mrkkrp mrkkrp force-pushed the ghc-lib-parser branch 2 times, most recently from 04e934f to 14bcf80 Compare November 1, 2019 11:55
@@ -43,16 +47,6 @@ p_ruleDecl = \case
p_ruleName :: (SourceText, RuleName) -> R ()
p_ruleName (_, name) = atom $ HsString NoSourceText name

p_ruleBndrs :: [LRuleBndr GhcPs] -> R ()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job at reducing the duplication!

@mrkkrp
Copy link
Member

mrkkrp commented Nov 1, 2019

This is good to go but is blocked on the CI issues.

@mrkkrp mrkkrp force-pushed the ghc-lib-parser branch 2 times, most recently from 0416101 to 994b9b9 Compare November 1, 2019 17:03
This would let us have features from GHC 8.8.1 while still using GHC 8.6.5
from Nixpkgs. Bonus: Ormolu will be compilable with GHCJS.
Copy link
Member

@mrkkrp mrkkrp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@mrkkrp mrkkrp merged commit b08af17 into tweag:master Nov 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use ghc-lib-parser instead of GHC Switch to GHC 8.8.1
2 participants