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

[style-guide] Better control of pattern match handlers and line breaks. #650

Open
isaacabraham opened this issue Nov 20, 2018 · 34 comments
Open

Comments

@isaacabraham
Copy link
Contributor

Description

I would like to suggest having an option for pattern matching that could be set to one of four ways:

  1. Current behaviour: Whatever this is exactly ;-)
  2. Prefer newlines: Always have the clause handling on a new line and indented.
  3. Prefer same line: If the pattern match handler is small enough, put it on the same line as the handler.
  4. Consistent mode: Prefer same line, but only if all matches can fit on the same line. Otherwise, prefer newlines for all cases (i.e. fallback to option 2).

I'm not sure if this ideas are feasible or not - just thinking out aloud at a number of different styles we see in the wild (my personal preference is 4).

@isaacabraham
Copy link
Contributor Author

Just noticed some overlap with fsprojects/fantomas#283

@nojaf
Copy link
Contributor

nojaf commented Nov 21, 2018

So to give some context when a match clause combines multiple paths like in

let rec multiline synExpr = 
    match synExpr with
    | ConstExpr _
    | NullExpr
    | OptVar _
    | SequentialSimple _ ->
        false

The AST looks like

Clause
              (Or
                 (Or
                    (Or
                       (Or
                          (Or
                             (Or
                                (Or
                                   (LongIdent
                                      (LongIdentWithDots ([ObjExpr],[]),None,
                                       None,
                                       Pats
                                         [Wild
                                            tmp.fsx (9,14--9,15) IsSynthetic=false],
                                       None,
                                       tmp.fsx (9,6--9,15) IsSynthetic=false),
                                    LongIdent
                                      (LongIdentWithDots ([While],[]),None,None,
                                       Pats
                                         [Wild
                                            tmp.fsx (10,12--10,13) IsSynthetic=false],
                                       None,
                                       tmp.fsx (10,6--10,13) IsSynthetic=false),
                                    tmp.fsx (9,6--10,13) IsSynthetic=false),
                                 LongIdent
                                   (LongIdentWithDots ([For],[]),None,None,
                                    Pats
                                      [Wild
                                         tmp.fsx (11,10--11,11) IsSynthetic=false],
                                    None,tmp.fsx (11,6--11,11) IsSynthetic=false),
                                 tmp.fsx (9,6--11,11) IsSynthetic=false),
                              LongIdent
                                (LongIdentWithDots ([ForEach],[]),None,None,
                                 Pats
                                   [Wild
                                      tmp.fsx (12,14--12,15) IsSynthetic=false],
                                 None,tmp.fsx (12,6--12,15) IsSynthetic=false),
                              tmp.fsx (9,6--12,15) IsSynthetic=false),
                           LongIdent
                             (LongIdentWithDots ([TryWith],[]),None,None,
                              Pats
                                [Wild tmp.fsx (13,14--13,15) IsSynthetic=false],
                              None,tmp.fsx (13,6--13,15) IsSynthetic=false),
                           tmp.fsx (9,6--13,15) IsSynthetic=false),
                        LongIdent
                          (LongIdentWithDots ([TryFinally],[]),None,None,
                           Pats [Wild tmp.fsx (14,17--14,18) IsSynthetic=false],
                           None,tmp.fsx (14,6--14,18) IsSynthetic=false),
                        tmp.fsx (9,6--14,18) IsSynthetic=false),
                     LongIdent
                       (LongIdentWithDots ([Sequentials],[]),None,None,
                        Pats [Wild tmp.fsx (15,18--15,19) IsSynthetic=false],
                        None,tmp.fsx (15,6--15,19) IsSynthetic=false),
                     tmp.fsx (9,6--15,19) IsSynthetic=false),
                  LongIdent
                    (LongIdentWithDots ([IfThenElse],[]),None,None,
                     Pats [Wild tmp.fsx (16,17--16,18) IsSynthetic=false],None,
                     tmp.fsx (16,6--16,18) IsSynthetic=false),
                  tmp.fsx (9,6--16,18) IsSynthetic=false),None,
               Const (Bool true,tmp.fsx (17,8--17,12) IsSynthetic=false),
               tmp.fsx (9,6--16,18) IsSynthetic=false,SequencePointAtTarget);

And the SynPat Or is treated as a single expression when printed in CodePrinter.
So it will try and put it all on one line if possible.

I would prefer to go with option 2, always use a newline.
Pinging @jindraivanek , @7sharp9 , @vasily-kirichenko , @AnthonyLloyd

