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

[Feature request] Annotations to ignore sections of code #1704

Open
4 of 6 tasks
amongonz opened this issue May 7, 2021 · 6 comments
Open
4 of 6 tasks

[Feature request] Annotations to ignore sections of code #1704

amongonz opened this issue May 7, 2021 · 6 comments

Comments

@amongonz
Copy link

amongonz commented May 7, 2021

I propose we support annotations in code to ignore certain sections from formatting. This is helpful for code known to trigger soundness bugs in Fantomas or sections that benefit from unusual formatting.

Looking at other formatters for inspiration, I see different annotations we could use for this:

  • Prettier (JS) uses comments like // prettier-ignore to exclude the next AST element from formatting. I think this is clever, although it can take more effort to get right.
  • Rider, IDEA and Eclipse exclude sections between comments // @formatter:off/on.
  • Roslyn excludes sections between #pragma warning disable/restore format. In F# this could be a directive #format disable/restore, but it'd raise a warning at compile time unless a language suggestion was made so the compiler recognises it.
  • A crude alternative could also be #if !FANTOMAS ... #endif.

The existing way Fantomas deals with this problem is by ignoring the entire file with .fantomasignore.

Pros and Cons

The advantage of making this adjustment to Fantomas is having an escape hatch for excluding small sections of code, specially to work around soundness bugs.

The disadvantage of making this adjustment to Fantomas is having to scan comments for annotations and track which nodes should be formatted. It could also interfact badly with existing soundness bugs around comments.

Examples

Using Prettier-like annotations and default settings. Before:

// fantomas-ignore
module Ignored =
    let bar=2
    let baz   x =  [x,2,3]

module Formatted =
    let bar=2
    let baz   x =  [x,2,3]

let ignoredPoint x   y=
    (* fantomas-ignore *){|X=x;    Y=y|}

let point x   y=
    {|X=x;    Y=y|}

After:

// fantomas-ignore
module Ignored =
    let bar=2
    let baz   x =  [x,2,3]

module Formatted =
    let bar = 2
    let baz x = [ x, 2, 3 ]

let ignoredPoint x y =
    (* fantomas-ignore *) {|X=x;    Y=y|}

let point x y = {| X = x; Y = y |}

The formatting of the syntax elements following fantomas-ignore comments has been ignored. Open questions are whether inner let/use are counted as an element or only their binding part and what precedence does it have respect to F# operators.

Using Rider-style annotations and default settings. Before:

let normalizeIgnored key   xs  =
    // @formatter:off
    let firstValue = xs |> Seq.find (fun (y,v) -> y = key) |> snd
    xs |> Seq.map (fun (y,v) -> (y, float v / firstValue) )
    // @formatter:on

let normalize key   xs  = 
    let firstValue = xs |> Seq.find (fun (y,v) -> y = key) |> snd
    xs |> Seq.map (fun (y,v) -> (y, float v / firstValue) )

After:

let normalizeIgnored key xs =
    // @formatter:off
    let firstValue = xs |> Seq.find (fun (y,v) -> y = key) |> snd
    xs |> Seq.map (fun (y,v) -> (y, float v / firstValue) )
    // @formatter:on

let normalize key xs =
    let firstValue =
        xs |> Seq.find (fun (y, v) -> y = key) |> snd

    xs
    |> Seq.map (fun (y, v) -> (y, float v / firstValue))

(Fantomas currently moves // @formatter:on to the first column.) The formatting of nodes between formatter:off and formatter:on comments has been ignored. #format disable/restore would behave similarly.

Extra information

Estimated cost (XS, S, M, L, XL, XXL): M/L, probably depends on which annotation syntax is chosen.

Affidavit (please submit!)

Please tick this by placing a cross in the box:

Please tick all that apply:

  • This is not a breaking change to Fantomas
  • I would be willing to help implement and/or test this
  • This suggestion is part of the Microsoft style guide (please add a link to section if so)
  • This suggestion is part of the G-Research style guide (please add a link to section if so)
@nojaf
Copy link
Contributor

nojaf commented May 8, 2021

Hello Adrián, thank you for showing interest in this project.
In principle, I would be ok with having such a feature.

In practice however, this would haunt me at night at first glance.
Code comments are currently not part of the AST tree.
Fantomas collects comments from the F# tokens and assigns them to existing AST nodes. (See video for some more context)
We have not nailed this part to be honest. Sometimes a comment is not assigned, wrongly assigned or not printed back out when the node is process. There is are still a lot of bugs in that area and you wish propose to build a new feature on top of it.

Next, if the marker part of the process were to be in place, shifting in the original code at the right place might also be challenging. Getting the code from the original file based on an AST range could potentially not work out as you'd expect.
Often the F# ranges are incorrect in the syntax tree or the indentation will be off when inserted into the formatted file.

I hope you understand my skepticism here. You will most likely be able to come up with something that works for a couple of use-cases, but once this gets canon and people start using it a whole new series of issues would be reported. This is purely because you cannot anticipate the spectrum of AST combinations that people use in their code.

Such a large impacting feature request would only fit in the next major release, no time frame is known for that right now.

@amongonz
Copy link
Author

amongonz commented May 8, 2021

Oh, I had no expectation of this being workable anytime soon, I'm aware this is blocked by all the trivia bugs. I even encountered some while preparing the examples for the request. But during discussion related to .fantomasignore I noticed others wish for this feature too, so I decided to make the request official.

@stefan-schweiger
Copy link
Contributor

@nojaf is the Dallas rewrite making it in any way more feasible to achieve this feature?

@nojaf
Copy link
Contributor

nojaf commented Jan 2, 2023

Hmm, I don't think so. Trivia has become a lot more accurate because of Dallas, so the risk of comments being removed has been improved.

The main struggling points I currently see are:

  • The // off and // on comments would need to be attached to the same Node. Otherwise strange things will happen. There are currently numerous "comment-shifted" bugs, and I have yet to encounter someone that really wants to solve those.
  • Something like // don't format next node would be easier to deal with on a technical level. But end-users might be confused with what we consider to be the next node.
  • "ignoring sections" would mean that we need to re-insert certain fragments of the original code. I can see multiple scenarios where the source code is so vastly different that just inserting the old code would no longer lead to valid code. For example, the source code has a different indentation size.

@auduchinok
Copy link
Contributor

auduchinok commented May 16, 2023

Another request: RIDER-93681.

@smoothdeveloper
Copy link
Contributor

This is probably a significant feature (with the reverse one, to only format specific sections) that would drive adoption in places where fantomas is not considered.

It would also make sense, rather than .fantomasignore, to offer the reverse one, anytime an all or nothing decision is taken; "leave it to the user to pick a side" philosophy, it doesn't make the tool less opinionated.

Thanks for having the suggestion open :)

If it would make it easier for implementation of a first version, it could behave in a way that only makes it easy to apply, maybe not to inner chunks of code, but for a whole member; it would emit a warning if it can't handle it, inviting user to put the delimiters at a greater scope.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants