hexagoxel
/
brittany
1
0
Fork 0
Code Issues 117 Pull Requests 5 Packages Projects Releases 5 Wiki Activity

Add type synonym formatting #189

Merged
ruhatch merged 4 commits from type-synonyms into master 2018-11-04 18:05:30 +01:00
Conversation 15 Commits 4 Files Changed 6
ruhatch commented 2018-10-14 18:17:24 +02:00 (Migrated from github.com)

Add layouter for type synonyms using a similar style to type signatures. It calls layoutType on the right-hand side and handles the left-hand side similiarly to the function name in a Sig. It might be possible to refactor the Sig and SynDecl code to share more structure.

I might have missed some possible cases, so please let me know if there are missing tests!

Add layouter for type synonyms using a similar style to type signatures. It calls `layoutType` on the right-hand side and handles the left-hand side similiarly to the function name in a `Sig`. It might be possible to refactor the `Sig` and `SynDecl` code to share more structure. I might have missed some possible cases, so please let me know if there are missing tests!
ruhatch commented 2018-10-14 18:18:18 +02:00 (Migrated from github.com)

@eborden, @tfausak - would you mind trying this on your codebases to try and spot missing cases/errors?

@eborden, @tfausak - would you mind trying this on your codebases to try and spot missing cases/errors?
tfausak commented 2018-10-15 17:08:59 +02:00 (Migrated from github.com)

We don't have that many type synonyms in our codebase. Looks like only 25. Regardless, formatting everything with this PR changed one file and all the tests still passed. 👍

We don't have that many type synonyms in our codebase. Looks like only 25. Regardless, formatting everything with this PR changed one file and all the tests still passed. 👍
eborden commented 2018-10-15 17:59:19 +02:00 (Migrated from github.com)

We also don't have too many type aliases, but I ran and everything looks fine. It only reformatted two of them and they looked reasonable 🤷‍♂️

We also don't have too many `type` aliases, but I ran and everything looks fine. It only reformatted two of them and they looked reasonable :man_shrugging:
lspitzner commented 2018-10-15 18:03:36 +02:00 (Migrated from github.com)

Some cases:

-- comment move around a bit
type Foo a -- fancy type comment
  = -- strange comment
    Int

-- crashes due to pattern match failure
type (a `Foo` b) c = (a, b, c)

-- Typeoperators already work. I would like a testcase for them.
type (a :+: b) = (a, b)

I can have a look on the comment thingy; those are tricky. Would be nice to have the failure fixed, even though I might not even view it as a blocker - it should be a rare occasion to write things this way.

(oh, and rebase onto latest master please; CI was still borked. master is finally clean.)

Some cases: ~~~~.hs -- comment move around a bit type Foo a -- fancy type comment = -- strange comment Int -- crashes due to pattern match failure type (a `Foo` b) c = (a, b, c) -- Typeoperators already work. I would like a testcase for them. type (a :+: b) = (a, b) ~~~~ I can have a look on the comment thingy; those are tricky. Would be nice to have the failure fixed, even though I might not even view it as a blocker - it should be a rare occasion to write things this way. (oh, and rebase onto latest master please; CI was still borked. master is finally clean.)
👍 1
lspitzner (Migrated from github.com) reviewed 2018-10-15 18:06:55 +02:00
src/Language/Haskell/Brittany/Internal/Layouters/Decl.hs
lspitzner (Migrated from github.com) commented 2018-10-15 18:06:55 +02:00

I think IdP GhcPs ~ GhcPs ~ RdrName, so no #conditional necessary? Or am I messing this up?

I think `IdP GhcPs` ~ `GhcPs` ~ `RdrName`, so no #conditional necessary? Or am I messing this up?
lspitzner (Migrated from github.com) reviewed 2018-10-15 18:10:22 +02:00
src/Language/Haskell/Brittany/Internal/Layouters/Decl.hs
lspitzner (Migrated from github.com) commented 2018-10-15 18:10:22 +02:00

89b02bc4e5/src/Language/Haskell/Brittany/Internal/Layouters/Decl.hs (L632-L636)

