Skip to content

Add Extract to function and Extract to private member refactorings - #20538

Open
xperiandri wants to merge 35 commits into
dotnet:mainfrom
xperiandri:feature/extract-function-refactoring
Open

xperiandri wants to merge 35 commits into
dotnet:mainfrom
xperiandri:feature/extract-function-refactoring

Conversation

@xperiandri

@xperiandri xperiandri commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor

Stacked on #20537.

Offers Ctrl+. on a selected expression to move it into a local function, a module function, or, when it uses this or base, a private member. Values the selection reads from the enclosing function become parameters in first-use order.

Parameters are annotated by default because the new function is its own generalization point; the new Parameter types in Extract to function option under Code Fixes can relax that. Not offered when the selection assigns to a captured mutable or reads a byref value.

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor

✅ Release notes checked


✅ Found changes and release notes in following paths:

Change path Release notes path Description
`vsintegration/src` docs/release-notes/.VisualStudio/18.vNext.md

@xperiandri
xperiandri force-pushed the feature/extract-function-refactoring branch from 9689259 to dc118df Compare September 14, 2026 14:36
@xperiandri
xperiandri marked this pull request as ready for review September 14, 2026 19:40
@xperiandri
xperiandri requested a review from a team as a code owner September 14, 2026 19:40
@github-actions github-actions Bot added the ⚠️ Affects-Design-Time Tooling check: PR touches type providers or dependency manager label Sep 14, 2026
@github-actions

This comment has been minimized.

@T-Gro T-Gro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖🕵️ Please shorten the description using this guidance. Focus on the problem and why the change is needed, in simplified technical English. Leave the implementation inventory to the Files tab and retain necessary caveats.

@github-project-automation github-project-automation Bot moved this from New to In Progress in F# Compiler and Tooling Sep 15, 2026
@xperiandri xperiandri changed the title Refactorings to extract a selected expression into a local function, a module function or a private member Add Extract to function and Extract to private member refactorings Sep 15, 2026
@xperiandri

Copy link
Copy Markdown
Contributor Author

Covered cases

Offered

  • Values the selection uses from the enclosing declaration become parameters of a local function: printfn "%d" (w * h + 1) → let extractedFunction (w: int) (h: int) = w * h + 1 + printfn "%d" (extractedFunction w h)
  • The module function is inserted before the declaration that uses it: let private extractedFunction (w: int) (h: int) = w * h + 1 above let area …
  • Selecting (w * h + 1) with its parentheses gives the same result, and the call stays parenthesized
  • A selection that uses no outside values becomes a function of unit: 1 + 2 → let extractedFunction () = 1 + 2 + extractedFunction ()
  • A selection that uses this becomes a private member after the enclosing member: l * this.Rate in List.sumBy (fun l -> l * this.Rate) → this.ExtractedMethod(l) + member private this.ExtractedMethod(l: int) = l * this.Rate
  • The parameter-annotation option: Always → let extractedFunction (s: string) (n: int) =, WhenNeeded → (s: string) n (only the receiver of s.ToUpper() is annotated), Never → s n
  • Which actions appear: a plain selection (w * h + 1) offers "Extract to local function" and "Extract to module function"; a selection using base (base.ToString() + "!") offers only "Extract to private member"
  • A caret in the header of a parenthesized lambda (x -> in (fun x -> x + n)) offers the same actions, with the same results, as selecting the whole (fun x -> x + n)

Not offered

  • With no selection, a caret in the lambda body: x + n in (fun x -> x + n)
  • The selection assigns to a captured mutable local: for i in 1 .. 3 do n <- n + i
  • The selection reads a byref parameter: x + 1 with x: byref<int>

@T-Gro T-Gro added the vsintegration-only Changes only Visual Studio integration, plus optional docs, release notes, or tests label Sep 21, 2026
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

xperiandri and others added 3 commits October 11, 2026 09:33
…tion

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- See a byref-like type through a type abbreviation.
- Do not offer when the selection reads a generic local function, or a
  mutable local inside a lambda or other delayed code.
