Fix issues with multiline generic type parameters #2852

josh-degraw · 2023-04-19T15:37:57Z

Closes #2706, from Amplifying F# session

nojaf · 2023-04-19T15:45:06Z

Thanks for following up on this. I need to play with this some more and we may want to have the conversation over at the style guide as well.
Once all is settled, we should target 6.1 for this.

nojaf

Hi @josh-degraw, many thanks again for taking the lead in this during https://amplifying-fsharp.github.io/sessions/2023/04/14/

I think the change is sensible but this does change the style and so I would like to see this covered by https://learn.microsoft.com/en-us/dotnet/fsharp/style-guide/formatting#formatting-explicit-generic-type-arguments-and-constraints

And also to have a link for people who will open an issue claiming the additional space is a bug. (Trust me, it will be reported 🙈).

Could you open an issue in https://github.com/fsharp/fslang-design/issues so we can further discuss these implications?

And please also retarget this branch to v6.1.

Thanks!

nojaf · 2023-05-02T14:00:04Z

src/Fantomas.Core.Tests/DallasTests.fs

-      (Position -> option<string>) *
-      FSharp.Compiler.Syntax.ParsedInput> =
+    : cmap<
+          DeclName,


This indent no longer is a multitude of the indent_size, we should look into why this is happening. Gut feeling this is related due to

fantomas/src/Fantomas.Core/CodePrinter.fs

Line 2887 in 4d8d328

+> atCurrentColumnIndent (genType rt.Type)

and not the changes here.
Nonetheless, we should investigate this one.

Do you still consider this a problem or are we okay with this indent level?

Hmm, I think I can live with this.
This sample screams "use type alias" anyway.

nojaf · 2023-05-02T14:05:59Z

src/Fantomas.Core/CodePrinter.fs

@@ -3037,7 +3045,8 @@ let genType (t: Type) =
        +> optSingle genIdentListNodeWithDot node.PostIdentifier
        +> genSingleTextNode node.LessThen
        +> addExtraSpace
