Skip to content

Commit 90f8221

Browse files
xperiandriclaude
andcommitted
Keep assignment targets and inactive string lines out of the extraction's way
The name collector now also records the heads of assignment targets, so the new local cannot shadow a value the selection's statement assigns to. Lines inside multi-line strings of inactive #if branches are found by lexing each inactive branch as the code it is when taken, and are not re-indented. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 9a41635 commit 90f8221

4 files changed

Lines changed: 169 additions & 17 deletions

File tree

‎vsintegration/src/FSharp.Editor/Refactor/ExtractLetBinding.fs‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,8 +69,11 @@ type internal FSharpExtractLetBindingRefactoring [<ImportingConstructor>] () =
6969
let indentSize =
7070
options.GetOption(FormattingOptions.IndentationSize, FSharpConstants.FSharpLanguageName)
7171

72+
let! defines, langVersion = document.GetFsharpParsingOptionsAsync(nameof FSharpExtractLetBindingRefactoring)
7273
let names = usedNames parseResults.ParseTree
73-
let literalLines = linesInsideLiterals parseResults.ParseTree
74+
75+
let literalLines =
76+
linesInsideLiterals sourceText parseResults.ParseTree defines langVersion
7477

7578
if isSelected then
7679
match anchorsOf target.Expr target.Path with

‎vsintegration/src/FSharp.Editor/Refactor/RefactoringHelpers.fs‎

Lines changed: 94 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ open Microsoft.CodeAnalysis.Text
1111
open FSharp.Compiler.Syntax
1212
open FSharp.Compiler.SyntaxTrivia
1313
open FSharp.Compiler.Text
14+
open FSharp.Compiler.Tokenization
1415

1516
/// A place in front of which a declaration used by the selected expression can be inserted.
1617
[<RequireQualifiedAccess; NoComparison; NoEquality>]
@@ -76,25 +77,107 @@ let restOfLineIsClosers (sourceText: SourceText) (position: pos) =
7677
code
7778
|> Seq.forall (fun c -> Char.IsWhiteSpace c || c = ')' || c = ']' || c = '}' || c = '|')
7879

