Skip to content

Commit 43110c7

Browse files
brianrourkebollbrianrourkeboll
committed
Shift paren contents less aggressively
* The logic in dotnet#16666 addressed some multiline parentheses removal issues, but it was slightly too aggressive and sometimes shifted lines farther than was necessary or safe.
1 parent f29ce0a commit 43110c7

File tree

3 files changed

+240
-83
lines changed

3 files changed

+240
-83
lines changed

‎vsintegration/src/FSharp.Editor/CodeFixes/RemoveUnnecessaryParentheses.fs‎

Lines changed: 68 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -46,60 +46,19 @@ module private Patterns =
4646
else
4747
ValueNone
4848

49-
/// Trim only spaces from the start if there is something else
50-
/// before the open paren on the same line (or else we could move
51-
/// the whole inner expression up a line); otherwise trim all whitespace
52-
// from start and end.
53-
let (|Trim|) (span: TextSpan) (sourceText: SourceText) =
54-
let linePosition = sourceText.Lines.GetLinePosition span.Start
55-
let line = (sourceText.Lines.GetLineFromPosition span.Start).ToString()
56-
57-
if line.AsSpan(0, linePosition.Character).LastIndexOfAnyExcept(' ', '(') >= 0 then
58-
fun (s: string) -> s.TrimEnd().TrimStart ' '
59-
else
60-
fun (s: string) -> s.Trim()
61-
62-
/// Returns the offsides diff if the given span contains an expression
63-
/// whose indentation would be made invalid if the open paren
64-
/// were removed (because the offside line would be shifted), e.g.,
65-
///
66-
/// // Valid.
67-
/// (let x = 2
68-
/// x)
69-
///
70-
/// // Invalid.
71-
/// ←let x = 2
72-
/// x◌
73-
///
74-
/// // Valid.
75-
/// ◌let x = 2
76-
/// x◌
77-
[<return: Struct>]
78-
let (|OffsidesDiff|_|) (span: TextSpan) (sourceText: SourceText) =
79-
let startLinePosition = sourceText.Lines.GetLinePosition span.Start
80-
let endLinePosition = sourceText.Lines.GetLinePosition span.End
81-
let startLineNo = startLinePosition.Line
82-
let endLineNo = endLinePosition.Line
83-
84-
if startLineNo = endLineNo then
85-
ValueNone
86-
else
87-
let rec loop innerOffsides lineNo startCol =
88-
if lineNo <= endLineNo then
89-
let line = sourceText.Lines[lineNo].ToString()
49+
[<NoEquality; NoComparison>]
50+
type private InnerOffsides =
51+
/// We haven't found an inner construct yet.
52+
| NoneYet
9053

91-
match line.AsSpan(startCol).IndexOfAnyExcept(' ', ')') with
92-
| -1 -> loop innerOffsides (lineNo + 1) 0
93-
| i -> loop (i + startCol) (lineNo + 1) 0
94-
else
95-
ValueSome(startLinePosition.Character - innerOffsides)
54+
/// The start column of the first inner construct we find.
55+
/// This may not be on the same line as the open paren.
56+
| FirstLine of col: int
9657

97-
loop startLinePosition.Character startLineNo (startLinePosition.Character + 1)
98-
99-
let (|ShiftLeft|NoShift|ShiftRight|) n =
100-
if n < 0 then ShiftLeft -n
101-
elif n = 0 then NoShift
102-
else ShiftRight n
58+
/// The leftmost start column of an inner construct on a line
59+
/// following the first inner construct we found.
60+
/// We keep the first column of the first inner construct for comparison at the end.
61+
| FollowingLine of firstLine: int * followingLine: int
10362