- No local function for a selection using this of a struct.
- No module function, and no private member for a type parameter of the
  member, when the selection names a type parameter from outside.
- No module function or private member called from an inline function
  or member that is not private.
- No private member where a local of the member hides the self
  identifier; keep the backticks of the self identifier and type name.
- Parenthesize the call that is the receiver of an assignment.
- Under WhenNeeded, annotate the arguments of methods and constructors.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@T-Gro T-Gro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diff is still way too big - apply the NoBloat instructions and CodeCompaction skill.

Especially the massive code comments would benefit not just with drastic reduction, but ideally with well named functions/arguments/bindings/types making the intention clear.

If a well named piece of code just tells what the comment says - delete the comment.

  • This diff adds 63 new code comment lines and 5702 new code comment chars.
  • ComputeRefactoringsAsync in ExtractFunction.fs:370-465 reaches 6 nested control-flow levels.
  • Tests would benefit from deduplicated setup or code-as-test reuse: ExtractFunctionTests.fs and ExtractLetBindingTests.fs copy dozens of single-case [<Fact>] methods with repeated module M setup instead of using parameterized tests.

xperiandri and others added 16 commits October 15, 2026 01:38
…g or a literal

The selected expression is bound to a new name in front of the statement that uses it: above a statement that starts its line, or, when the body of a binding, match clause, branch or lambda shares the line with its keyword, on new lines under that keyword. A constant inside a module-level declaration can instead become a [<Literal>] in front of that declaration. The shared selection and placement logic lives in Refactor/RefactoringHelpers.fs, and the refactoring test framework gains helpers that run a provider on a selected span.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
C# offers Introduce Constant without a selection. With a caret alone only the
literal is offered; a let binding still needs a selection.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A caret between 'fun' and the end of '->' acts as a selection of the
parenthesized lambda; in the body a caret offers nothing new.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e of its section

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…on'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>
…to literal

- Annotate an extracted lambda or function expression with the type its
  context gave it, unless that type has a variable inference introduced.
- Do not extract from inside assert, the operand of nameof, or a
  parenthesized name, address-of or named argument.
- Bind an expression of a finally block inside that block.
- Indent a body moved under its keyword from the let, match, try, if or
  loop that starts inside the line.
- Offer the literal for decimal constants and not for native integers.
- Qualify the generated Literal attribute where another LiteralAttribute
  hides the FSharp.Core one.
- Do not offer the literal where the module-level let does not start its
  line.
- Reserve every identifier of a long pattern when picking a new name.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ected inside its parentheses

The checker records the type of the parenthesized expression, so a lambda
selected without the parentheses around it takes the type from them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ion-refactoring

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Replace the explanatory comments with names, keep the document state of an extraction and the registration of its code actions in one record and one function, flatten the provider and the declaration builders to two levels of nesting, and scan the rest of a line for closing brackets without copying it.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
One theory per kind of assertion with a scenario name per row. The module wrapper, the selection and caret spans, the refactored text and the offered titles move to the refactoring test framework, where the Extract to function tests can use them too. Every case is kept; tests that asserted several things became one row per assertion, so 64 cases became 74.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Both sides had the same Extract to let binding code; the merge takes the compacted helpers, providers and tests of feature/extract-let-binding-refactoring and keeps the resource strings of both branches. ExtractFunction.fs only follows the renamed target spans and the shared Extraction record; it is compacted in the next commits.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Replace the explanatory comments with names, give the symbol classification of the captures active patterns instead of five nested matches, split the three variants and the provider into functions that nest at most two levels, and drop the duplicated registration, padding and state gathering in favour of the shared Extraction record.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
One theory per kind of assertion with a scenario name per row, on the helpers the Extract to let binding tests share. Every case is kept; tests that asserted several things became one row per assertion, so 35 cases became 38.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@xperiandri

Copy link
Copy Markdown
Contributor Author