@jindraivanek
Copy link

Fixed by fsprojects/fantomas#391.

@isaacabraham
Copy link
Contributor Author

@jindraivanek is the handler itself now always on a newline? That was ironically what I was hoping for (not so much the patterns themselves!).

@jindraivanek
Copy link

@isaacabraham It is, at least when used after let =. It is side effect of pattern match expression being multiline now.

@nojaf
Copy link
Contributor

nojaf commented Jan 22, 2019

I think we might have misinterpreted @isaacabraham question.
Maybe we closed this too soon.

If you have something like:

let foo x =
    match x with
    | 8 -> "8"
    | 9 -> "9"
    | 9001 -> "It's over nine thousand!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"
    | _ -> "dont care"

and you formatted with latest code of master you get:

let foo x =
    match x with
    | 8 -> "8"
    | 9 -> "9"
    | 9001 ->
        "It's over nine thousand!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"
    | _ -> "dont care"

Only the handler for 9001 is on a newline. I think was Isaac was hoping for is that all handlers were on the next line?

Like

let foo x =
    match x with
    | 8 -> 
        "8"
    | 9 -> 
        "9"
    | 9001 ->
        "It's over nine thousand!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"
    | _ -> 
        "dont care"

@isaacabraham
Copy link
Contributor Author

@nojaf exactly right. I wasn't ever talking about the match clauses but the handlers of those clauses.

So if I expand upon my original issue, options 2-4 might be as follows:

Given this code:

let foo x =
    match x with
    | 8 -> "8"
    | 9 -> "9"
    | 9001 -> "foo"
    | _ -> "dont care"

Option 2

Always put on a new line:

let foo x =
    match x with
    | 8 -> 
        "8"
    | 9 ->
        "9"
    | 9001 ->
        "foo"
    | _ ->
        "dont care"

Option 3

As per the starting code above.

Option 4

This would do EITHER option 2 or 3, depending on all the handlers: if at least one handler is "forced" onto a new line, then all of them are (so option 2). Otherwise, they all stay on the same line as the clause (option 3)

@nojaf
Copy link
Contributor

nojaf commented Jan 25, 2019

Ok, maybe option 4 could be a thing. We should investigate if it possible to determine.

@nojaf nojaf reopened this Jan 25, 2019
@isaacabraham
Copy link
Contributor Author

Thanks a lot - and sorry I didn't explain myself properly at the start. Code samples always help!

@Micha-kun
Copy link

Any news on this?

@nojaf
Copy link
Contributor

nojaf commented Jun 18, 2019

I'm afraid not, all effort is going to fsprojects/fantomas#434 atm.

@nojaf
Copy link
Contributor

nojaf commented Aug 30, 2019

@Micha-kun the new setting in fsprojects/fantomas#449 might help a bit in this scenario.

@isaacabraham
Copy link
Contributor Author

Not trying to bump this, but do you think that there's still interest in getting something for this done at some point in the future? Either:

  1. All match handlers go on a new line.
  2. All match handlers are on the same line OR on a new line if one rolls over (IOW - never mix same-line and new-line matches)?

@nojaf
Copy link
Contributor

nojaf commented May 23, 2021

Hi, thanks for the ping here.
I believe this is still worth pursuing.
These days, there is a bit of a process when it comes to stylistic requests.
I'd like to see some guidance first in the MS F# style guide.
The guide now mentions what to do for an individual case but doesn't really take a stance when there is a mix of short/long.

At the bottom of the style guide, there is a button to initiate the conversation:
image
Once there is a merged outcome there, we can just follow the guide and change the behaviour in Fantomas.

I'm well aware this is might seem like a lot of hoops to jump through but so far this process has worked out quite nicely.
If there is a good heuristic, the whole community can benefit from this.

@nojaf
Copy link
Contributor

nojaf commented Nov 9, 2021

@dsyme

@dsyme
Copy link
Contributor

dsyme commented Nov 9, 2021

Cool, I batch these up :)

@nojaf nojaf transferred this issue from fsprojects/fantomas Dec 2, 2021
@nojaf nojaf changed the title Better control of pattern match handlers and line breaks. [style-guide] Better control of pattern match handlers and line breaks. Dec 2, 2021
@dsyme
Copy link
Contributor

dsyme commented May 26, 2022

I'm running through the style guide questions in this repo at last.

I agree with @isaacabraham that at option (4) listed above should be an option.