https://github.com/lspitzner/brittany/blob/89b02bc4e55cd7b5d384af0e464e365d1ae48d23/src/Language/Haskell/Brittany/Internal/Layouters/Decl.hs#L632-L636
ruhatch (Migrated from github.com) reviewed 2018-10-15 18:48:09 +02:00
src/Language/Haskell/Brittany/Internal/Layouters/Decl.hs
ruhatch (Migrated from github.com) commented 2018-10-15 18:48:09 +02:00

I thought that, but it wouldn't type check on 8.4.3 for some reason 😕

I thought that, but it wouldn't type check on `8.4.3` for some reason :confused:
ruhatch commented 2018-10-15 21:07:26 +02:00 (Migrated from github.com)

I have fixed the pattern matching error, but then the parens are a bit of a rabbit hole:

type (a :-: b) c = (a, b)

type ((a :%: b) c) = (a , c)

type ((a :*: b) c) d = (a , c)

These are stored as AnnOpenPs with relative DPs, so I'm not sure how to go about laying them out properly. A simple solution that produces at least syntactically valid results is to drop all but the parens around the operator.

I have fixed the pattern matching error, but then the parens are a bit of a rabbit hole: ``` type (a :-: b) c = (a, b) type ((a :%: b) c) = (a , c) type ((a :*: b) c) d = (a , c) ``` These are stored as `AnnOpenP`s with relative `DP`s, so I'm not sure how to go about laying them out properly. A simple solution that produces at least syntactically valid results is to drop all but the parens around the operator.
ruhatch commented 2018-10-15 21:36:24 +02:00 (Migrated from github.com)

Also the fixity testing on 8.0.2 is a bit tricky - the first example above prints as

type :-: a b c = (a, b)

because there's no backtick and no fixity declaration to check. Any ideas how to detect that?

Also the fixity testing on `8.0.2` is a bit tricky - the first example above prints as ``` type :-: a b c = (a, b) ``` because there's no backtick and no fixity declaration to check. Any ideas how to detect that?
lspitzner (Migrated from github.com) reviewed 2018-10-16 17:13:46 +02:00
src/Language/Haskell/Brittany/Internal/Layouters/Decl.hs
lspitzner (Migrated from github.com) commented 2018-10-16 17:13:46 +02:00

Ah, indeed. I think we can put

type family IdP p
type instance IdP GhcPs = RdrName

to the <=8.2 conditional in Prelude.hs and avoid this instance of mid-signature conditional. I have tested this on all but 8.6.

(And I saw you cleaned up the conditionals a bit. Good work. Still not pretty, but I think that is the price for supporting multiple versions.)

Ah, indeed. I think we can put ~~~~.hs type family IdP p type instance IdP GhcPs = RdrName ~~~~ to the <=8.2 conditional in `Prelude.hs` and avoid this instance of mid-signature conditional. I have tested this on all but 8.6. (And I saw you cleaned up the conditionals a bit. Good work. Still not pretty, but I think that is the price for supporting multiple versions.)
lspitzner (Migrated from github.com) reviewed 2018-10-16 17:14:15 +02:00
src/Language/Haskell/Brittany/Internal/Layouters/Decl.hs
lspitzner (Migrated from github.com) commented 2018-10-16 17:14:15 +02:00

i.e. the type signature would be unconditionally Located (IdP GhcPs)

i.e. the type signature would be unconditionally `Located (IdP GhcPs)`
ruhatch (Migrated from github.com) reviewed 2018-10-16 22:56:25 +02:00
src/Language/Haskell/Brittany/Internal/Layouters/Decl.hs
ruhatch (Migrated from github.com) commented 2018-10-16 22:56:25 +02:00

Aha, I'll give that a go

Aha, I'll give that a go
lspitzner commented 2018-10-17 01:05:49 +02:00 (Migrated from github.com)

I have had a bit of a look at the infix-detection and parenthesis stuff. I think it should be possible to hack around this by considering a combination of hasAnnKeyword name AnnBackquote, hasAnnKeyword name AnnOpenP (on the Unqual) and testing on whether the name starts with an ":" (which I believe is the only way for type operators).