79-
let linesInsideLiterals (parseTree: ParsedInput) =
80-
(HashSet<int>(), parseTree)
81-
||> ParsedInput.fold (fun lines _ node ->
82-
match node with
83-
| SyntaxNode.SynExpr(SynExpr.Const(range = m))
84-
| SyntaxNode.SynExpr(SynExpr.InterpolatedString(range = m))
85-
| SyntaxNode.SynPat(SynPat.Const(range = m)) ->
86-
for line in m.StartLine + 1 .. m.EndLine do
87-
lines.Add(Line.toZ line) |> ignore
88-
| _ -> ()
80+
/// Whether a line lexed from this state starts inside a string literal.
81+
let private continuesString (state: FSharpTokenizerColorState) =
82+
match state with
83+
| FSharpTokenizerColorState.InitialState
84+
| FSharpTokenizerColorState.Token
85+
| FSharpTokenizerColorState.IfDefSkip
86+
| FSharpTokenizerColorState.EndLineThenSkip
87+
| FSharpTokenizerColorState.EndLineThenToken
88+
| FSharpTokenizerColorState.Comment
89+
| FSharpTokenizerColorState.SingleLineComment
90+
| FSharpTokenizerColorState.StringInComment
91+
| FSharpTokenizerColorState.VerbatimStringInComment
92+
| FSharpTokenizerColorState.TripleQuoteStringInComment -> false
93+
// String, VerbatimString, TripleQuoteString and the extended interpolated string state, which the public enum does not name.
94+
| _ -> true
95+
96+
let private isSkipped (state: FSharpTokenizerLexState) =
97+
match FSharpLineTokenizer.ColorStateOfLexState state with
98+
| FSharpTokenizerColorState.IfDefSkip
99+
| FSharpTokenizerColorState.EndLineThenSkip -> true
100+
| _ -> false
101+
102+
let private startsWithDirective (code: ReadOnlySpan<char>) (keyword: string) =
103+
code.StartsWith(keyword.AsSpan(), StringComparison.Ordinal)
104+
&& (code.Length = keyword.Length
105+
|| Char.IsWhiteSpace code[keyword.Length]
106+
|| code[keyword.Length] = '/')
107+
108+
/// Whether the line is a directive the lexer recognizes while it skips an inactive branch.
109+
let private isSkippedDirective (line: string) =
110+
let code = line.AsSpan().TrimStart()
111+
112+
startsWithDirective code "#if"
113+
|| startsWithDirective code "#elif"
114+
|| startsWithDirective code "#else"
115+
|| startsWithDirective code "#endif"
116+
117+
let private scanLine (tokenizer: FSharpSourceTokenizer) (line: string) (state: FSharpTokenizerLexState) =
118+
let lineTokenizer = tokenizer.CreateLineTokenizer line
119+
120+
let rec loop state =
121+
match lineTokenizer.ScanToken state with
122+
| Some _, next -> loop next
123+
| None, next -> next
124+
125+
loop state
126+
127+
/// Lines inside multi-line string literals of the branches of `#if` the defines leave inactive. Each run of inactive
128+
/// lines between two directives is lexed as the code it is when its branch is taken.
129+
let private linesInsideInactiveLiterals (sourceText: SourceText) (defines: string list) (langVersion: string) (lines: HashSet<int>) =
130+
let fileTokenizer = FSharpSourceTokenizer(defines, None, Some langVersion)
131+
let branchTokenizer = FSharpSourceTokenizer([], None, Some langVersion)
132+
let mutable fileState = FSharpTokenizerLexState.Initial
133+
let mutable branchState = FSharpTokenizerLexState.Initial
134+
135+
for line in sourceText.Lines do
136+
let text = line.ToString()
137+
138+
if isSkipped fileState && not (isSkippedDirective text) then
139+
if continuesString (FSharpLineTokenizer.ColorStateOfLexState branchState) then
140+
lines.Add line.LineNumber |> ignore
141+
142+
branchState <- scanLine branchTokenizer text branchState
143+
else
144+
branchState <- FSharpTokenizerLexState.Initial
145+
146+
fileState <- scanLine fileTokenizer text fileState
147+
148+
/// Lines inside multi-line string literals, including those in branches of `#if` the defines leave inactive.
149+
let linesInsideLiterals (sourceText: SourceText) (parseTree: ParsedInput) (defines: string list) (langVersion: string) =
150+
let lines =
151+
(HashSet<int>(), parseTree)
152+
||> ParsedInput.fold (fun lines _ node ->
153+
match node with
154+
| SyntaxNode.SynExpr(SynExpr.Const(range = m))
155+
| SyntaxNode.SynExpr(SynExpr.InterpolatedString(range = m))
156+
| SyntaxNode.SynPat(SynPat.Const(range = m)) ->
157+
for line in m.StartLine + 1 .. m.EndLine do
158+
lines.Add(Line.toZ line) |> ignore
159+
| _ -> ()
160+
161+
lines)
162+
163+
let directives =
164+
match parseTree with
165+
| ParsedInput.ImplFile file -> file.Trivia.ConditionalDirectives
166+
| ParsedInput.SigFile file -> file.Trivia.ConditionalDirectives
167+
168+
if not directives.IsEmpty then
169+
linesInsideInactiveLiterals sourceText defines langVersion lines
89170

90-
lines)
171+
lines
91172

