Return attribute deleted on reformatting

sadgit commented 4 years ago

Please describe here fantomas problem you encountered

Fantomas has deleted the return attribute - this breaks the code

Code

let function1     x : [<return: MyCustomAttributeThatWorksOnReturns>] int = x + 1

Result

let function1 x: int = x + 1

Options

Fantomas Next - 4.0.0-alpha-001-1/1/1990

Name	Value
`IndentSpaceNum`	`4`
`PageWidth`	`120`
`SemicolonAtEndOfLine`	`false`
`SpaceBeforeParameter`	`true`
`SpaceBeforeLowercaseInvocation`	`true`
`SpaceBeforeUppercaseInvocation`	`false`
`SpaceBeforeClassConstructor`	`false`
`SpaceBeforeMember`	`false`
`SpaceBeforeColon`	`false`
`SpaceAfterComma`	`true`
`SpaceBeforeSemicolon`	`false`
`SpaceAfterSemicolon`	`true`
`IndentOnTryWith`	`false`
`SpaceAroundDelimiter`	`true`
`MaxIfThenElseShortWidth`	`40`
`MaxInfixOperatorExpression`	`50`
`MaxRecordWidth`	`40`
`MaxArrayOrListWidth`	`40`
`MaxLetBindingWidth`	`40`
`MultilineBlockBracketsOnSameColumn`	`false`
`NewlineBetweenTypeDefinitionAndMembers`	`false`
`StrictMode`	`false`

nojaf commented 4 years ago

Hello, this is an AST bug.

The attributes of the return type are stored in SynBinding -> SynValInfo -> SynArgInfo -> SynAttributes.

So the LetBinding active pattern should be extended to also capture the synValData.

And then the attributes can be generated in genLetBinding. I believe the additional attributes should be passed to genPatWithReturnType.

The attributes themselves can be printed with genAttributes.

Could I persuade you to tackle this issue yourself? Check out this video if you are less familiar with AST.

kontomondo commented 4 years ago

@nojaf From what I could tell, the genPatWithReturnType is never called in this case, under getLetBinding, the below code is hit:

       +> leadingExpressionIsMultiline
               (afterLetKeyword +> genPat +> enterNodeTokenByName rangeBetweenBindingPatternAndExpression "EQUALS")
               (genExprSepEqPrependType astContext p e)

and in turn in: genExprSepEqPrependType:

        (addExtraSpaceBeforeGenericType
         +> genCommentBeforeColon
         +> sepColon
         +> genType astContext false t

since this is called from a number of places, I tried introducing an option valInfo in there: and genExprSepEqPrependType astContext (pat:SynPat) (e: SynExpr) (valInfo:SynValInfo option) (isPrefixMultiline: bool) ctx =

and pass it from getLetBinding, then I added:

        let genMetadataAttributes ctx =
            match valInfo with
            | Some(SynValInfo(_, SynArgInfo(attributes, _, _))) -> genAttributes astContext attributes ctx
            | None -> sepNone ctx

and

        (addExtraSpaceBeforeGenericType
         +> genCommentBeforeColon
         +> sepColon
         +> genMetadataAttributes
         +> genType astContext false t

when formatting let f x: [<return: MyCustomAttributeThatWorksOnReturns>] int = x the result is :

let f x: [<``return``:MyCustomAttributeThatWorksOnReturns>]
int = x

note the extra new line and the return in quotes, am I on the right path here ?

nojaf commented 4 years ago

I think you are on the right track but again this is hard to read. It is really encouraged to make a (draft) PR, by correct linking you are letting other people know that you are working on this. As for the extra `` I think I know where those are coming from.

sadgit commented 4 years ago

That looks proimising - now we have to fix the back-tcks on the reserved word return

On Sat, 16 May 2020 at 10:08, Florian Verdonck notifications@github.com wrote:

I think you are on the right track but again this is hard to read. It is really encouraged to make a (draft) PR, by correct linking you are letting other people know that you are working on this. As for the extra `` I think I know where those are coming from.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/fsprojects/fantomas/issues/800#issuecomment-629614074, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFJ5S3GVBH4VCQRTJLFH4TRRZJZJANCNFSM4MXCE4XQ .

kontomondo commented 4 years ago

@nojaf @sadgit this can be closed as the fix has been merged in

fsprojects / fantomas