Repository navigation
Add a refactoring between F# and .NET-compatible optional parameters - #20547
xperiandri wants to merge 9 commits into
Conversation
✅ Release notes checked
|
This comment has been minimized.
This comment has been minimized.
Covered casesConverts
Not offered
|
T-Gro
left a comment
There was a problem hiding this comment.
🤖 🕵️ AI review — verify independently.
| let defaultText = | ||
| match defaultValue with | ||
| | ValueSome value -> sourceText.ToString(spanOf sourceText value.Range) | ||
| | ValueNone -> "Unchecked.defaultof<_>" |
There was a problem hiding this comment.
🤖 🕵️ [P2] Converting x to F# ?x changes C.M() from true to false — an omitted obj receives null instead of Missing.Value.
open System.Runtime.InteropServices
type C() =
static member M([<Optional>] x: obj) =
obj.ReferenceEquals(x, System.Reflection.Missing.Value)
printfn "%b" (C.M())There was a problem hiding this comment.
Fixed in 6a6a694: for an F# member, [<Optional>] without DefaultParameterValue passes Missing.Value only when the parameter type is obj (infos.fs, isObjTyAnyNullness). Every other type gets DefaultValue, which matches Unchecked.defaultof. So the conversion is no longer offered when the parameter's type, with abbreviations stripped, is System.Object. Test: the [<Optional>] x: obj case of No action.
| | Some(node, | ||
| SyntaxNode.SynExpr(SynExpr.App(isInfix = false; funcExpr = SingleIdent func; argExpr = arg) as inner) :: SyntaxNode.SynExpr(SynExpr.App( | ||
| isInfix = false; funcExpr = funcExpr; argExpr = (SynExpr.Const _ as defaultValue)) as application) :: rest) when | ||
| isSame arg node && hasText "defaultArg" func && isSame funcExpr inner |
There was a problem hiding this comment.
🤖 🕵️ [P2] Converting ?x to .NET optional changes C.M() from 99 to 0 — matching defaultArg by spelling removes a call to a different function.
let defaultArg (_: int option) (_: int) = 99
type C() =
static member M(?x: int) = defaultArg x 0
printfn "%d" (C.M())There was a problem hiding this comment.
| match found, attributeText with | ||
| | ValueSome found, ValueSome attributeText -> | ||
| [ | ||
| TextChange(TextSpan((spanOf sourceText optionalValRange).Start, 1), attributeText) |
There was a problem hiding this comment.
🤖 🕵️ [P2] Converting only ?y to .NET optional produces FS1212 — the remaining F# optional ?x precedes the converted parameter.
type C() =
static member M(?x: int, ?y: int) =
defaultArg x 1 + defaultArg y 2There was a problem hiding this comment.
Fixed in 6a6a694: the .NET form is no longer offered when a ? parameter comes before the target in the same member. Test: the M(?x: int, ?y: int) case of No action.
|
|
||
| let struct (prefix, defaultFunction) = | ||
| if asStruct then | ||
| struct ("[<Struct>] ?", "defaultValueArg") |
There was a problem hiding this comment.
🤖 🕵️ [P2] Converting x to [<Struct>] ?x leaves a formerly valid caller failing with FS0001 — Some 1 supplies option, but the parameter now requires voption.
open System.Runtime.InteropServices
type C =
static member M([<Optional; DefaultParameterValue(0)>] x: int) = x
let result = C.M(?x = Some 1)There was a problem hiding this comment.
Fixed in 6a6a694 by rewriting the callers, as #20546 does. Deciding not to offer it would need the same solution-wide reference search at lightbulb time; rewriting does that search only when the action runs. The struct action now changes the solution. ?x = Some v/None become ValueSome v/ValueNone when they are the FSharp.Core cases, and any other value is wrapped in ValueOption.ofOption. Test: Converting to a struct optional parameter converts optional arguments to value options.
| /// The whole line of `let x = defaultArg x c` when that line does nothing but rebind the parameter. | ||
| let private tryShadowingLine (sourceText: SourceText) (name: string) (application: SynExpr) (path: SyntaxVisitorPath) = | ||
| match path with | ||
| | SyntaxNode.SynBinding(SynBinding(headPat = SynPat.Named(ident = SynIdent(ident, _)); expr = rhs; trivia = trivia)) :: SyntaxNode.SynExpr(SynExpr.LetOrUse letOrUse) :: _ when |
There was a problem hiding this comment.
🤖 🕵️ [P2] Converting ?x to .NET optional deletes let mutable x, leaving x <- x + 1 failing with FS0027 because the parameter is immutable.
type C() =
static member M(?x: int) =
let mutable x = defaultArg x 0
x <- x + 1
xThere was a problem hiding this comment.
Fixed in 6a6a694: a let mutable shadow is no longer removed. It becomes let mutable x = x. Test: Mutable shadowing keeps its binding.
| found | ||
| || match node with | ||
| | SyntaxNode.SynModule(SynModuleDecl.Open(target = SynOpenDeclTarget.ModuleOrNamespace(longId = SynLongIdent(id = ids)))) -> | ||
| (ids |> List.map _.idText) = interopServices |
There was a problem hiding this comment.
🤖 🕵️ [P2] Converting ?x to .NET optional produces FS0039 for both attributes — an open in another module suppresses the import needed at C.M.
module Other =
open System.Runtime.InteropServices
let x = 1
type C() =
static member M(?x: int) = defaultArg x 0There was a problem hiding this comment.
Fixed in 6a6a694: an open System.Runtime.InteropServices counts only when it ends before the member, inside a module or namespace that encloses the member. Otherwise the open is inserted. Tests: Open in another module does not count as in scope, Open in an enclosing module counts as in scope.
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.
abonie
left a comment
There was a problem hiding this comment.
🤖🕵️ AI review — verify independently.
| (List.toArray interopServices) | ||
| FSharp.Compiler.EditorServices.OpenStatementInsertionPoint.TopLevel | ||
|
|
||
| OpenDeclarationHelper.insertOpenDeclaration text insertionContext (String.Join(".", interopServices)) |
There was a problem hiding this comment.
🤖🕵️ Inserting this import rebinds untouched CharSet.None, changing result from 123 to 1 while both versions compile. Fully qualify the generated attributes instead of adding a broad import.
// User.fs — preceding file
namespace User
type CharSet = | None = 123// Main.fs
module Main
open User
type C =
static member M(?x: int) = defaultArg x 7
let result = int CharSet.None
let omitted = C.M()There was a problem hiding this comment.
Fixed in 008e3f9: no open is inserted any more. The attributes are written as System.Runtime.InteropServices.Optional and System.Runtime.InteropServices.DefaultParameterValue, unless that namespace is already open at the member. Tests: Attributes are fully qualified where System.Runtime.InteropServices is not open and Open in another module does not count as in scope.
|
|
||
| let lists = | ||
| attributes | ||
| |> List.filter (fun list -> List.exists isOptionalAttribute list.Attributes) |
There was a problem hiding this comment.
🤖🕵️ Separate [<ParamArray>] list escapes validation and survives on the generated option/voption parameter. Both reverse actions break the previously valid omitted and packed calls with FS0501. Validate all parameter attributes, not only lists containing optional attributes.
open System
open System.Runtime.InteropServices
type C =
static member M([<ParamArray>] [<Optional>] x: int[]) =
if isNull x then -1 else x.Length
printfn "%A" [C.M(); C.M(1, 2)]There was a problem hiding this comment.
Fixed in 008e3f9: every attribute list of the parameter is checked now, and the parameter's symbol may carry no attribute other than Optional and DefaultParameterValue. Test: the [<ParamArray>] [<Optional>] case of No action.
| let changedSolution = | ||
| cancellableTask { | ||
| let! cancellationToken = CancellableTask.getCancellationToken () | ||
| let! memberUses = SymbolHelpers.getSymbolUses memberUse document checkResults |
There was a problem hiding this comment.
🤖🕵️ Fixed SRTP caller contract left incompatible: both reverse actions change the physical parameter type but retain int -> int, producing FS0001. The option/voption conversion in #20546 has the same failure.
open System.Runtime.InteropServices
type C =
static member M([<Optional; DefaultParameterValue(7)>] x: int) = x
let inline call (t: ^T) =
((^T) : (static member M : int -> int) 2)
printfn "%d" (call Unchecked.defaultof<C>)49e1086 to
891d0c5
Compare
abonie
left a comment
There was a problem hiding this comment.
🤖🕵️ AI review — verify independently.
| for struct (application, _, path) in found do | ||
| match tryShadowingLine sourceText parameter.Ident.idText application path with | ||
| | ValueSome line -> TextChange(line, "") | ||
| | ValueNone -> TextChange(spanOf sourceText application.Range, parameter.Ident.idText) |
There was a problem hiding this comment.
🤖🕵️ Escaped parameter spelling lost in generated expressions: forward conversion emits bare type, and reverse conversion emits let type = defaultArg type 7, producing syntax errors. Preserve or correctly escape the identifier in both directions.
open System.Runtime.InteropServices
type C =
static member M(?``type``: int) = defaultArg ``type`` 7
printfn "%d" (C.M())There was a problem hiding this comment.
Fixed in 008e3f9: the parameter name is taken from the source text in both directions, so its backticks stay. Test: Escaped parameter name keeps its backticks.
| (hasText "Some" head || hasText "None" head) | ||
| && isFSharpCore "Microsoft.FSharp.Core.FSharpOption`1" [ head ] | ||
| -> | ||
| [ TextChange(TextSpan((spanOf sourceText head.idRange).Start, 0), "Value") ] |
There was a problem hiding this comment.
🤖🕵️ Prefix insertion corrupts a valid escaped builtin case: the struct action generates Value``Some``, producing FS0039. Replace the complete identifier token rather than inserting text before its backticks.
open System.Runtime.InteropServices
type C =
static member M([<Optional; DefaultParameterValue(7)>] x: int) = x
printfn "%d" (C.M(?x = ``Some`` 2))There was a problem hiding this comment.
Fixed in 008e3f9: the whole identifier is replaced with ValueSome or ValueNone. Test: the Some case of Converting to a struct optional parameter converts optional arguments to value options.
|
|
||
| let mutable solution = document.Project.Solution | ||
|
|
||
| for documentId, documentUses in usesByDocument do |
There was a problem hiding this comment.
🤖🕵️ Declaring-project gate does not protect dependent callers: the struct action rewrites the F#9 caller to ValueSome 2, then it fails FS0001/FS0193 against the converted member. Check every affected project’s feature support; #20546 has the same gap.
// Project A: F#10
module A
open System.Runtime.InteropServices
type C =
static member M([<Optional; DefaultParameterValue(7)>] x: int) = x// Project B: F#9, references A
module B
let result = A.C.M(?x = Some 2)There was a problem hiding this comment.
Fixed in 008e3f9: the struct form is offered only when the member's project and every F# project that depends on it support value options as optional parameters. Test: Struct optional parameter is offered only when every dependent F# project supports it.
891d0c5 to
22767fd
Compare
22767fd to
90a557e
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 32 new code comment lines and 2999 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.
…meters ?x: T is optional for F# callers and gives the body an option; [<Optional; DefaultParameterValue(c)>] x: T is optional for C# and VB callers too and gives the body the value. The refactoring moves the default between defaultArg x c in the body and the attribute, adds the InteropServices open when it is missing, and leaves call sites alone: F# accepts both forms the same way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…form [<Optional; DefaultParameterValue(c)>] x: T now converts back to either ?x: T with let x = defaultArg x c, or, from F# 10, [<Struct>] ?x: T with let x = defaultValueArg x c. 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>
Resolve defaultArg by symbol, not spelling. Do not offer the F# form for an [<Optional>] obj without a default, whose omitted value is Missing.Value, nor the .NET form after an earlier ? parameter (FS1212). Keep a mutable shadowing binding, look for the InteropServices open only in modules that enclose the member, and rewrite ?x = option arguments when converting to a struct optional parameter. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… names Converting `[<Optional; DefaultParameterValue(c)>] x: T` to `?x: T`: - Read the attributes from the parameter's symbol: they must be the `OptionalAttribute` and at most the `DefaultParameterValueAttribute` of `System.Runtime.InteropServices`, and the parameter may have no other attribute, in whichever attribute list it is written. - Require the default constant to be of the parameter's type, as the compiler does before it passes one, and take that type from the symbol in both directions. - Do not offer it for a byref or byref-like parameter type, nor when a parameter that is not a `?` one follows (FS1212). - Write `defaultArg` and `defaultValueArg` with their full names where the short name resolves to something else, and recognise a qualified `defaultArg` on the way back. - Keep a comment between `=` and the body, and the backticks of an escaped parameter name. Converting to `[<Struct>] ?x: T`: - Offer it only when every F# project that depends on the member's project takes value options for optional parameters. - Replace the whole `Some` or `None` identifier at call sites, with the full name of the value option case where the short one is shadowed. Converting `?x: T` to the .NET form: - Write the attributes with their namespace instead of inserting `open System.Runtime.InteropServices`, unless it is open at the member. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
20e796b to
8e2d352
Compare
… 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>
The module header and the System.Runtime.InteropServices open come from two helpers instead of being written into every test string. The one-off conversion facts and the two struct conversion theories become rows of three named theories, every row of the theories gets a name so a failure says which scenario it was, and the comment about re-checking a synthetic project is the name of a helper, assertCompilesAsProject. Every input and every expected result is kept: 47 cases before and after. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Commits ddb5850 (source) and d52a564 (tests) apply the compaction: added comment lines 32 -> 1 and comment characters 2999 -> 123, the one left being the license header, with the explanations now carried by names such as |
T-Gro
left a comment
There was a problem hiding this comment.
🤖🕵️ AI review — verify independently.
| match found, attributes with | ||
| | ValueSome found, ValueSome attributes when not (hasOptionalValBefore optionalValRange.Start headPat) -> | ||
| let qualifier = | ||
| if hasInteropServicesOpenAt parameter.MemberName.idRange.Start parseTree then |
There was a problem hiding this comment.
🤖🕵️ Local attribute capture changes an omitted argument from 7 to 0.
open System.Runtime.InteropServices
type DefaultParameterValueAttribute(value: int) = inherit System.Attribute()
type C =
static member M(?x: int) = defaultArg x 7
let result = C.M()| match trivia.EqualsRange with | ||
| | Some equals when body.Range.StartLine > equals.EndLine -> | ||
| let line = sourceText.Lines[Line.toZ body.Range.StartLine] | ||
| ValueSome(TextChange(TextSpan(line.Start, 0), $"{String(' ', body.Range.StartColumn)}{declaration}{lineBreak}")) |
There was a problem hiding this comment.
🤖🕵️ Generated binding lands inside a block comment; the expression fails with FS0001/FS0043.
static member M([<Optional; DefaultParameterValue(7)>] x: int) =
(* first
second *) x + 1…form From dotnet#20547 (release note left out: 18.vNext.md has local edits). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… commenting them Port of the dotnet#20547 compaction (feature/optional-parameter-default-value-refactoring 8e2d352..d52a564) to all-fixes: - Name the optional parameter default value conversion steps instead of commenting them - Parameterize the optional parameter default value 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>
…form From dotnet#20547 (release note left out: 18.vNext.md has local edits). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… commenting them Port of the dotnet#20547 compaction (feature/optional-parameter-default-value-refactoring 8e2d352..d52a564) to all-fixes: - Name the optional parameter default value conversion steps instead of commenting them - Parameterize the optional parameter default value 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>
…form From dotnet#20547 (release note left out: 18.vNext.md has local edits). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… commenting them Port of the dotnet#20547 compaction (feature/optional-parameter-default-value-refactoring 8e2d352..d52a564) to all-fixes: - Name the optional parameter default value conversion steps instead of commenting them - Parameterize the optional parameter default value 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>
…form From dotnet#20547 (release note left out: 18.vNext.md has local edits). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… commenting them Port of the dotnet#20547 compaction (feature/optional-parameter-default-value-refactoring 8e2d352..d52a564) to all-fixes: - Name the optional parameter default value conversion steps instead of commenting them - Parameterize the optional parameter default value 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+.to switch between?x: TwithdefaultArg x cin the body and[<Optional; DefaultParameterValue(c)>] x: T, which C# and VB callers can omit too. Converting back offers?xwithdefaultArgand, from F# 10,[<Struct>] ?xwithdefaultValueArg. F# call sites compile against both forms, so only the member is edited.Not offered unless every use is
defaultArg x cwith one constant of the parameter's type, or for overrides, abstract slots, constructors, or a file with a paired.fsi.🤖 Generated with Claude Code