92173
let usedNames (parseTree: ParsedInput) =
93174
(HashSet<string>(StringComparer.Ordinal), parseTree)
94175
||> ParsedInput.fold (fun names _ node ->
95176
match node with
96177
| SyntaxNode.SynExpr(SynExpr.Ident ident)
97178
| SyntaxNode.SynExpr(SynExpr.LongIdent(longDotId = SynLongIdent(id = ident :: _)))
179+
| SyntaxNode.SynExpr(SynExpr.LongIdentSet(longDotId = SynLongIdent(id = ident :: _)))
180+
| SyntaxNode.SynExpr(SynExpr.NamedIndexedPropertySet(longDotId = SynLongIdent(id = ident :: _)))
98181
| SyntaxNode.SynPat(SynPat.Named(ident = SynIdent(ident, _)))
99182
| SyntaxNode.SynPat(SynPat.LongIdent(longDotId = SynLongIdent(id = ident :: _))) -> names.Add ident.idText |> ignore
100183
| _ -> ()

‎vsintegration/tests/FSharp.Editor.Tests/Refactors/ExtractLetBindingTests.fs‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -309,6 +309,62 @@ let ``Lines inside a multi-line string are not re-indented`` () =
309309

310310
Assert.Equal(expected, extracted extractToLetBinding code "String.Format(\"\"\"Hello\n{0}\"\"\", name)")
311311

312+
[<Fact>]
313+
let ``Lines inside a multi-line string in an inactive branch are not re-indented`` () =
314+
// The code contains a triple-quoted string, which a triple-quoted literal cannot hold.
315+
let code =
316+
"module M\n\nlet r = combine 1 [\n #if ALT\n \"\"\"first\n preserved\n last\"\"\"\n #else\n \"\"\n #endif\n ]\n"
317+
318+
let expected =
319+
"module M\n\nlet r =\n let extracted = 1\n combine extracted [\n #if ALT\n \"\"\"first\n preserved\n last\"\"\"\n #else\n \"\"\n #endif\n ]\n"
320+
321+
Assert.Equal(expected, extractedAt extractToLetBinding code (TextSpan(code.IndexOf("1 [", StringComparison.Ordinal), 1)))
322+
323+
[<Theory>]
324+
[<InlineData("let extracted = { Value = 0 }", "extracted.Value <- ")>]
325+
[<InlineData("let extracted = [| 0 |]", "extracted[0] <- ")>]
326+
[<InlineData("let mutable extracted = 0", "extracted <- ")>]
327+
let ``Name does not collide with an assignment target from another file`` (declaration: string, assignment: string) =
328+
let dependency =
329+
"""
330+
module Dependency
331+
332+
type Box = { mutable Value: int }
333+
334+
DECLARATION
335+
"""
336+
.Replace("DECLARATION", declaration)
337+
338+
let code =
339+
"""
340+
module M
341+
342+
open Dependency
343+
344+
let f () =
345+
ASSIGNMENT({ Value = 1 }).Value
346+
"""
347+
.Replace("ASSIGNMENT", assignment)
348+
349+
let expected =
350+
"""
351+
module M
352+
353+
open Dependency
354+
355+
let f () =
356+
let extracted1 = { Value = 1 }
357+
ASSIGNMENTextracted1.Value
358+
"""
359+
.Replace("ASSIGNMENT", assignment)
360+
361+
use context = TestContext.CreateWithCodeAndDependency code dependency
362+
363+
let document =
364+
refactorSpan code (selectionOf code "({ Value = 1 })") extractToLetBinding context (new FSharpExtractLetBindingRefactoring())
365+
366+
Assert.Equal(expected, (document.GetTextAsync() |> GetTaskResult).ToString())
367+
312368
let private greet =
313369
"""
314370
module M

‎vsintegration/tests/FSharp.Editor.Tests/Refactors/RefactorTestFramework.fs‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ open Microsoft.CodeAnalysis
88
open Microsoft.CodeAnalysis.Text
99
open Microsoft.VisualStudio.FSharp.Editor.CancellableTasks
1010

11+
open FSharp.Test.ProjectGeneration
12+
1113
open FSharp.Editor.Tests.Helpers
1214
open Microsoft.CodeAnalysis.CodeRefactorings
1315
open Microsoft.CodeAnalysis.CodeActions
@@ -31,11 +33,19 @@ type TestContext(Solution: Solution) =
3133
new TestContext(solution)
3234

3335
static member CreateWithCodeAndDependency (code: string) (codeForPreviousFile: string) =
34-
let mutable solution = RoslynTestHelpers.CreateSolution(codeForPreviousFile)
35-
36-
let firstProject = solution.Projects.First()
37-
solution <- solution.AddDocument(DocumentId.CreateNewId(firstProject.Id), "test2.fs", code, filePath = "C:\\test2.fs")
38-
36+
let project =
37+
{ SyntheticProject.Create(
38+
{ sourceFile "First" [] with
39+
Source = codeForPreviousFile
40+
},
41+
{ sourceFile "Second" [ "First" ] with
42+
Source = code
43+
}
44+
) with
45+
AutoAddModules = false
46+
}
47+
48+
let solution, _ = RoslynTestHelpers.CreateSolution project
3949
new TestContext(solution)
4050

4151
let tryRefactor (code: string) (cursorPosition) (context: TestContext) (refactorProvider: 'T :> CodeRefactoringProvider) =

0 commit comments

Comments
 (0)