I personally would be in favour of updating the style guide to make this the default option. (If @nojaf vetos and prefers the status quo as the default option, that would be ok, in that case I would think that option (4) should be a fantomas option, and listed in the style guide as a valid chocie )

@nojaf if it's ok I will propose a PR to the style-guide.

@nojaf
Copy link
Contributor

nojaf commented May 30, 2022

The main issue with this persistence mode is that you need to potentially format each clause to see if it is multiline or not. This feels expensive to check, although maybe it is not that bad.
@dsyme I'm ok with having this thought I'd like to veto against the setting idea.
If we believe in this, it should just be the default.

@dsyme
Copy link
Contributor

dsyme commented May 31, 2022

The main issue with this persistence mode is that you need to potentially format each clause to see if it is multiline or not. This feels expensive to check, although maybe it is not that bad.

Yes, it could cause problems. Maybe try and see

@dsyme I'm ok with having this thought I'd like to veto against the setting idea. If we believe in this, it should just be the default.

I suppose so, though I do see a lot of code that uses a mix. People might be annoyed. Feels like I'll want to trial it to see just how bad the damage is.

@T-Gro
Copy link
Contributor

T-Gro commented May 6, 2024

Docs PR https://github.com/dotnet/docs/pull/40762/files#diff-5583236f005ad44753c6945d4312c09cbd439fb251f9a01db543048fe51d284bR1095 makes the suggestion an official and recommended default.

A particular aspect I did not see in this thread is what this change causes to diffs :
When an existing pattern match with many cases receives a new change causing an overflow at a single line, and is then auto-formatted.

If the proposed change becomes the default covered by Fantomas and not just an option, diffs might become artificially larger and true intent less readable.

@nojaf
Copy link
Contributor

nojaf commented May 6, 2024

Yes, that is a fair take. If this is the default, huge diffs will appear the moment users will upgrade.

@Lanayx
Copy link
Contributor

Lanayx commented May 6, 2024

I don't think huge diffs are something that should prevent us fixing the situation - fantomas itself is not THAT popular and without it there will be no huge diffs (adding fantomas to a project is itself a reason of huge diffs). At least for me the current behavior is a a show stopper for introducing fantomas to my work projects. It's true that there was a chance to fix it 5 years ago, but I suppose it's better to do it now than never. If there is a problem with fantomas performance or necessity of avoiding diffs - it should just stop trying to reformat line breaks in match expressions at all to comply with the desired behavior.

@nojaf
Copy link
Contributor

nojaf commented May 6, 2024

I disagree with a lot of what you just said. Anyway, at this point we need a prototype to continue the conversation in any meaningful way.

@Lanayx
Copy link
Contributor

Lanayx commented May 6, 2024

@nojaf Do you mean prototype in fantomas? It looks like conversation is going into direction "if it's possible to implement it in fantomas, then let style guide go otherwise drop it", I'd like to have a confirmation from @dsyme on it. For me it should work in the opposite direction e.g. first agree on style guide and then decide on implementation, since it should always be possible to just discard existing formatting logic for match cases that moves next short line inline (in case of performance issue or large diff issues) without actually forcing reformat (e.g. let user choose how he likes it).

@nojaf
Copy link
Contributor

nojaf commented May 6, 2024

Do you mean prototype in fantomas?

Assessing the potential impact is challenging. Typically, we start with guidance and then proceed with implementation in Fantomas. However, in this instance, seeing the actual results will be the true test.

To address performance concerns, a prototype can provide clarity. Additionally, the existing community can provide valuable insight into whether enabling this feature by default is advisable. My sentiments align with Don's recent response.

@Lanayx
Copy link
Contributor

Lanayx commented May 6, 2024

I see, however I don't want to invest time working on implementation without being sure it will be accepted (and it looks like nobody wanted either within 5 years). So if everyone agrees that the new default should be option 4 from initial suggestion, then it will be 2 ways of implementing this - to enforce new reformatting (that will bring huge diffs and possibly some perf issues), or to disable forced reformatting and let user follow recommended default himself. If any of those 2 options is guaranteed to be accepted, I can work on that.

@isaacabraham
Copy link
Contributor Author

Assuming the concern raised by @T-Gro about diffs isn't that bad (I accept that it will have an impact on diffs but we already have that e.g. adding a member to a record), I would even go as far as making option 4 the default (assuming the prototype is successful) and have an opt-out setting (much like we have with record formatting).

Just an idea...

@Thorium
Copy link

Thorium commented May 7, 2024

The AST looks like

