Implement formatting logic (without comments/linebreak support) by avh4 · Pull Request #117 · gren-lang/compiler

avh4 · 2022-08-27T06:17:58Z

This is the base formatting logic. This is one part of #42. Handling non-documentation comments and linebreak sensitivity are not part of this PR.

Run with gren format from inside a project with a gren.json, or run gren format <files and/or directories...>.

This is based on the learnings from elm-format, but is rewritten and greatly simplified, and also should be more performant.

TODO:

stuff that's already finished when I wrote the todo list
record extension types
port modules and ports
doc comments
binary operator declarations
fix string literal escaping
keep nested function type indentation to one level
correct spacing in exposing listings
shortcut record pattern sugar { f1, f2 } (instead of { f1 = f1, f2 = f2 }

Will be done in a future PR (tracked in #42):

allow kernel module features when formatting

This reverts commit 6931d87.

…e track the newlines from the input file)

…en files

Now it's A -> B -> C -> D instead of A -> B -> C -> D

This primarily affected `exposing` clauses, but also was sometiems visible for record expressions and possibly other places.

robinheghan

Overall, this looks good to me. I have a few comments, but nothing major.

robinheghan · 2022-09-02T08:29:56Z

              then
                let deps = map Src.getImportName imports
-                    local = Details.Local path time deps (any isMain values) lastChange buildID
+                    local = Details.Local path time deps (any isMain (fmap snd values)) lastChange buildID


Any reason you're using fmap here instead of the (isMain . snd) approach you use further down?

robinheghan · 2022-09-02T08:35:35Z

-canonicalize pkg ifaces modul@(Src.Module _ exports docs imports values _ _ binops effects) =
+canonicalize pkg ifaces modul@(Src.Module _ exports docs imports values' _ _ binops effects) =
  do
+    let values = fmap snd values'


I know the prime char isn't considered bad practice in Haskell, but could we call this ordredValues or something to that effect?

I saw you were using the prime convention elsewhere as well. I struggle a bit with reading those functions, as the name doesn't really give me a clue about what's different from the non-prime version.

Consider using full names where possible. Makes it much easier to read and understand the code.

robinheghan · 2022-09-02T08:44:46Z

-    _aliases :: [A.Located Alias],
+    _values :: [(SourceOrder, A.Located Value)],
+    _unions :: [(SourceOrder, A.Located Union)],
+    _aliases :: [(SourceOrder, A.Located Alias)],


Would it be better to use a new type A.SourceOrderWithLocation a instead of using (SourceOrder, A.Located a)? That would get rid of having to do fmap snd everywhere, wouldn't it? 🤔

I think the fmap snd's you're talking about are fmap'ing a List, and have type [(sourceOrder, A.Located a)] -> A.Located a, so having a A.SourceOrderWithLocation a type would still need an fmap someNewFunction with someNewFunction : A.SourceOrderWithLocation a -> A.Located a, or maybe in some places where A.toValue is used later on, we'd have someOtherNewFunction : A.SourceOrderWithLocation a -> a.

So I think the result would just change fmap snd into fmap someNewFunction, which I think does have different trade-offs, but doesn't seem strictly better to me. But if you think having some more named functions would be worth it to avoid using more tuples, then I can do that. What do you think?

Just keep it as is.

A part of me is thinking "there's got to be a way to do this without adding fmaps all over", but I'm sure you're right in your assessment that it would include different trade offs.

The code is easy enough to understand, so just keep it.

maybe the fmap's are a smell that the SourceOrder's are being passed down further than they need to be? I'll keep an eye out for that when I fix the other feedback.

robinheghan · 2022-09-02T09:45:59Z

+spaceOrIndent' :: Bool -> NonEmpty Block -> Block
+spaceOrIndent' forceMultiline = Block.rowOrIndent' forceMultiline (Just Block.space)
+
+{-# INLINE group #-}


Does this make a big difference, performance wise?

Probably not. My estimation is there's maybe a 60% chance this does improve performance at the cost of increased binary size, since this is a large enough function that ghc might not inline it without the hint. But also the improvement is certainly going to be tiny and probably not noticeable.

Would you mind removing the pragmas, then?

Don't mind keeping it if we know for certain that it makes an important difference in performance, but until we know that to be true I'd rather just trust the compiler to do its thing.

robinheghan · 2022-09-02T09:59:45Z

Should've mentioned: this is a real impressive piece of work 👏

avh4 · 2022-09-07T07:50:10Z

Comments addressed.

I think this should be working now, and should preserve the meaning of any input code when formatting. Comments will be lost, and linebreak sensitivity isn't implemented in this PR (with a preference toward splitting things onto multiple lines), so it should be enough for people to play around with while #58 gets worked on.

If you have any preferences about the format itself, I'm happy to hear that here, or address it in a later PR.

robinheghan · 2022-09-09T08:00:18Z

Just tested out this branch by formatting gren-lang/core and gren-lang/browser. There are some files that fail to parse, but it seems parsing works correctly when running gren make. Any idea why those files fail?

The code in this PR looks good to me, just want to know why certain files fail before an eventual merge.

avh4 · 2022-09-09T09:05:45Z

There are some files that fail to parse, but it seems parsing works correctly when running gren make. Any idea why those files fail?

Yeah, that's because the parser allows different things depending on if the project is a package and if it's a special package that can have kernel features. Currently when I parse for formatting, I just tell it it's an application project. I'll make that work better in a future PR. For now, that means that modules that have infix operator definitions, and effect manager modules won't be able to parse.

robinheghan · 2022-09-09T09:06:54Z

Thank you for explaining it.

LGTM.

avh4 added 8 commits May 1, 2022 14:54

Extract Gren.Format

49fd254

Merge remote-tracking branch 'origin/main' into avh4/format

654359a

Gren.Format: output more efficiently using ByteString.Builder

b70419d

Retain declaration order in Source.Module

8fa9e25

Merge remote-tracking branch 'origin/main' into avh4/format

f91dd33

Remove redundant imports

12417b7

Revert "Disable format option."

3a2cdca

This reverts commit 6931d87.

Format hs files

f74081e

This was referenced Aug 27, 2022

Implement code formatting #42

Open

[WIP] Implement formatting logic #57

Closed

WIP: implement formatting

1973aaf

avh4 force-pushed the format branch from 283947d to 1973aaf Compare August 27, 2022 06:21

avh4 added 18 commits August 27, 2022 16:02

format: filter out default imports before rendering

41090e2

WIP: implement formatting: if expressions

e82c5d2

WIP: implement formatting: float literals

f62fc94

WIP: implement formatting: negation unary operator

e37ef7c

WIP: implement formatting: Src.Op

3383a9e

ormolu

5431034

WIP: implement formatting: lambda expressions

7c35ab6

format: keep parens around expressions where needed

ddb3f1a

format: keep parens around types where needed

d64cf6c

format: keep parens around patterns where needed

3ae358e

implement formatting: record patterns

a953dda

Remove outdated comment

5000632

Use multiline formatting for pipeline operators (just for now until w…

bdb7041

…e track the newlines from the input file)

format: put blank lines between definitions in let expressions

d5a91e2

format: handle port module and effect module headers

1849492

format: handle port declarations

bee79a2

format: when recursively finding files in a directory, only match .gr…

5f7ddce

…en files

format: retain module doc comments

24dc6ba

avh4 added 6 commits August 29, 2022 21:42

format: retain doc comments for top-level definitions

68a0a7d

format: handle record extension types

fec41f6

format: don't double-escape string literals

99512fd

format: only indent nested lambda types to one level deep

4e85cfa

Now it's A -> B -> C -> D instead of A -> B -> C -> D

format: Don't insert extra spaces before commas

94bda8a

This primarily affected `exposing` clauses, but also was sometiems visible for record expressions and possibly other places.

format: handle infix definitions

ea0ec5b

robinheghan reviewed Sep 2, 2022

View reviewed changes

avh4 added 6 commits September 6, 2022 23:53

Merge remote-tracking branch 'origin/main' into format

643ec67

format: support record pattern sugar

13f2200

Prefer function composition over additional fmap

87e622b

Avoid use of prime and trailing underscore in variable names

7ff7075

Add note about future plans to extract Text.PrettyPrint.Avh4

e2f4306

Remove unverified INLINE pragmas

dc74a14

avh4 marked this pull request as ready for review September 7, 2022 07:42

avh4 changed the title ~~[WIP] Implement formatting logic~~ Implement formatting logic Sep 7, 2022

avh4 requested a review from robinheghan September 7, 2022 07:50

avh4 changed the title ~~Implement formatting logic~~ Implement formatting logic (without comments support) Sep 7, 2022

avh4 changed the title ~~Implement formatting logic (without comments support)~~ Implement formatting logic (without comments/linebreak support) Sep 7, 2022

robinheghan merged commit 7fd91be into gren-lang:main Sep 9, 2022

avh4 mentioned this pull request Sep 9, 2022

Fix merge conflict with https://github.com/gren-lang/compiler/pull/121 #123

Merged

Uh oh!

Conversation

avh4 commented Aug 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robinheghan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avh4 Sep 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robinheghan commented Sep 2, 2022

Uh oh!

avh4 commented Sep 7, 2022

Uh oh!

robinheghan commented Sep 9, 2022

Uh oh!

avh4 commented Sep 9, 2022

Uh oh!

robinheghan commented Sep 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

avh4 commented Aug 27, 2022 •

edited

Loading

avh4 Sep 3, 2022 •

edited

Loading