fslang-design
fslang-design copied to clipboard
fsharp
[style-guide] Better control of pattern match handlers and line breaks.
Description
I would like to suggest having an option for pattern matching that could be set to one of four ways:
- Current behaviour: Whatever this is exactly ;-)
- Prefer newlines: Always have the clause handling on a new line and indented.
- Prefer same line: If the pattern match handler is small enough, put it on the same line as the handler.
- 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).
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 is the handler itself now always on a newline? That was ironically what I was hoping for (not so much the patterns themselves!).
@isaacabraham It is, at least when used after let =. It is side effect of pattern match expression being multiline now.
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"
@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)
Ok, maybe option 4 could be a thing. We should investigate if it possible to determine.
Thanks a lot - and sorry I didn't explain myself properly at the start. Code samples always help!
I'm afraid not, all effort is going to https://github.com/fsprojects/fantomas/pull/434 atm.
@Micha-kun the new setting in https://github.com/fsprojects/fantomas/issues/449 might help a bit in this scenario.
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:
- All match handlers go on a new line.
- 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)?
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:
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.
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.
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.
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.
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.
Yes, that is a fair take. If this is the default, huge diffs will appear the moment users will upgrade.
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.
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.
@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).
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.
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.
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...
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
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
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.
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.