-        +> col sepComma node.Arguments genType
+        +> leadingExpressionIsMultiline (colGenericTypeParameters node.Arguments) (fun isMultiline ->


We should add some tests where the < and > contain trivia.
Cases like:

type X = Teq< // int // >

won't be covered right now.

Also please add a code comment on why we are doing this in the first place.

src/Fantomas.Core/CodePrinter.fs

nojaf · 2023-05-02T14:12:30Z

src/Fantomas.Core.Tests/TypeAnnotationTests.fs

+"""
+
+[<Test>]
+let `` Aligned bracket style in anonymous record is respected for multiple types, #2706`` () =


We should have the same test for the other styles.

src/Fantomas.Core.Tests/TypeAnnotationTests.fs

josh-degraw · 2023-11-11T18:25:45Z

Sorry it's taken me so long to finally get back around to trying to finish this up haha.

I'm not sure if something has changed on the parser level since I originally worked on this, but a test that previously succeeded is now failing and I'm having trouble figuring out the best way to address it.

I extracted the relevant bits into its own test but essentially this kind of expression (which is how my PR originally formatted this in the updated tests here):

let bv =
    unbox<
        Foo<
            'innerContextLongLongLong,
            'bb -> 'b
            >
    >
        bf

Is no longer an idempotent operation. It seems like if I increase the indent of the final > when it's output in genExpr under Expr.TypeApp so it looks like this:

let bv =
    unbox<
        Foo<
            'innerContextLongLongLong,
            'bb -> 'b
            >
     >
        bf

it becomes idempotent again, but since this worked before I rebased this PR I'm not sure if this is a bug in the AST parsing layer or just in my code here.

nojaf · 2023-11-11T20:00:11Z

@josh-degraw thanks for picking this up again! Much appreciated!

I'm afraid this one kinda sucks:

The unbox<...> bf no longer is a function application after formatting.

So, instead of a b it becomes a ; b conceptually.

I'm not sure what we need to do to keep this as a function application.

josh-degraw · 2023-11-11T20:04:36Z

@josh-degraw thanks for picking this up again! Much appreciated!

I'm afraid this one kinda sucks:

The unbox<...> bf no longer is a function application after formatting.

So, instead of a b it becomes a ; b conceptually.

I'm not sure what we need to do to keep this as a function application.

Yeah like I mentioned it seems like just increasing the indent for the closing angle bracket at least fixes the parser issue, but that's still valid code with the additional indent right? If not I guess we'd have to carve out an exemption for this scenario or wrap in parentheses or something

nojaf · 2023-11-11T20:13:33Z

Ok, consider this example:

foo<
    'bar
        -> int
>
    barry

This looks like it is a SynExpr.App(foo<...>, barry) but actually it is already is two top level SynModuleDecl.Expr.

Adding a space before > makes it into a SynModuleDecl.Expr (SynExpr.App ...).
So, I think our hand is forced here to just move it one space further.

nojaf · 2023-11-11T20:21:46Z

Added a workaround of let startColumn = ctx.Column + 1

josh-degraw · 2023-11-11T20:23:05Z

I tried that but that messes with a bunch of other tests, wouldn't we want to only do this in this specific situation?

nojaf · 2023-11-11T20:28:47Z

Hmm, I think the safest thing to do would be to have it all the time.
We essentially do the same thing with SynType I think.

Digging into the exact case of SynExpr.App(SynExpr.TypeApp, ...) might be quite challenging and not really worth it.

josh-degraw · 2023-11-11T20:29:45Z

Fair enough. Looks like most of the tests that fail with that change were added in this PR anyway haha. I'll update the tests to reflect this

nojaf · 2023-11-11T20:40:39Z

@josh-degraw took a quick peek if it is possible. It seems less intrusive at first glance to remove the space.

This of course can still go wrong in things like Expr.AppLongIdentAndSingleParenArg, Expr.AppSingleParenArg, Expr.AppWithLambda, Expr.NestedIndexWithoutDot, EndsWithDualListApp, EndsWithSingleListApp, ...

We should check what happens in those scenarios. If we need to update all these individually I don't think it is worth it.

josh-degraw · 2023-11-18T21:11:22Z

This of course can still go wrong in things like Expr.AppLongIdentAndSingleParenArg, Expr.AppSingleParenArg, Expr.AppWithLambda, Expr.NestedIndexWithoutDot, EndsWithDualListApp, EndsWithSingleListApp, ...

Can you maybe help me know in what situations these nodes would apply here? I'm not quite sure exactly how to test for these just going off of the node names themselves (not quite there yet haha..)

nojaf · 2023-11-20T11:26:54Z

Can you maybe help me know in what situations these nodes would apply here?

Sure thing.

SynExpr.App has historically been a tricky one to format. Because it really depends on what expressions it has before we decide how we can format it.

Some examples:

// ExprAppSingleParenArgNode
Foo(
    //
    bar
)

// ExprAppWithLambdaNode
bar(fun x -> x)

// ExprAppNode
a [ Href "some-link"] [
    str "content"
]

// ExprInfixAppNode 
x - y

// ExprSameInfixAppsNode
x * y * z

All of these expressions are SynExpr.App, but we transform them into different Oak shapes in ASTTransformer.

This is sometimes very convenient but at the same has its drawbacks.

A clear pro: sometimes you really need to deal with these shapes very carefully and it helps to have separation.

A clear con: the is no single entry genApp that deals with all types of applications. It can be a bit scattered.

A lot of what we have today is somewhat historical as well. Infix applications are a good example of that. Almost all of the code pre-dates the style guide and it is just what we have today.

We split the applications into different Oak nodes in:

fantomas/src/Fantomas.Core/ASTTransformer.fs

Lines 1184 to 1315 in 21bae08

    
           | SynExpr.App(_, 
        
                         false, 
        
                         SynExpr.LongIdent(_, 
        
                                           SynLongIdent([ ident ], [], [ Some(IdentTrivia.OriginalNotation operatorName) ]), 
        
                                           _, 
        
                                           _), 
        
                         e2, 
        
                         _) when 
        
               PrettyNaming.IsValidPrefixOperatorDefinitionName( 
        
                   PrettyNaming.ConvertValLogicalNameToDisplayNameCore ident.idText 
        
               ) 
        
               -> 
        
               ExprPrefixAppNode(stn operatorName ident.idRange, mkExpr creationAide e2, exprRange) 
        
               |> Expr.PrefixApp 
        
           | NewlineInfixApps(head, xs) 
        
           | MultipleConsInfixApps(head, xs) 
        
           | SameInfixApps(head, xs) -> 
        
               let rest = xs |> List.map (fun (operator, e) -> operator, mkExpr creationAide e) 
        
               ExprSameInfixAppsNode(mkExpr creationAide head, rest, exprRange) 
        
               |> Expr.SameInfixApps 
        
           | InfixApp(e1, operator, e2) -> 
        
               ExprInfixAppNode(mkExpr creationAide e1, operator, mkExpr creationAide e2, exprRange) 
        
               |> Expr.InfixApp 
        
           | IndexWithoutDot(identifierExpr, indexExpr) -> 
        
               ExprIndexWithoutDotNode(mkExpr creationAide identifierExpr, mkExpr creationAide indexExpr, exprRange) 
        
               |> Expr.IndexWithoutDot 
        
           | ChainExpr links -> 
        
               let chainLinks = 
        
                   links 
        
                   |> List.map (function 
        
                       | LinkExpr.Identifier identifierExpr -> mkExpr creationAide identifierExpr |> ChainLink.Identifier 
        
                       | LinkExpr.Dot mDot -> stn "." mDot |> ChainLink.Dot 
        
                       | LinkExpr.Expr e -> mkExpr creationAide e |> ChainLink.Expr 
        
                       | LinkExpr.AppUnit(f, mUnit) -> 
        
                           LinkSingleAppUnit(mkExpr creationAide f, mkUnit mUnit, unionRanges f.Range mUnit) 
        
                           |> ChainLink.AppUnit 
        
                       | LinkExpr.AppParen(f, lpr, e, rpr, pr) -> 
        
                           LinkSingleAppParen( 
        
                               mkExpr creationAide f, 
        
                               mkParenExpr creationAide lpr e rpr pr, 
        
                               unionRanges f.Range pr 
        
                           ) 
        
                           |> ChainLink.AppParen 
        
                       | LinkExpr.IndexExpr e -> mkExpr creationAide e |> ChainLink.IndexExpr 
        
                       | link -> failwithf "cannot map %A" link) 
        
               ExprChain(chainLinks, exprRange) |> Expr.Chain 
        
           | AppSingleParenArg(SynExpr.LongIdent(longDotId = longDotId), px) -> 
        
               ExprAppLongIdentAndSingleParenArgNode(mkSynLongIdent longDotId, mkExpr creationAide px, exprRange) 
        
               |> Expr.AppLongIdentAndSingleParenArg 
        
           | AppSingleParenArg(e, px) -> 
        
               ExprAppSingleParenArgNode(mkExpr creationAide e, mkExpr creationAide px, exprRange) 
        
               |> Expr.AppSingleParenArg 
        
           | SynExpr.App(funcExpr = App(fe, args); argExpr = ParenLambda(lpr, pats, mArrow, body, mLambda, rpr)) -> 
        
               let lambdaNode = mkLambda creationAide pats mArrow body mLambda 
        
               ExprAppWithLambdaNode( 
        
                   mkExpr creationAide fe, 
        
                   List.map (mkExpr creationAide) args, 
        
                   stn "(" lpr, 
        
                   Choice1Of2 lambdaNode, 
        
                   stn ")" rpr, 
        
                   exprRange 
        
               ) 
        
               |> Expr.AppWithLambda 
        
           | SynExpr.App(funcExpr = fe; argExpr = ParenLambda(lpr, pats, mArrow, body, mLambda, rpr)) -> 
        
               let lambdaNode = mkLambda creationAide pats mArrow body mLambda 
        
               ExprAppWithLambdaNode(mkExpr creationAide fe, [], stn "(" lpr, Choice1Of2 lambdaNode, stn ")" rpr, exprRange) 
        
               |> Expr.AppWithLambda 
        
           | SynExpr.App(funcExpr = App(fe, args); argExpr = ParenMatchLambda(lpr, mFunction, clauses, mMatchLambda, rpr)) -> 
        
               let lambdaNode = mkMatchLambda creationAide mFunction clauses mMatchLambda 
        
               ExprAppWithLambdaNode( 
        
                   mkExpr creationAide fe, 
        
                   List.map (mkExpr creationAide) args, 
        
                   stn "(" lpr, 
        
                   Choice2Of2 lambdaNode, 
        
                   stn ")" rpr, 
        
                   exprRange 
        
               ) 
        
               |> Expr.AppWithLambda 
        
           | SynExpr.App(funcExpr = fe; argExpr = ParenMatchLambda(lpr, mFunction, clauses, mMatchLambda, rpr)) -> 
        
               let lambdaNode = mkMatchLambda creationAide mFunction clauses mMatchLambda 
        
               ExprAppWithLambdaNode(mkExpr creationAide fe, [], stn "(" lpr, Choice2Of2 lambdaNode, stn ")" rpr, exprRange) 
        
               |> Expr.AppWithLambda 
        
           | SynExpr.App(ExprAtomicFlag.NonAtomic, 
        
                         false, 
        
                         SynExpr.App(ExprAtomicFlag.Atomic, 
        
                                     false, 
        
                                     identifierExpr, 
        
                                     SynExpr.ArrayOrListComputed(false, indexExpr, _), 
        
                                     _), 
        
                         argExpr, 
        
                         _) -> 
        
               ExprNestedIndexWithoutDotNode( 
        
                   mkExpr creationAide identifierExpr, 
        
                   mkExpr creationAide indexExpr, 
        
                   mkExpr creationAide argExpr, 
        
                   exprRange 
        
               ) 
        
               |> Expr.NestedIndexWithoutDot 
        
           | SynExpr.App(ExprAtomicFlag.NonAtomic, 
        
                         false, 
        
                         SynExpr.App(ExprAtomicFlag.NonAtomic, 
        
                                     false, 
        
                                     identifierExpr, 
        
                                     (SynExpr.ArrayOrListComputed(isArray = false; expr = indexExpr) as indexArgExpr), 
        
                                     _), 
        
                         argExpr, 
        
                         _) when (RangeHelpers.isAdjacentTo identifierExpr.Range indexArgExpr.Range) -> 
        
               ExprNestedIndexWithoutDotNode( 
        
                   mkExpr creationAide identifierExpr, 
        
                   mkExpr creationAide indexExpr, 
        
                   mkExpr creationAide argExpr, 
        
                   exprRange 
        
               ) 
        
               |> Expr.NestedIndexWithoutDot 
        
           | App(fe, args) -> 
        
               ExprAppNode(mkExpr creationAide fe, List.map (mkExpr creationAide) args, exprRange) 
        
               |> Expr.App

So, to answer your question, that would be the list you need to check.

On a side note, some of these nodes probably shouldn't exist because they are too focused on what we do with them inside CodePrinter. For this reason, you also have EndsWithDualListApp as an active pattern instead of a separate node. It is hard to really draw the line there.

Anyway, you should check what happens if any of these nodes can be used with ExprTypeAppNode and what happens if they do with a specific case.

I hope this helps a little.

nojaf · 2023-11-23T06:59:40Z

src/Fantomas.Core.Tests/TypeAnnotationTests.fs

@@ -266,25 +262,27 @@ let private asJson (arm: IArmResource) =
       >
 """

+let forcedLongDefnConfig =


I'm not a fan of this configuration.
The only thing you are trying to achieve is to spread things over multiple lines, using MaxLineLength = 30. All the other settings are irrelevant and misleading to readers.

Yeah, fully agree, this latest commit was mainly a WIP

nojaf · 2023-11-23T07:00:26Z

src/Fantomas.Core.Tests/TypeAnnotationTests.fs

-    |}>
-"""     
-        config
+let ``Multiline type argument with AppLongIdentAndSingleParenArg`` () =


Nit: lowercase unit test names.

nojaf · 2023-11-27T07:34:39Z

src/Fantomas.Core.Tests/TypeAnnotationTests.fs

 [<Test>]
 let ``type application including nested multiline function type`` () =
-    forcedLongDefnConfig
+    { config with


Nitpick but this doesn't comply with the pull request ground rules.

I don't really see a compelling reason to deviate from the standard unit template.
All our tests are the same, I'm not a fan of mixing that up in this PR.

nojaf · 2023-11-27T07:38:04Z

Be aware that in its current form this is not working out for every SynExpr.App combination.
I did a quick test with:

[<Test>]
let ``dual list application`` () =
    formatSourceString
        """