10463
[<ExportCodeFixProvider(FSharpConstants.FSharpLanguageName, Name = CodeFix.RemoveUnnecessaryParentheses); Shared; Sealed>]
10564
type internal FSharpRemoveUnnecessaryParenthesesCodeFixProvider [<ImportingConstructor>] () =
@@ -147,22 +106,67 @@ type internal FSharpRemoveUnnecessaryParenthesesCodeFixProvider [<ImportingConst
147106

148107
match firstChar, lastChar with
149108
| '(', ')' ->
109+
/// Trim only spaces from the start if there is something else
110+
/// before the open paren on the same line (or else we could move
111+
/// the whole inner expression up a line); otherwise trim all whitespace
112+
/// from start and end.
113+
let (|Trim|) (sourceText: SourceText) =
114+
let linePosition = sourceText.Lines.GetLinePosition context.Span.Start
115+
let line = (sourceText.Lines.GetLineFromPosition context.Span.Start).ToString()
116+
117+
if line.AsSpan(0, linePosition.Character).LastIndexOfAnyExcept(' ', '(') >= 0 then
118+
fun (s: string) -> s.TrimEnd().TrimStart ' '
119+
else
120+
fun (s: string) -> s.Trim()
121+
122+
let (|ShiftLeft|NoShift|ShiftRight|) (sourceText: SourceText) =
123+
let startLinePosition = sourceText.Lines.GetLinePosition context.Span.Start
124+
let endLinePosition = sourceText.Lines.GetLinePosition context.Span.End
125+
let startLineNo = startLinePosition.Line
126+
let endLineNo = endLinePosition.Line
127+
128+
if startLineNo = endLineNo then
129+
NoShift
130+
else
131+
let outerOffsides = startLinePosition.Character
132+
133+
let rec loop innerOffsides lineNo startCol =
134+
if lineNo <= endLineNo then
135+
let line = sourceText.Lines[lineNo].ToString()
136+
137+
match line.AsSpan(startCol).IndexOfAnyExcept(' ', ')') with
138+
| -1 -> loop innerOffsides (lineNo + 1) 0
139+
| i ->
140+
match innerOffsides with
141+
| NoneYet -> loop (FirstLine(i + startCol)) (lineNo + 1) 0
142+
| FirstLine innerOffsides -> loop (FollowingLine(innerOffsides, i + startCol)) (lineNo + 1) 0
143+
| FollowingLine(firstLine, innerOffsides) ->
144+
loop (FollowingLine(firstLine, min innerOffsides (i + startCol))) (lineNo + 1) 0
145+
else
146+
innerOffsides
147+
148+
match loop NoneYet startLineNo (startLinePosition.Character + 1) with
149+
| NoneYet -> NoShift
150+
| FirstLine innerOffsides when innerOffsides < outerOffsides -> ShiftRight(outerOffsides - innerOffsides)
151+
| FirstLine innerOffsides -> ShiftLeft(innerOffsides - outerOffsides)
152+
| FollowingLine(firstLine, followingLine) ->
153+
match firstLine - outerOffsides with
154+
| 0 -> NoShift
155+
| 1 when firstLine < followingLine -> NoShift
156+
| primaryOffset when primaryOffset < 0 -> ShiftRight -primaryOffset
157+
| primaryOffset -> ShiftLeft primaryOffset
158+
150159
let adjusted =
151160
match sourceText with
152161
| TrailingOpen context.Span -> txt[1 .. txt.Length - 2].TrimEnd()
153-
154-
| Trim context.Span trim & OffsidesDiff context.Span spaces ->
155-
match spaces with
156-
| NoShift -> trim txt[1 .. txt.Length - 2]
157-
| ShiftLeft spaces -> trim (txt[1 .. txt.Length - 2].Replace("\n" + String(' ', spaces), "\n"))
158-
| ShiftRight spaces -> trim (txt[1 .. txt.Length - 2].Replace("\n", "\n" + String(' ', spaces)))
159-
160-
| _ -> txt[1 .. txt.Length - 2].Trim()
162+
| Trim trim & NoShift -> trim txt[1 .. txt.Length - 2]
163+
| Trim trim & ShiftLeft spaces -> trim (txt[1 .. txt.Length - 2].Replace("\n" + String(' ', spaces), "\n"))
164+
| Trim trim & ShiftRight spaces -> trim (txt[1 .. txt.Length - 2].Replace("\n", "\n" + String(' ', spaces)))
161165

162166
let newText =
163167
let (|ShouldPutSpaceBefore|_|) (s: string) =
164-
// "……(……)"
165-
// ↑↑ ↑
168+
// ……(……)
169+
// ↑↑ ↑
166170
match sourceText[max (context.Span.Start - 2) 0], sourceText[max (context.Span.Start - 1) 0], s[0] with
167171
| _, _, ('\n' | '\r') -> None
168172
| '[', '|', (Punctuation | LetterOrDigit) -> None
@@ -178,8 +182,8 @@ type internal FSharpRemoveUnnecessaryParenthesesCodeFixProvider [<ImportingConst
178182
| _ -> None
179183

180184
let (|ShouldPutSpaceAfter|_|) (s: string) =
181-
// "(……)…"
182-
// ↑ ↑
185+
// (……)…
186+
// ↑ ↑
183187
match s[s.Length - 1], sourceText[min context.Span.End (sourceText.Length - 1)] with
184188
| '>', ('|' | ']') -> Some ShouldPutSpaceAfter
185189
| _, (')' | ']' | '[' | '}' | '.' | ';' | ',' | '|') -> None

‎vsintegration/tests/FSharp.Editor.Tests/CodeFixes/CodeFixTestFramework.fs‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -205,13 +205,19 @@ module Xunit =
205205
let split (s: string) =
206206
s.Split([| Environment.NewLine |], StringSplitOptions.RemoveEmptyEntries)
207207

208-
let expected = split expected
209-
let actual = split actual
208+
let expectedLines = split expected
209+
let actualLines = split actual
210210

211-
try
212-
Assert.All((expected, actual) ||> Array.zip |> Array.rev, (fun (expected, actual) -> Assert.Equal(expected, actual)))
213-
with :? Xunit.Sdk.AllException as all when all.Failures.Count = 1 ->
214-
raise all.Failures[0]
211+
if expectedLines.Length <> actualLines.Length then
212+
Assert.Equal(expected, actual)
213+
else
214+
try
215+
Assert.All(
216+
(expectedLines, actualLines) ||> Array.zip |> Array.rev,
217+
(fun (expected, actual) -> Assert.Equal(expected, actual))
218+
)
219+
with :? Xunit.Sdk.AllException as all when all.Failures.Count = 1 ->
220+
raise all.Failures[0]
215221

216222
/// <summary>
217223
/// Expects no code fix to be applied to the given code.

0 commit comments

Comments
 (0)