As ghc-exactprint demonstrates in its test-suite (in TypeBrackets.hs, just if you are curious) there is enough information to do things properly on ghc-8.0.2, too. But it looks really messy and I cannot make any sense of how it works - the fact that there are negative offsets in the relevant DPs is really confusing to me. (I am no exactprint expert, but in general I can make rough sense of that stuff ..)

As a consequence, I agree with dropping down to "normalize while formatting" behaviour, i.e. to turn: type (((MyType))) = Int into type MyType = Int and type ((a `OtherType` b) c) = Int into type (a `OtherType` b) c = Int, even when brittany usually refrains from any sort of "parenthesis normalization".

Please say if this is getting too complex or if the goal of keeping things consistent over different ghc versions causes too much headache; I can give it a go when I find time too, or we can drop to exactprint for complex cases or even declare that the more-than-two-type-args-while-infix case is so rare that we don't support it (for now).

Looking at the comment thing is still on my to-do list.

I have had a bit of a look at the infix-detection and parenthesis stuff. I think it should be possible to hack around this by considering a combination of `hasAnnKeyword name AnnBackquote`, `hasAnnKeyword name AnnOpenP` (on the `Unqual`) and testing on whether the name starts with an ":" (which I believe is the only way for type operators). As ghc-exactprint demonstrates in its test-suite (in `TypeBrackets.hs`, just if you are curious) there is enough information to do things properly on ghc-8.0.2, too. But it looks really messy and I cannot make any sense of how it works - the fact that there are negative offsets in the relevant `DP`s is really confusing to me. (I am no exactprint expert, but in general I can make rough sense of that stuff ..) As a consequence, I agree with dropping down to "normalize while formatting" behaviour, i.e. to turn: ```type (((MyType))) = Int``` into `type MyType = Int` and ```type ((a `OtherType` b) c) = Int``` into ```type (a `OtherType` b) c = Int```, even when brittany usually refrains from any sort of "parenthesis normalization". Please say if this is getting too complex or if the goal of keeping things consistent over different ghc versions causes too much headache; I can give it a go when I find time too, or we can drop to exactprint for complex cases or even declare that the more-than-two-type-args-while-infix case is so rare that we don't support it (for now). Looking at the comment thing is still on my to-do list.
ruhatch commented 2018-10-17 01:34:06 +02:00 (Migrated from github.com)

Okay, so I got infix stuff working for 8.0.2 by checking the first character of the name. I assume it's an infix operator if it's not an upper-case letter or an opening bracket, which I believe is reasonable.

To complicate things though, the brackets in type (a :+: b) c = (a, b, c) are stored as comments in 8.4.3, so we get missing comment warnings! Is there a way to safely drop them?

I think here the "normalize while formatting" approach seems fine - the extra parens don't seem to make it more readable anyway ;)

Other than that 8.4.3 thing the comments are the only thing left. I also left a couple of other pending tests in place so that someone could come back to this if they want.

Okay, so I got infix stuff working for `8.0.2` by checking the first character of the name. I assume it's an infix operator if it's not an upper-case letter or an opening bracket, which I believe is reasonable. To complicate things though, the brackets in `type (a :+: b) c = (a, b, c)` are stored as comments in `8.4.3`, so we get missing comment warnings! Is there a way to safely drop them? I think here the "normalize while formatting" approach seems fine - the extra parens don't seem to make it more readable anyway ;) Other than that `8.4.3` thing the comments are the only thing left. I also left a couple of other pending tests in place so that someone could come back to this if they want.
ruhatch commented 2018-10-17 23:06:06 +02:00 (Migrated from github.com)

Okay I added another commit that deals with parens on 8.4.3 - it actually works out better than on the other two versions, because the comments are better placed. It required some small changes in layoutBriDocM, so it would be good to look closely at that.

Only thing left is dealing with comments now!

Okay I added another commit that deals with parens on `8.4.3` - it actually works out better than on the other two versions, because the comments are better placed. It required some small changes in `layoutBriDocM`, so it would be good to look closely at that. Only thing left is dealing with comments now!
ruhatch commented 2018-10-19 21:36:45 +02:00 (Migrated from github.com)

