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

Return attribute deleted on reformatting #800

Closed
sadgit opened this issue May 1, 2020 · 5 comments
Closed

Return attribute deleted on reformatting #800

sadgit opened this issue May 1, 2020 · 5 comments
Labels
good first issue Long hanging fruit: easy issue to get your feet wet!

Comments

@sadgit
Copy link

sadgit commented May 1, 2020

Issue created from fantomas-online

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
Copy link
Contributor

nojaf commented May 8, 2020

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.

@nojaf nojaf added the good first issue Long hanging fruit: easy issue to get your feet wet! label May 8, 2020
@kontomondo
Copy link
Contributor

kontomondo commented May 16, 2020

@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
Copy link
Contributor

nojaf commented May 16, 2020

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
Copy link
Author

sadgit commented May 16, 2020 via email

@kontomondo
Copy link
Contributor

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

@nojaf nojaf closed this as completed May 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Long hanging fruit: easy issue to get your feet wet!
Projects
None yet
Development

No branches or pull requests

3 participants