Repository navigation
Add Extract to let binding and Extract to literal refactorings - #20537
xperiandri wants to merge 12 commits into
Conversation
✅ Release notes checked
|
|
|
||
| tryGetRefactoringActionsForSpan code (selectionOf code selected) context (new FSharpExtractLetBindingRefactoring()) | ||
| |> Seq.map _.Title | ||
| |> List.ofSeq |
There was a problem hiding this comment.
| |> List.ofSeq | |
| |> Seq.toList |
This comment has been minimized.
This comment has been minimized.
T-Gro
left a comment
There was a problem hiding this comment.
🤖🕵️ 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.
Covered casesOffered
Not offered
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| | SyntaxNode.SynExpr(SynExpr.LetOrUse letOrUse), SyntaxNode.SynExpr expr when isSame letOrUse.Body expr -> | ||
| ValueSome(Anchor.Statement expr.Range.Start) | ||
| | SyntaxNode.SynExpr(SynExpr.For(doBody = body) | SynExpr.ForEach(bodyExpr = body) | SynExpr.While(doExpr = body) | SynExpr.TryWith( | ||
| tryExpr = body) | SynExpr.TryFinally(tryExpr = body) | SynExpr.ComputationExpr(expr = body)), |
There was a problem hiding this comment.
🤖 🕵️ [P1] Keep extraction inside the finally block. Selecting stream.Dispose() inserts let extracted = stream.Dispose() before the try, so this working function throws ObjectDisposedException at WriteByte. Moving cleanup across the finally boundary is different from evaluating a value earlier within its enclosing statement. Give finallyExpr its own anchor, as tryExpr already has, or do not offer extraction there.
open System.IO
let f () =
use stream = new MemoryStream()
try
stream.WriteByte(1uy)
finally
stream.Dispose() // Select this expression.There was a problem hiding this comment.
Fixed in 07b192f: finallyExpr has its own anchor, so the binding goes inside the finally block. Test: Expression in a finally block is bound inside the block.
| |> String.concat "" | ||
|
|
||
| [ | ||
| TextChange(TextSpan(line.Start, 0), $"{attributeLines}{padding indent}{declaration}{lineBreak}{lineBreak}") |
There was a problem hiding this comment.
🤖 🕵️ [P2] Keep the literal inside its enclosing module. Selecting 42 here inserts [<Literal>] let ExtractedConstant = 42 between namespace N and module M, so the valid original becomes a namespace-level value and fails with FS0201, "Namespaces cannot contain values." Inserting at the physical line start escapes the inline module. Split its body correctly or decline this action when the module's let is not line-leading.
namespace N
module M = let f () = 42 // Extract 42 to literal.There was a problem hiding this comment.
Fixed in 07b192f: the literal is not offered when the module-level let does not start its line. Test: Literal is not offered where the declaration does not start its line.
c2d5181 to
90f8221
Compare
T-Gro
left a comment
There was a problem hiding this comment.
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 38 new code comment lines and 3462 new code comment chars.
ExtractLetBinding.fs:78-93reaches at least 3 logical nesting levels.- Tests would benefit from deduplicated setup or code-as-test reuse:
ExtractLetBindingTests.fscontains 21 copied[<Fact>]methods with repeatedmodule Mboilerplate that should be parameterized.
…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>
d3742a6 to
2920e42
Compare
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>
|
Compacted in 034e6f2 (production code) and 1711de9 (tests): the F# lines this PR adds against main went from 1731 to 1498, and the comment lines from 38 (3578 chars) to 3 (320 chars: two license headers and one note that the public tokenizer enum does not name the interpolated string states); everything else is now said by names such as |
T-Gro
left a comment
There was a problem hiding this comment.
🤖🕵️ AI review — verify independently.
|
|
||
| lines | ||
|
|
||
| let usedNames (parseTree: ParsedInput) = |
There was a problem hiding this comment.
🤖🕵️ Extracted binding shadows an import used in an inactive branch; enabling ALT changes 13 to 6.
module M
open Dependency
let f () =
let n = (1 + 2)
#if ALT
n + extracted
#else
n
#endifThis branch had only the two copies without the PR link. They give way to the bullet the PR keeps (7bf3578 of dotnet#20537). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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>
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 had only the two copies without the PR link. They give way to the bullet the PR keeps (7bf3578 of dotnet#20537). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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>
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 had only the two copies without the PR link. They give way to the bullet the PR keeps (7bf3578 of dotnet#20537). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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>
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 had only the two copies without the PR link. They give way to the bullet the PR keeps (7bf3578 of dotnet#20537). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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>
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>
Offers
Ctrl+.on a selected expression to bind it to aletin front of the statement that uses it, or, for a constant in a module-level declaration, to a[<Literal>]above that declaration. A caret inside a constant offers the literal; a caret in thefun … ->header of a parenthesized lambda extracts the lambda.The value is computed at the start of the enclosing statement, so it can run earlier than where it was written, as with C#'s Introduce Local. The selection must match a whole expression; nothing is offered inside quotations, interpolation holes,
whileconditions orwhenguards.🤖 Generated with Claude Code