I made some progress with comments, but the example you gave above has a problem.

-- comment move around a bit
type Foo a -- fancy type comment
  = -- strange comment
    Int

=>

-- comment move around a bit
type Foo a -- fancy type comment
  =  -- strange comment
    Int

Notice the extra space before -- strange comment. This problem exists in type signatures as well actually. I'm not sure how best to fix this. I tried adding a hasAnyCommentPrior but I'm not sure how to modify the layout once we know the type has prior comments.

I made some progress with comments, but the example you gave above has a problem. ``` -- comment move around a bit type Foo a -- fancy type comment = -- strange comment Int => -- comment move around a bit type Foo a -- fancy type comment = -- strange comment Int ``` Notice the extra space before `-- strange comment`. This problem exists in type signatures as well actually. I'm not sure how best to fix this. I tried adding a `hasAnyCommentPrior` but I'm not sure how to modify the layout once we know the type has prior comments.
ruhatch commented 2018-10-19 21:59:06 +02:00 (Migrated from github.com)

I fixed that up by using appSep instead of an explicit space, so this is actually all good to go!

It performs 'normalisation while formatting' for parens on 8.0.2 and 8.2.2, but everything else seems to work perfectly :D

I fixed that up by using `appSep` instead of an explicit space, so this is actually all good to go! It performs 'normalisation while formatting' for parens on `8.0.2` and `8.2.2`, but everything else seems to work perfectly :D
👍 1
lspitzner (Migrated from github.com) reviewed 2018-10-25 21:25:19 +02:00
src/Language/Haskell/Brittany/Internal/Backend.hs
@ -170,9 +172,13 @@ layoutBriDocM = \case
priors
`forM_` \(ExactPrint.Types.Comment comment _ _, ExactPrint.Types.DP (y, x)) ->
do
lspitzner (Migrated from github.com) commented 2018-10-25 21:25:19 +02:00

What is the reasoning for needing layoutMoveToCommentPosX? In what I have tested, using pure () for ")" as well seemed to work just as well. Am I missing some special-case?

What is the reasoning for needing `layoutMoveToCommentPosX`? In what I have tested, using `pure ()` for ")" as well seemed to work just as well. Am I missing some special-case?
lspitzner commented 2018-10-25 21:45:20 +02:00 (Migrated from github.com)

Sorry for the delay. The fix for comments looks good. I'd just like to confirm that layoutMoveToCommentPosX is indeed necessary as I'd prefer not baking more implicit assumptions into the backend state fields, it is sufficiently confusing as is :p. Can you give feedback on the comment?

The resulting behaviour looks good to me across ghc versions, although I have not tested 8.6 yet. The ghc-8.6 branch should be working as of today, but it has merge conflicts. I plan to merge this PR first, and then rebase the 8.6 branch, so this PR will not need to be 8.6-ready.

Sorry for the delay. The fix for comments looks good. I'd just like to confirm that `layoutMoveToCommentPosX` is indeed necessary as I'd prefer not baking more implicit assumptions into the backend state fields, it is sufficiently confusing as is :p. Can you give feedback on the comment? The resulting behaviour looks good to me across ghc versions, although I have not tested 8.6 yet. The ghc-8.6 branch should be working as of today, but it has merge conflicts. I plan to merge this PR first, and then rebase the 8.6 branch, so this PR will _not_ need to be 8.6-ready.
ruhatch (Migrated from github.com) reviewed 2018-10-26 17:55:11 +02:00
src/Language/Haskell/Brittany/Internal/Backend.hs
@ -170,9 +172,13 @@ layoutBriDocM = \case
priors
`forM_` \(ExactPrint.Types.Comment comment _ _, ExactPrint.Types.DP (y, x)) ->
do
ruhatch (Migrated from github.com) commented 2018-10-26 17:55:11 +02:00

It was for the case where you have:

type ((MyType a) b) = (a, b)

to avoid it layouting as

type ((MyType a ) b ) = (a, b)

You're right though it's hacky, makes the backend more complicated, and assumes the thing before the ")" is a docSeparator. Without it we would need each type variable to know whether the next one has a comment or not. I've thought about alternatives a little, so let me know if you'd like me to do it another way or if this doesn't make sense!

