Should not move the starting point of a single-line comment

knocte commented 3 years ago

Code

type CustomCancelSource() =
    interface IDisposable with
        member self.Dispose() =
            try
                self.Cancel()
            with
            | :? ObjectDisposedException ->
                ()
            // TODO: cleanup also subscribed handlers? see https://stackoverflow.com/q/58912910/544947

Result

type CustomCancelSource() =
    interface IDisposable with
        member self.Dispose() =
            try
                self.Cancel()
            with :? ObjectDisposedException -> ()
// TODO: cleanup also subscribed handlers? see https://stackoverflow.com/q/58912910/544947

Problem description

There's no need to move the comment to the most-left column. (Essentially the opposite effect to https://github.com/fsprojects/fantomas/issues/1223 )

Extra information

[ ] The formatted result breaks by code.
[ ] The formatted result gives compiler warnings.
[maybe] I or my company would be willing to help fix this.

Options

Fantomas Master at 11/07/2020 09:02:01 - de55bd935d4b7cc63d2408b520ec8a7f962fa9e2

Default Fantomas configuration

btzo commented 3 years ago

So I analyzed this example using the fantomas tool, the Trivia containing the comment is linked to SynModuleDecl_Types, this causes it to use the same range as the whole declaration type, ignoring the old position.

I see two possible solutions for this particular case:

Make Trivia use the SynExp_TryWith node, or another node closer to the comment, instead of the SynModuleDecl_Types node.
When creating the Trivia with the tokens in the collectTrivia function, add in the range parameter the length of the whitespace token before it, thus maintaining the initial position.

What do you think @nojaf ? For me these two solutions would impact other parts of the code, especially the 2.

nojaf commented 3 years ago

Hello Mike, this is known problem and appears in multiple places. Trying to solve this will impact the entire unit test suite. And that is the tricky part, you really need to nail it in many scenarios.

Option 1 is the only that would work in my opinion. You'll need to do something clever in

https://github.com/fsprojects/fantomas/blob/7b6419a2f441b0ff3efa98dd5b0278576e9f004a/src/Fantomas/Trivia.fs#L327-L339

To select the better trivia node to attach the comment to. In the past I did some attempts myself and well, let's say it is really challenging to get all cases right. Next to that please keep performance in mind, this part is now already one of the slowest parts of Fantomas. Doing more checks here could really have a bad impact.

btzo commented 3 years ago

Hey @nojaf , I'm trying to solve the single-line comments problem based on #1393 discussion.

I added the initial column position of the comment in the LineCommentOnSingleLine type.

You can see all that in this branch.

I take your idea of adding the missing spaces in the comment and print the new string at the right indentation, but I'm having a double indentation, see should keep comment position - replicate of ( attribute, line comment, attribute, new line, record definition field ) test in CommentTests.fs for example.

atIndentLevel true 0 <string> should write <string> from the start of the line without adding any indentation right or I'm missing something?

nojaf commented 3 years ago

Hey Mike, the thing I suggested in #1393 is a solution for that problem, not this one. That solution really talks about adding missing spaces within a BlockComment, it does not touch the subject of adding spaces before (or after) a comment.

This information cannot reliably come from the source code. Example: if the indentation in the source code is 2 spaces and the configuration to format is 4 spaces this idea will never work.

I described how this problem should be solve in my comment above in this thread. Using a solution for another problem doesn't really fit the bill here.

Allow me to again re-elaborate the rational: Imagine the following code:

let foo a =
    someLongFunctionCall parameterOne parameterTwo parameterThree
    // bar

The line comment // bar is found on line 3. Now that comment needs to be assigned to some AST node that will be processed in CodePrinter.

Currently it is assigned as ContentAfter for the SynModuleDecl_Let node. That is why it is formatted on the start of the line 3:

let foo a =
    someLongFunctionCall parameterOne parameterTwo parameterThree
// bar

The better candidate node in this case was SynExpr_App (2,4) (2,65). So the trivia assignment needs a better algorithm to make the correct choice.

fsprojects / fantomas