The code fix alignment problem

[brianrourkeboll]

The problem

Adding or removing text in source, as all code fixes and refactorings do, may break the indentation of a multiline, indentation-sensitive expression that begins farther to the right on the same line. This may sound like an obvious and trivial statement, but it does mean that any individual application of any code fix or refactoring, or any "fix-all" application thereof to a whole document, project, or solution, has the potential to break code.

While I originally ran into this problem when applying the "remove unnecessary parentheses" code fix to the entire VisualFSharp solution, it applies to any code fix or refactoring in any editor that uses text-based code fixes (as Visual Studio and FSAC/Ionide do; I'm not familiar with Rider).

For example, using the "remove unnecessary parentheses" code fix to remove parentheses from any arbitrary expression or pattern may break the indentation of a multiline expression that begins on the same line to the right. Such an offending trailing multiline expression could be hanging off a match-clause, a let-, use-, or do-binding, a member definition, an if-then-else expression, any arbitrary infix operator (including user-defined ones), etc., etc., etc., and it could be nested to any depth in nearly any kind of outer expression:

// Removing any of these extra parens below will cause breakages.
type A (_x) = //   ↓ ↓                                  ↓ ↓       ↓ ↓                   ↓   ↓                            ↓ ↓
        member _.M (x) = A x;; type B = static member N (_) = let (_) = A(3).M(4).M(5).M((6)).M(7).M(match None with Some(_) -> let x = 3
                                                                                                                                x + x | _ -> 99).M(8).ToString()
                                                              3                                                              // ↑ Vulnerable.
                                                           // ↑ Vulnerable.

Recording.2024-04-07.193448.mp4

Code changes are not even safe across module boundaries:

// Removing these extra parens will break code in the trailing module.
// So would applying the fixes for "add type annotation,"
// "add return type," "add explicit type to parameter," or
// any other code fix or refactoring that altered the source
// to the left of the sensitive construct.
//                         ↓ ↓
module M = begin let f x = (3) end;; module N = let _ = match None with Some _ -> let x = 3
                                                                                  x + x | _ -> 99
                                                                               // ↑ Vulnerable.

Code that is sensitive to changes in this way is, thankfully, relatively uncommon in most F# codebases these days, and it is generally advised against, but it does nonetheless occur, including in this repo:

fsharp/vsintegration/tests/Salsa/salsa.fs

Lines 759 to 762 in 050271d

	match prevSolutions.TryFind fullname with
	| Some(s) -> prevSolutions <- prevSolutions.Remove fullname
	s
	| None -> failwith "solution with that project does not exist"

I do not however see an obviously cheap or easy way of using the existing AST traversal to determine whether any arbitrary expression is followed on the same line by an indentation-sensitive multiline expression — as far as I can tell, it would involve a significant amount of backtracking and retraversal, and it would likely require memoization to avoid massive worst-case time complexity.

Known workarounds

• Always begin a potentially indentation-sensitive multiline expression on a new line, and always use verbose syntax — although even doing both of these may not cover all possible cases.
• Use a code formatter configured to avoid all indentation-sensitive code and ensure that the code has been formatted accordingly before applying any code fix.
• Manually fix the indentation of any broken code after applying a code fix or refactoring.

Potential solutions

AST-based code fixes

In theory, this problem could be avoided altogether if the tooling that offered the analysis and code fix (VS, FSAC, Rider) were also integrated with a code formatter — i.e., if the code fix were formulated as an update to the AST instead of as an update to the source text. The formatter would then simply print the updated AST following its existing rules.

Dedicated data structure

Alternatively, I wonder whether it would be possible (and, if so, worthwhile) to track indentation-sensitive multiline constructs in some kind of dedicated data structure when building the AST that would enable easy querying by range later on, e.g., something that tracked such sensitive constructs by start position and that could be queried by another construct's end position to determine whether that construct was followed on the same line by an indentation-sensitive multiline construct.