It was for the case where you have: ``` type ((MyType a) b) = (a, b) ``` to avoid it layouting as ``` type ((MyType a ) b ) = (a, b) ``` You're right though it's hacky, makes the backend more complicated, and assumes the thing before the ")" is a `docSeparator`. Without it we would need each type variable to know whether the next one has a comment or not. I've thought about alternatives a little, so let me know if you'd like me to do it another way or if this doesn't make sense!
ruhatch (Migrated from github.com) reviewed 2018-10-26 17:56:07 +02:00
src/Language/Haskell/Brittany/Internal/Backend.hs
@ -170,9 +172,13 @@ layoutBriDocM = \case
priors
`forM_` \(ExactPrint.Types.Comment comment _ _, ExactPrint.Types.DP (y, x)) ->
do
ruhatch (Migrated from github.com) commented 2018-10-26 17:56:07 +02:00

This behaviour is only on 8.4.3 where the annotations are moved into comments.

This behaviour is only on `8.4.3` where the annotations are moved into comments.
lspitzner commented 2018-10-27 19:23:51 +02:00 (Migrated from github.com)

@ruhatch I added one commit that (hopefully) should work just as well as before, and it fixes a case of superfluous spaces being retained on ghc-8.4. Being a bit more selective in using docSeparator also makes the one backend hack unnecessary.

-- e.g.
type ((a :%: b  ) c  ) = (a, c)
-- yielded
type ((a :%: b ) c ) = (a, c)

(there was the x - 1 in the code before, but that was not sufficient if in the input the x was >1.)

Maybe it is time to add testcases for non-fixpoints to the suite, i.e. have (input, expected output) pairs. The bug I describe above cannot currently be tested against, which is unfortunate.

If you have any interesting (non-fixpoint) testcases lying around it would be nice if you could test against the latest commit. Otherwise I'd say this is good to merge.

thanks!

@ruhatch I added one commit that (hopefully) should work just as well as before, and it fixes a case of superfluous spaces being retained on ghc-8.4. Being a bit more selective in using `docSeparator` also makes the one backend hack unnecessary. ~~~~.hs -- e.g. type ((a :%: b ) c ) = (a, c) -- yielded type ((a :%: b ) c ) = (a, c) ~~~~ (there was the `x - 1` in the code before, but that was not sufficient if in the input the `x` was >1.) Maybe it is time to add testcases for non-fixpoints to the suite, i.e. have (input, expected output) pairs. The bug I describe above cannot currently be tested against, which is unfortunate. If you have any interesting (non-fixpoint) testcases lying around it would be nice if you could test against the latest commit. Otherwise I'd say this is good to merge. thanks!
ruhatch commented 2018-10-31 14:53:25 +01:00 (Migrated from github.com)

@lspitzner that all looks good to me! Thank you!

@lspitzner that all looks good to me! Thank you!
lspitzner commented 2018-11-04 18:06:48 +01:00 (Migrated from github.com)

thanks!

thanks!
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
blocked: dependency
   
blocked: info-needed
   
bug
   
duplicate
   
enhancement
   
fixed in HEAD
   
help wanted
   
hs:arrows
   
hs:brackets
   
hs:classes
   
hs:comments
   
hs:do-notation
   
hs:guards
   
hs:lists
   
hs:operators
   
hs:patterns
   
hs:records
   
hs:types
   
invalid
   
language extension support
   
layouting
   
needs confirmation
   
priority: high
   
priority: low
   
question
   
revisit before next release
   
wontfix
No Label
blocked: dependency
blocked: info-needed
bug
duplicate
enhancement
fixed in HEAD
help wanted
hs:arrows
hs:brackets
hs:classes
hs:comments
hs:do-notation
hs:guards
hs:lists
hs:operators
hs:patterns
hs:records
hs:types
invalid
language extension support
layouting
needs confirmation
priority: high
priority: low
question
revisit before next release
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: hexagoxel/brittany#189
There is no content yet.
Delete Branch "type-synonyms"

Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?