Merged the compacted base of #20537 (1711de9) in 5aabbc8 and compacted this PR's own code in 204be8f (ExtractFunction.fs) and b4cccb8 (tests): against main the F# lines this PR adds went from 2766 to 2378, and the comment lines from 63 (5996 chars) to 4 (445 chars: three license headers and one note that the public tokenizer enum does not name the interpolated string states); the rest is said by names such as CapturedLocal, TypeParameterDeclaredOutside, closureCannotCapture, readsMutableInDelayedCode and assignsCapturedValue.
ComputeRefactoringsAsync went from 7 nested levels by my indentation count (6 in the review) to 2 and now only hands the selection to FunctionExtraction.offer; tryCaptures (4), positionsNeedingType and curriedParameters (3 each) are at 2 as well, and the registration, padding and state gathering that duplicated ExtractLetBinding.fs are now the shared registerRefactoring, padding and Extraction.
ExtractFunctionTests went from 17 [<Fact>] plus 4 [<Theory>] methods (35 cases) to 5 [<Theory>] methods and one [<Fact>] over 38 named rows, and ExtractLetBindingTests from 31 methods (64 cases) to 6 theories over 74 rows (a test that asserted several things became one row per assertion), both on the module M helper and the span, text and title helpers in RefactorTestFramework.fs.
ExtractLetBindingTests 74/74, ExtractFunctionTests 38/38 and the AddReturnTypeTests control 22/22 pass, and FSharp.Editor builds with --optimize+.

@T-Gro T-Gro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖🕵️ AI review — verify independently.

let application = $"{name} {curriedParameters never captures.Parameters}"

struct {|
Header = $"{name} {curriedParameters annotate captures.Parameters}"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖🕵️ Default extraction loses the lambda's contextual type and fails type checking.

let f (xs: string list) =
    xs |> List.map (fun x -> x.Length)


match tryCaptures extraction.CheckResults selection declaration extraction.Context.CancellationToken with
| ValueSome captures when
not (assignsCapturedValue target.Expr captures.Parameters)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖🕵️ Extraction copies mutable storage; the result changes from 1 to 0.

let f () =
    let mutable n = 0
    let bump () = n <- n + 1
    (bump (); n)

}

true
| (false, _), Some fullType when not (isByRefLike fullType) && value.GenericParameters.Count = 0 ->

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖🕵️ Extraction copies a mutable struct receiver; the result changes from 2 to 1.

[<Struct>]
type S =
    val mutable X: int
    new(x) = { X = x }
    member self.Inc() = self.X <- self.X + 1

xperiandri added a commit to xperiandri/fsharp that referenced this pull request Oct 19, 2026
Port of the dotnet#20537 compaction (feature/extract-let-binding-refactoring
2920e42..1711de9) to all-fixes:

- Compact the Extract to let binding code and name what its comments said
- Fold the Extract to let binding tests into parameterized theories

Extract to function (dotnet#20538) shares RefactoringHelpers.fs, so its callers
follow the renames here, as in the pull request's merge of this head.

The refactoring test framework carries the helpers of both the Extract and the
tuple/record test suites on all-fixes. The caret helper of the Extract suites,
which returns a TextSpan and reads a `|` in the marker, is therefore named
caretSpanAt; the existing caretAt of the conversion suites keeps returning the
index of the marker.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
xperiandri added a commit to xperiandri/fsharp that referenced this pull request Oct 19, 2026
Port of the dotnet#20538 compaction (feature/extract-function-refactoring
838b68d..b4cccb8) to all-fixes. The part of that range which merges the
compacted Extract to let binding (dotnet#20537) came with the previous commit; this
one carries what is specific to Extract to function:

- Compact the Extract to function code and name what its comments said
- Fold the Extract to function tests into parameterized theories

The tests use caretSpanAt, the name the Extract suites' caret helper has in the
shared refactoring test framework on all-fixes.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
xperiandri added a commit to xperiandri/fsharp that referenced this pull request Oct 19, 2026
Port of the dotnet#20537 compaction (feature/extract-let-binding-refactoring
2920e42..1711de9) to all-fixes:

- Compact the Extract to let binding code and name what its comments said
- Fold the Extract to let binding tests into parameterized theories

Extract to function (dotnet#20538) shares RefactoringHelpers.fs, so its callers
follow the renames here, as in the pull request's merge of this head.

The refactoring test framework carries the helpers of both the Extract and the
tuple/record test suites on all-fixes. The caret helper of the Extract suites,
which returns a TextSpan and reads a `|` in the marker, is therefore named
caretSpanAt; the existing caretAt of the conversion suites keeps returning the
index of the marker.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
xperiandri added a commit to xperiandri/fsharp that referenced this pull request Oct 19, 2026
Port of the dotnet#20538 compaction (feature/extract-function-refactoring
838b68d..b4cccb8) to all-fixes. The part of that range which merges the
compacted Extract to let binding (dotnet#20537) came with the previous commit; this
one carries what is specific to Extract to function:

- Compact the Extract to function code and name what its comments said
- Fold the Extract to function tests into parameterized theories

The tests use caretSpanAt, the name the Extract suites' caret helper has in the
shared refactoring test framework on all-fixes.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
xperiandri added a commit to xperiandri/fsharp that referenced this pull request Oct 23, 2026
Port of the dotnet#20537 compaction (feature/extract-let-binding-refactoring
2920e42..1711de9) to all-fixes:

- Compact the Extract to let binding code and name what its comments said
- Fold the Extract to let binding tests into parameterized theories

Extract to function (dotnet#20538) shares RefactoringHelpers.fs, so its callers
follow the renames here, as in the pull request's merge of this head.

The refactoring test framework carries the helpers of both the Extract and the
tuple/record test suites on all-fixes. The caret helper of the Extract suites,
which returns a TextSpan and reads a `|` in the marker, is therefore named
caretSpanAt; the existing caretAt of the conversion suites keeps returning the
index of the marker.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
xperiandri added a commit to xperiandri/fsharp that referenced this pull request Oct 23, 2026
Port of the dotnet#20538 compaction (feature/extract-function-refactoring
838b68d..b4cccb8) to all-fixes. The part of that range which merges the
compacted Extract to let binding (dotnet#20537) came with the previous commit; this
one carries what is specific to Extract to function:

- Compact the Extract to function code and name what its comments said
- Fold the Extract to function tests into parameterized theories

The tests use caretSpanAt, the name the Extract suites' caret helper has in the
shared refactoring test framework on all-fixes.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
xperiandri added a commit to xperiandri/fsharp that referenced this pull request Oct 23, 2026
Port of the dotnet#20537 compaction (feature/extract-let-binding-refactoring
2920e42..1711de9) to all-fixes:

- Compact the Extract to let binding code and name what its comments said
- Fold the Extract to let binding tests into parameterized theories

Extract to function (dotnet#20538) shares RefactoringHelpers.fs, so its callers
follow the renames here, as in the pull request's merge of this head.

The refactoring test framework carries the helpers of both the Extract and the
tuple/record test suites on all-fixes. The caret helper of the Extract suites,
which returns a TextSpan and reads a `|` in the marker, is therefore named
caretSpanAt; the existing caretAt of the conversion suites keeps returning the
index of the marker.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
xperiandri added a commit to xperiandri/fsharp that referenced this pull request Oct 23, 2026
Port of the dotnet#20538 compaction (feature/extract-function-refactoring
838b68d..b4cccb8) to all-fixes. The part of that range which merges the
compacted Extract to let binding (dotnet#20537) came with the previous commit; this
one carries what is specific to Extract to function:

- Compact the Extract to function code and name what its comments said
- Fold the Extract to function tests into parameterized theories

The tests use caretSpanAt, the name the Extract suites' caret helper has in the
shared refactoring test framework on all-fixes.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

⚠️ Affects-Build-Infra Tooling check: PR touches build infrastructure ⚠️ Affects-Design-Time Tooling check: PR touches type providers or dependency manager vsintegration-only Changes only Visual Studio integration, plus optional docs, release notes, or tests

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

4 participants