unbox<Foo<'innerContextLongLongLong,'bb -> 'b>> [
    ClassName "meh"
] [
    str "foo"
]
"""
        { config with
            MaxLineLength = 12
            MultilineBracketStyle = Stroustrup }
    |> prepend newline
    |> should
        equal
        """
unbox<
    Foo<
        'innerContextLongLongLong,
        'bb
            -> 'b
     >
> [
    ClassName 
        "meh"
] [
    str 
        "foo"
]
"""

josh-degraw · 2023-11-27T14:37:29Z

Be aware that in its current form this is not working out for every SynExpr.App combination.

Yeah I know, I'm still having a little trouble pinpointing the spot to fix some of these

nojaf · 2023-11-27T14:53:31Z

Don't hesitate to reach out if you wanna pair on this sometime!

josh-degraw · 2023-11-27T17:07:51Z

Don't hesitate to reach out if you wanna pair on this sometime!

That might be helpful. I felt like a few I found right where I thought I needed to make changes to fix some of these issues but it didn't work haha

* Expose TransformAST api. * Add additional unit test. * Revert changelog entry

nojaf

Alright, I think this is worth merging in and release as an alpha.
Thank you for your endurance here @josh-degraw!

josh-degraw self-assigned this Apr 19, 2023

nojaf mentioned this pull request May 2, 2023

Multiline type arguments parse issue dotnet/fsharp#15171

Open

nojaf requested changes May 2, 2023

View reviewed changes

josh-degraw changed the base branch from main to v6.1 May 2, 2023 16:07

josh-degraw force-pushed the aligned-anonymous-arg branch from 4d8d328 to fe504e3 Compare May 2, 2023 16:08

nojaf force-pushed the v6.1 branch from 93ae592 to d870608 Compare June 2, 2023 11:15

nojaf force-pushed the v6.1 branch 3 times, most recently from 9329316 to 87d2a45 Compare June 20, 2023 11:23

josh-degraw changed the base branch from v6.1 to main November 11, 2023 16:48

josh-degraw force-pushed the aligned-anonymous-arg branch from eeb8f27 to 9056cf1 Compare November 11, 2023 18:17

josh-degraw mentioned this pull request Nov 13, 2023

[style-guide]: Multiline generic type parameters bracket alignment fsharp/fslang-design#756

Open

nojaf mentioned this pull request Nov 20, 2023

Remove ExprAppSingleParenArgNode from Oak model. #2994

Closed

nojaf reviewed Nov 23, 2023

View reviewed changes

nojaf reviewed Nov 27, 2023

View reviewed changes

nojaf and others added 12 commits January 2, 2024 09:44

Expose TransformAST api. (fsprojects#2868)

d2c6776

* Expose TransformAST api. * Add additional unit test. * Revert changelog entry

Fix issues with multiline generic type parameters

ee664f5

Refactor a few things per feedback

92ccc38

Updates

9466eea

Rebase and add new test

8c70d94

Add one extra space to ensure expr remains an application.

0fe4174

Only add extra space when in SynExpr.App

0539e98

Rebase and add new test

4f287b7

Add test

1cd2a37

WIP

e2581e7

Update tests

4d9bbd1

Refactor test configs

0209297

josh-degraw force-pushed the aligned-anonymous-arg branch from 66fdc2d to 0209297 Compare January 2, 2024 16:57

josh-degraw and others added 5 commits January 4, 2024 13:34

Fix issue and update newly broken tests

346ca92

Format CodePrinter

51f76cb

Use more typical unit test template.

9f4f46c

Add additional unit tests.

9f00368

Add changelog entry

9cc60a2

nojaf approved these changes Jan 9, 2024

View reviewed changes

nojaf enabled auto-merge (squash) January 9, 2024 17:01

nojaf merged commit 3a10e88 into fsprojects:main Jan 9, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issues with multiline generic type parameters #2852

Fix issues with multiline generic type parameters #2852

josh-degraw commented Apr 19, 2023

nojaf commented Apr 19, 2023

nojaf left a comment

nojaf May 2, 2023

josh-degraw Nov 13, 2023

nojaf Jan 9, 2024

nojaf May 2, 2023

nojaf May 2, 2023

nojaf May 2, 2023

josh-degraw commented Nov 11, 2023

nojaf commented Nov 11, 2023

josh-degraw commented Nov 11, 2023

nojaf commented Nov 11, 2023

nojaf commented Nov 11, 2023

josh-degraw commented Nov 11, 2023

nojaf commented Nov 11, 2023

josh-degraw commented Nov 11, 2023

nojaf commented Nov 11, 2023

josh-degraw commented Nov 18, 2023

nojaf commented Nov 20, 2023

nojaf Nov 23, 2023

josh-degraw Nov 23, 2023

nojaf Nov 23, 2023

nojaf Nov 27, 2023

nojaf commented Nov 27, 2023

josh-degraw commented Nov 27, 2023

nojaf commented Nov 27, 2023

josh-degraw commented Nov 27, 2023

nojaf left a comment

Fix issues with multiline generic type parameters #2852

Fix issues with multiline generic type parameters #2852

Conversation

josh-degraw commented Apr 19, 2023

nojaf commented Apr 19, 2023

nojaf 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

josh-degraw commented Nov 11, 2023

nojaf commented Nov 11, 2023

josh-degraw commented Nov 11, 2023

nojaf commented Nov 11, 2023

nojaf commented Nov 11, 2023

josh-degraw commented Nov 11, 2023

nojaf commented Nov 11, 2023

josh-degraw commented Nov 11, 2023

nojaf commented Nov 11, 2023

josh-degraw commented Nov 18, 2023

nojaf commented Nov 20, 2023

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

nojaf commented Nov 27, 2023

josh-degraw commented Nov 27, 2023

nojaf commented Nov 27, 2023

josh-degraw commented Nov 27, 2023

nojaf left a comment

Choose a reason for hiding this comment