Would it be beneficial for the compiler build the AST as balanced binary-tree instead? For faster execution and avoid stack increase? So ((a or b) or (c or d)) instead of ((((a or b) or c) or d) ?

This is an example for LINQ ASTs:
https://github.com/Thorium/Linq.Expression.Optimizer/blob/edbda892f91ba8872f028b8418a318446b9554f3/src/Code/ExpressionOptimizer.fs#L278

@nojaf
Copy link
Contributor

nojaf commented May 7, 2024

The AST looks like

Would it be beneficial for the compiler build the AST as balanced binary-tree instead? For faster execution and avoid stack increase? So ((a or b) or (c or d)) instead of ((((a or b) or c) or d) ?

We use a different tree these days, so I wouldn't say the problem still lies there.

Assuming the concern raised by @T-Gro about diffs isn't that bad

That really is an assumption and I want to highlight that this might be unsettling for folks.
Consider this example, the initial code:

match 0 with
| 1 -> true
| 2 -> true
| 3 -> true
| 4 -> true
| 5 -> true
| 6 -> true
| 7 -> true
| 8 -> true
| 9 -> true
| 10 -> true
| 11 -> true
| 12 -> true
| 13 -> true
| 14 -> true
| 15 -> true
| 16 -> true
| 17 -> true
| 18 -> true
| 19 -> true
| 20 -> true
| _ -> false

After changing the last clause:

| _ ->
    // great comment
    false

it leads to

image

I believe I would want to opt out of this.

I see, however, I don't want to invest time working on implementation without being sure it will be accepted

Let's introduce an experimental flag for this feature. The latest git diff doesn't instill enough confidence to make it the default formatting option.

@T-Gro
Copy link
Contributor

T-Gro commented May 7, 2024

Thanks @nojaf for visually demonstrating my concern, this is exactly it.
The compiler codebase has many matches which go well above 20 cases, so this mimics a real world situation we might be facing regularly.

@Lanayx:
Even without Fantomas - we should assume that if a suggestion becomes a default in the style guide, some people will live by it.
Be it by manual control, nitpicking colleagues or automated tools - the diff would be equally bad when created manually by someone who respects the style guide.

Fantomas just happens to be the most convenient method to meet the style guide, and also the most convenient to asses changes in larger bodies of code.

@dawedawe
Copy link
Contributor

dawedawe commented May 7, 2024

I don't see enough on the pro side of this thing to make it the new default.
After so many years, new defaults need a stronger case from my point of view.

I'd be okay with having it in Fantomas behind a config flag as long as the people pushing for it are also willing to help maintaining it.

@Lanayx
Copy link
Contributor

Lanayx commented May 7, 2024

@T-Gro As soon as we are looking at diffs as a concern - this could only be an issue in large legacy F# code bases that rarely change where reformatting will destroy valuable blame information. So normally in such code bases nobody would run reformatting on the whole code with new fantomas version or new style guide version (although there could be exceptions), which means that new default won't really affect such solutions. Style guide really affects just the new/small code bases where it will bring only benefits (from my POV), hence I wanted this change to be made. I was also taking into account that F# user/code bases are very limited in number overall compared to other languages, which could really be a driver for large number of useful changes (even breaking changes), but it isn't.

Still, where I was wrong it's thinking that everyone would support this new default as more consistent (which is what style guide is for - for consistency). It looks like most of commenters don't really see current situation as a bug that has to be fixed, so I'll close my PR and end participation here.

@isaacabraham
Copy link
Contributor Author

The sample PR by @nojaf illustrates the valid concerns, which I accept. I would suggest that this example is somewhat contrived as a "worst case" - as in, it's highly unlikely in my experience that you'll have 19 single-line matches and one multiline - but yes, it's possible.

I would love for @Lanayx to continue his work here, even if it was an "opt-in" flag to start with - I would definitely adopt it here, and we could run it on multiple existing codebases to see the impact.

FWIW I still believe that it will improve readability through the consistent approach i.e. people can visually see either for an entire pattern match expression:

pattern -> handler
pattern -> handler
pattern -> handler

or

pattern ->
    handler
pattern ->
    handler
pattern ->
    handler

@Thorium
Copy link

Thorium commented May 7, 2024

Generally I'd prefer the "arrow" -> to point your code and not out-of-screen.

And also I tend to run out of vertical screen space rather than horizontal, so I prefer initially the same line.

However, the downside is that when you have to modify your code, you have to spend extra seconds with tab-key. But "always multi-line" would be premature optimisation.

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

9 participants