Repository navigation
Add a refactoring between ?x and [<Struct>] ?x optional parameters - #20546
xperiandri wants to merge 9 commits into
Conversation
✅ Release notes checked
|
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.
?x and [<Struct>] ?x optional parameters
Covered casesConverts
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.
This comment has been minimized.
This comment has been minimized.
|
🔍 Tooling Safety Check — Affects-Build-Infra
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Several edge cases currently produce uncompilable, semantics-changing, or comment-losing edits.
Review effort: Balanced
Findings: 2
Open (6)
Unqualified destination binding can resolve to user code · New Rewrites callers in projects lacking F# 10 support · New Misses explicitly targeted Struct attributes · New Renames functions when option argument is not final · New Generic calls are missed during call-site rewriting · New Conversion removes comments between function and argument · New
What changed in this PR
Adds an F# 10 IDE refactoring that converts optional member parameters between option and voption, updating compatible body usages and solution call sites.
Changes:
- Adds the conversion provider and safety checks.
- Adds tests, localized action resources, and project wiring.
- Documents the feature in Visual Studio release notes.
| File | Description |
|---|---|
vsintegration/src/FSharp.Editor/Refactor/ConvertOptionalParameterStruct.fs |
Implements the refactoring. |
vsintegration/src/FSharp.Editor/FSharp.Editor.fsproj |
Includes the provider. |
vsintegration/src/FSharp.Editor/FSharp.Editor.resx |
Adds action titles. |
vsintegration/src/FSharp.Editor/xlf/FSharp.Editor.cs.xlf |
Adds Czech resources. |
vsintegration/src/FSharp.Editor/xlf/FSharp.Editor.de.xlf |
Adds German resources. |
vsintegration/src/FSharp.Editor/xlf/FSharp.Editor.es.xlf |
Adds Spanish resources. |
vsintegration/src/FSharp.Editor/xlf/FSharp.Editor.fr.xlf |
Adds French resources. |
vsintegration/src/FSharp.Editor/xlf/FSharp.Editor.it.xlf |
Adds Italian resources. |
vsintegration/src/FSharp.Editor/xlf/FSharp.Editor.ja.xlf |
Adds Japanese resources. |
vsintegration/src/FSharp.Editor/xlf/FSharp.Editor.ko.xlf |
Adds Korean resources. |
vsintegration/src/FSharp.Editor/xlf/FSharp.Editor.pl.xlf |
Adds Polish resources. |
vsintegration/src/FSharp.Editor/xlf/FSharp.Editor.pt-BR.xlf |
Adds Portuguese resources. |
vsintegration/src/FSharp.Editor/xlf/FSharp.Editor.ru.xlf |
Adds Russian resources. |
vsintegration/src/FSharp.Editor/xlf/FSharp.Editor.tr.xlf |
Adds Turkish resources. |
vsintegration/src/FSharp.Editor/xlf/FSharp.Editor.zh-Hans.xlf |
Adds Simplified Chinese resources. |
vsintegration/src/FSharp.Editor/xlf/FSharp.Editor.zh-Hant.xlf |
Adds Traditional Chinese resources. |
vsintegration/tests/FSharp.Editor.Tests/Refactors/ConvertOptionalParameterStructTests.fs |
Tests conversions and exclusions. |
vsintegration/tests/FSharp.Editor.Tests/FSharp.Editor.Tests.fsproj |
Includes the tests. |
docs/release-notes/.VisualStudio/18.vNext.md |
Documents the refactoring. |
| match isStruct, ident.idText with | ||
| | false, "defaultArg" -> "defaultValueArg" | ||
| | true, "defaultValueArg" -> "defaultArg" | ||
| | false, name -> "Value" + name | ||
| | true, name -> name.Substring "Value".Length |
There was a problem hiding this comment.
Fixed in eea18fa: every name the conversion writes (ValueSome/Some, ValueOption.*/Option.*, defaultValueArg/defaultArg) is now resolved at the place where it is written. If it can mean anything other than FSharp.Core there, the member body is not offered the conversion, and a call site keeps its value behind ValueOption.ofOption/toOption, written as Microsoft.FSharp.Core.ValueOption.… when that module is shadowed too. Tests: No action (local module ValueOption, local defaultValueArg) and Optional arguments at call sites follow the conversion.
| for documentId, documentUses in usesByDocument do | ||
| let useDocument = solution.GetDocument documentId | ||
| let! text = useDocument.GetTextAsync cancellationToken | ||
|
|
||
| let! useParseResults, useCheckResults = |
There was a problem hiding this comment.
Fixed in eea18fa: converting to a value option now needs F# 10 in the declaring project and in every F# project that depends on it, so the action is not offered in a mixed-version solution. The callers are only known after find-references, so the check covers all dependent projects, not only the ones with call sites. Test: Value option is offered when the dependent projects have F# 10.
| let private isStructAttribute (attribute: SynAttribute) = | ||
| attribute.Target.IsNone | ||
| && match List.tryLast attribute.TypeName.LongIdent with | ||
| | Some name -> hasText "Struct" name || hasText "StructAttribute" name | ||
| | None -> false |
There was a problem hiding this comment.
Fixed in eea18fa: the attribute is recognised whatever its target, so [<param: Struct>] ?x is a value option parameter and converts back to ?x. Test: Struct attribute with a target marks a value option parameter.
| | _, SyntaxNode.SynExpr(SynExpr.App(isInfix = false; funcExpr = func; argExpr = arg)) :: _ when isSame arg node -> | ||
| tryModuleQualifier isFSharpCore isStruct func |
There was a problem hiding this comment.
Fixed in eea18fa: each supported module function now carries its argument count, and its qualifier is renamed only when the parameter is the last argument, applied directly or piped. Test: No action (the Option.defaultValue x (Some None) case).
| | SyntaxNode.SynExpr(SynExpr.App(isInfix = false; funcExpr = func; argExpr = arguments)) when | ||
| Position.posEq func.Range.End useRange.End |
There was a problem hiding this comment.
Fixed in eea18fa: a call site is now matched with or without its type arguments. The static form C.M<int>(?x = …) was already rewritten; the instance form c.M<int>(?x = …) was not, and now is. (C.M)(?x = …) does not parse (FS0734), so parentheses need no handling. Test: Optional arguments at call sites follow the conversion (both forms).
| | SynExpr.LongIdent(longDotId = SynLongIdent(id = [ qualifier; name ])), SynExpr.App(isInfix = false; argExpr = inner) when | ||
| hasText "ValueOption" qualifier | ||
| && hasText inverse name | ||
| && isFSharpCore (optionModule true) [ qualifier; name ] | ||
| -> | ||
| [ TextChange(valueSpan, sourceText.ToString(spanOf sourceText inner.Range)) ] |
There was a problem hiding this comment.
Fixed in eea18fa: only the conversion function and the whitespace after it are removed, so ValueOption.ofOption (* keep *) x becomes (* keep *) x. Test: Optional arguments at call sites follow the conversion.
abonie
left a comment
There was a problem hiding this comment.
🤖🕵️ AI review — verify independently.
| || mfv.IsDispatchSlot | ||
| || mfv.IsConstructor | ||
| || mfv.IsExtensionMember | ||
| ) |
There was a problem hiding this comment.
🤖🕵️ Converting the first ?x creates the second overload’s exact signature, producing FS0438. Check for destination-signature collisions before offering the action; #20547 has the same gap.
type C() =
static member M(?x: int) = defaultArg x 0
static member M([<Struct>] ?x: int) = defaultValueArg x 1There was a problem hiding this comment.
Fixed in eea18fa: no action when another overload of the member already has the signature the conversion would produce. The overloads are compared parameter by parameter, with their type parameters matched by position. Tests: No action (this repro from both overloads, and a generic pair); Overload that differs in another parameter does not block the conversion covers the opposite case.
f01c6c3 to
acbdfd3
Compare
abonie
left a comment
There was a problem hiding this comment.
🤖🕵️ AI review — verify independently.
| ValueSome(head :: heads) | ||
| else | ||
| ValueNone | ||
| | ValueSome heads, (SynPat.Wild _ | SynPat.Named _) -> ValueSome heads |
There was a problem hiding this comment.
🤖🕵️ Named match binder lets the changed representation escape validation: conversion changes x to voption, but leaves Option.defaultValue 0 y untouched and produces FS0001. Follow the bound symbol’s uses or reject this transfer.
type C() =
static member M(?x: int) =
match x with
| y -> Option.defaultValue 0 yThere was a problem hiding this comment.
Fixed in eea18fa: a match clause that binds the parameter to a name is rejected; only the option cases and _ are accepted as clause heads. Test: No action (this repro).
| |> List.distinctBy _.Span | ||
| |> List.sortBy _.Span.Start | ||
|
|
||
| solution <- solution.WithDocumentText(documentId, text.WithChanges changes) |
There was a problem hiding this comment.
🤖🕵️ Nested call edits overlap and throw ArgumentException: simplifying the outer ValueOption.toOption replaces a span containing the inner Some edit. Compose nested rewrites before applying SourceText.WithChanges.
type C() =
static member M(?x: int) = defaultArg x 0
let result =
C.M(?x = ValueOption.toOption (ValueSome (C.M(?x = Some 1))))There was a problem hiding this comment.
Fixed in eea18fa: dropping the outer ValueOption.toOption now removes only the function, not the whole argument, so it no longer overlaps the edit of the nested call. Test: Optional arguments at call sites follow the conversion (this repro).
acbdfd3 to
fd24fc7
Compare
| CodeAction.Create( | ||
| title, | ||
| Func<CancellationToken, Task<Solution>>(fun cancellationToken -> | ||
| CancellableTask.start cancellationToken changedSolution), |
There was a problem hiding this comment.
🤖 🕵️ [P2] Recreate the computation for each action invocation
Canceling a preview during reference discovery also cancels a later application of the same action, even with an uncanceled application token. The callback restarts the captured changedSolution, but CancellableTaskBuilder.Run retains its mutable state machine on the default scheduler, so the second invocation resumes the first invocation's canceled awaiter.
module M
type C() =
static member M(?x: int) = defaultArg x 0
let result = C.M(x = 1)A focused probe loading the unmodified provider and CancellableTasks.fs, with a controlled reference-discovery gate and Roslyn's preview/application APIs, reproduced this under both optimization settings: application was canceled with the preview token and reference discovery started only once. Changing changedSolution to a factory invoked inside this callback made application succeed and discovery start twice. Create a fresh computation per invocation, or make the shared builder safely restartable.
There was a problem hiding this comment.
Fixed in eea18fa: changedSolution is now a function called inside the callback, so every invocation of the action builds its own computation. Test: Action applies after an invocation that was cancelled; it failed with TaskCanceledException before the change.
| tryModuleQualifier isFSharpCore isStruct func | ||
| | _, | ||
| SyntaxNode.SynExpr(SynExpr.App(isInfix = true; funcExpr = pipe; argExpr = arg)) :: SyntaxNode.SynExpr(SynExpr.App( | ||
| isInfix = false; argExpr = func)) :: _ when isSame arg node && isOperator "op_PipeRight" pipe -> |
There was a problem hiding this comment.
🤖 🕵️ [P2] Resolve the pipe operator before accepting this use
This checks op_PipeRight only by spelling. A locally defined pipe can require an option even when the continuation resolves to the genuine FSharp.Core Option.defaultValue:
module M
let (|>) (x: int option) f = f x
type C() =
static member M(?x: int) = x |> Option.defaultValue 0
let result = C.M(x = 1)The original type-checks, but the current helpers offer the action and produce M([<Struct>] ?x: int) = x |> ValueOption.defaultValue 0, yielding FS0001 for both the operand and continuation. I reproduced the exact edits and errors with the current helper source. Verify that the source pipe resolves to FSharp.Core before allowing this branch; checking only the option function does not establish that the surrounding use preserves its type.
There was a problem hiding this comment.
Fixed in eea18fa: the piped use is accepted only when |> resolves to the FSharp.Core operator. Test: No action (this repro).
fd24fc7 to
6080188
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 22 new code comment lines and 2176 new code comment chars.
- Comments bypass the type system and become stale maintenance state. Keep only durable public contracts; express implementation intent through names, functions, active patterns, discriminated unions, and types.
?x and [<Struct>] ?x give the member body an option or a value option. The refactoring toggles the attribute, renames what keeps its type in the body (defaultArg, Option functions that do not return an option, match cases on the parameter) and rewrites ?x = value arguments at the call sites in the solution. Bodies that pass the parameter on or map it are refused. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
Some/None, defaultArg and Option.* in the member body are renamed only when they resolve to FSharp.Core; a shadowed name leaves the refactoring off. At call sites, a value built with a shadowed Some goes through ValueOption.ofOption instead of being renamed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review findings on the refactoring between `?x` and `[<Struct>] ?x`: - The names the conversion writes (`ValueSome`, `ValueOption.isSome`, `defaultValueArg` and their option counterparts) are checked where they are written: when user code shadows one, the member body is not offered the conversion, a call site keeps its value behind a conversion function, and that function is written with its full FSharp.Core name. - Converting to a value option needs F# 10 in every F# project that depends on the declaring one, because their call sites are rewritten too. - `[<param: Struct>] ?x` is a value option parameter, as it is for the compiler. - An option module function is renamed only when the parameter is its last argument, and `|>` only counts when it is the FSharp.Core operator. - A match clause that binds the parameter to a new name is not converted. - No action when another overload already has the converted signature. - A call of an instance member with explicit type arguments is rewritten. - Dropping `ValueOption.ofOption` at a call site keeps the comment after it and no longer overlaps the edits of a call nested in its argument. - The solution change is computed anew for every invocation of the action, so an invocation that was cancelled does not cancel the next one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
f176a68 to
ed9ca77
Compare
The doc comments of ConvertOptionalParameterStruct.fs move into names: resolvesOnlyToFSharpCore, arityOfOptionKindIndependentFunction, tryEnclosingMember, tryRenamesOfUse, tryRenamesOfEveryUse, calleeEndsAt, isUseInMemberBody, otherParametersToOwnTypes, ownAsListedByEntity and projectAndDependentsSupportValueOptions. Only the license header stays. ComputeRefactoringsAsync nested eight levels of control flow. The action is now built by tryCreateAction and the solution by changedSolution, so the provider nests three. Repeated pieces are merged: tryGetSymbolUse serves the provider and isFSharpCoreSymbol, isConvertibleCase names the check two call sites spelled out, Range.rangeContainsPos replaces a hand-written position comparison and SingleIdent replaces isOperator. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The module header comes from one helper instead of being written into 45 strings. The six one-off conversion facts become named rows of the two conversion theories, and every row of the three theories gets a name, so a failure says which scenario it was. The caret is the first question mark of the source unless a row needs another marker. Every input and every expected result is kept: 36 cases before and after. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Commits d40c554 (source) and c9643f6 (tests) apply the compaction: added comment lines 22 -> 1 and comment characters 2176 -> 123, the one left being the license header, with the explanations now carried by names such as |
… commenting them The doc comments of ConvertOptionalParameterDefaultValue.fs move into names: the Form cases FSharpOptional and DotNetOptional, fsharpCoreNameAt, IdentsAndLast, tryAnnotatedParameter, hasOnlyOptionalAttributes, tryShadowingLineSpan, whiteSpaceBeforeBody, passesMissingValueWhenOmitted, isUseInMemberBody and projectAndDependentsSupportStructOptionals. Only the license header stays. ComputeRefactoringsAsync nested nine levels of control flow. The actions are now built by actionsFor and the solution by changedSolution, so the provider nests three. tryForm and tryParameter go from four levels to two through tryDotNetOptional and searchResultAt. Repeated pieces are merged: tryGetSymbolUse serves the provider and isFSharpCoreSymbol, documentAction builds the two document actions, Range.rangeContainsPos replaces hand-written position comparisons and SingleIdent replaces isOperator. tryGetSymbolUse, isInFSharpCore and isFSharpCoreSymbol are the same text as in ConvertOptionalParameterStruct.fs of the optional parameter struct refactoring (PR dotnet#20546); the two pull requests keep their own copies. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Port of the dotnet#20546 compaction (feature/optional-parameter-struct-refactoring ed9ca77..c9643f6) to all-fixes: - Name the optional parameter conversion steps instead of commenting them - Parameterize the optional parameter conversion tests The refactoring's member lookup is the one call site of getSymbolUses; it keeps the RelatedSymbolUseKind.AllInCode argument that all-fixes needs. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Port of the dotnet#20546 compaction (feature/optional-parameter-struct-refactoring ed9ca77..c9643f6) to all-fixes: - Name the optional parameter conversion steps instead of commenting them - Parameterize the optional parameter conversion tests The refactoring's member lookup is the one call site of getSymbolUses; it keeps the RelatedSymbolUseKind.AllInCode argument that all-fixes needs. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Port of the dotnet#20546 compaction (feature/optional-parameter-struct-refactoring ed9ca77..c9643f6) to all-fixes: - Name the optional parameter conversion steps instead of commenting them - Parameterize the optional parameter conversion tests The refactoring's member lookup is the one call site of getSymbolUses; it keeps the RelatedSymbolUseKind.AllInCode argument that all-fixes needs. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Port of the dotnet#20546 compaction (feature/optional-parameter-struct-refactoring ed9ca77..c9643f6) to all-fixes: - Name the optional parameter conversion steps instead of commenting them - Parameterize the optional parameter conversion tests The refactoring's member lookup is the one call site of getSymbolUses; it keeps the RelatedSymbolUseKind.AllInCode argument that all-fixes needs. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>



Offers
Ctrl+.on a member's optional parameter to switch it betweenoptionandvoption(F# 10).defaultArg,Optionfunctions andmatchcases on it in the body, and?x = valuearguments at call sites across the solution, are rewritten to match.Not offered when a use of the parameter would change type (passed on,
Option.map), for overrides, abstract slots and constructors, or when the file has a paired.fsi.🤖 Generated with Claude Code