Compiled ToStrings under -reflectionfree for DUs and Records - #19976
Conversation
Adds string_operator_info / mkCallStringOperator so generated code can call Operators.string. These lines are duplicated by the interpolated-string PR (dotnet#19971); kept identical there so a future merge resolves cleanly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Under --reflectionfree the union ToString previously emitted nothing, so DUs fell back to Object.ToString() (the namespace-qualified type name). Instead generate a match over the cases that builds "CaseName(f0, f1, ...)" using the 'string' operator on each field, via a TypedTree expression fed to CodeGenMethodForExpr. This recurses naturally into nested unions and is reflection-free. The default (sprintf "%+A") path is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The "concatenate a list of string exprs, picking the cheapest String.Concat overload by arity" pattern was duplicated in CheckExpressions (interpolation lowering) and the optimizer, and our new union ToString used the array overload unconditionally. Extract mkStringConcat into TypedTreeOps.ExprOps and route all three through it. This also lets single-field union cases emit Concat3 instead of allocating a string[] (IlxGen runs after the optimizer, so nothing else would collapse that array form). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The match-based ToString body is a TypedTree expression codegen'd via CodeGenMethodForExpr, but it was built with `eenv`, which lacks the tycon's type parameters. For generic unions this produced wrong IL: the wrong case branch (always the null-as-true-value case) or a NullReferenceException for single-case unions. Use `eenvinner` (the per-tycon environment) so the generic method body resolves its type parameters. The old sprintf path was unaffected because it emits raw IL off the pre-built ilThisTy. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
To make a generated union ToString consistent with how option/list format their contents (LanguagePrimitives.anyToStringShowingNull), format each field as: if (box field) is non-null then 'string field' else "null". Previously a null field rendered as "" (the 'string' operator's null behaviour). Generated inline rather than calling anyToStringShowingNull, which is internal to FSharp.Core and so not callable from user-compiled code. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Normalize union declarations to a leading '|', use System.Console.WriteLine instead of printfn (the printf machinery is what these changes move away from), and make the null-field test compare the union's rendering directly against option's rather than asserting a fixed string. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Result and Choice had no ToString override, so they fell back to the compiler-generated sprintf "%+A" one, which uses reflection. Give them hand-written overrides mirroring option/list (String.Concat + anyToStringShowingNull), e.g. Ok 5 -> "Ok(5)", Choice1Of2 7 -> "Choice1Of2(7)". This is reflection-free / AOT-friendly and consistent with option's "Some(x)" rendering. Note: this changes the observable ToString of Result/Choice from the "%A"-style "Ok 5" to "Ok(5)". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Records previously fell back to Object.ToString() (the namespace-qualified
type name) under --reflectionfree. Generate "{ F1 = v1; F2 = v2 }" on a single
line (no line breaks, unlike sprintf "%+A"), with fields formatted like union
fields (null -> "null", otherwise via 'string'). Factor the shared field
formatter and ToString-method emission out of the union path. The default
(sprintf "%+A") path is unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Result and Choice`2..7 now declare an explicit ToString() override, so they appear in the public surface area. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
❗ Release notes requiredYou can open this PR in browser to add release notes: open in github.dev
|
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Kickass |
This comment has been minimized.
This comment has been minimized.
…ionfree
Drive anonymous-record ToString through the synthetic record tycon (already
built for equality/comparison) rather than sprintf "%A", so under
--reflectionfree it renders "{| Name = value; ... |}" on a single line.
GenRecordToStringMethod now takes open/close brace strings ("{ "/" }" for
records, "{| "/" |}" for anonymous records). The default (non-reflection-free)
codegen path is unchanged and still falls back to sprintf "%+A".
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…free Addresses review feedback: generation is gated on `not (HasMember "ToString")`, so a user-defined ToString on a union or record wins over the generated one. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses review feedback: distinguish the reflective sprintf path from the structural one. GenPrintingMethod -> GenSprintfPrintingMethod (the sprintf "%+A" ToString/get_Message), GenToStringMethodFromExpr -> EmitToStringMethodDef. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Struct records and unions read fields off the this pointer and switch on the tag, and the anonymous record path is generated separately in IlxGen, so each gets its own baseline. The anonymous baseline omits the field reads: they name the anonymous type, whose mangled name is not stable across compilations. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The open/close braces carry inner spaces ("{| " and " |}"); with no
fields they abut and render "{| |}". Trim the leading space when the
field list is empty, matching %A's "{| |}".
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Head branch was pushed to by a user without write access
|
@abonie is it my responsibility to get all the checks to pass? I see 2 failing checks and one stalled but I can't see the azure devops errors and the github error here seems like an infra problem not caused by this PR. |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
@charlesroddie I am afraid there is nothing you can do about the the check_release_notes step failing, it's a security/orchestration issue (that I might not be able to fix this week unfortunately). |
|
Can you not merge regardless? |
|
I think kerams is right actually, I was under the impression that this step is blocking. I see the CI failed on rerun, but it is a bit suspicious - does not seem related to the changes in this PR. Also that particular leg run over 2x as long as usual this time. |
|
/azp run |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
|
@charlesroddie can you resolve the conflict? If I do it, it will invalidate my approvals (past and future on this PR) |
|
🔍 Tooling Safety Check — Affects-Compiler-Output
|
|
@abonie updated |
…ollector Resolves the duplicated 'string' operator intrinsic: dotnet#19976 added an identical v_string_operator_info / mkCallStringOperator, so the two copies collapse into one.
dotnet#19976 extracted the arity-dispatched String.Concat builder into TypedTreeOps.ExprOps, so the interpolation lowering no longer needs its own copy. The only case it keeps is the lone possibly-null string, which has no Concat to map null to "". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This PR implements
ToStringon user-defined DUs and records, under the--reflectionfreeswitch, by delegating into the equivalent ofLanguagePrimitives.anyToStringShowingNull, as is currently used byOption. Previously only the type name was shown. The newToStringis AOT/trim friendly, provided that the inner types have AOT/trim-friendlyToStringmethods.Fixes fsharp/fslang-suggestions#919
Behavioural changes (subject to
--reflectionfree)ToStrings instead of just the type name. The behaviour matchesOptionfor the internal rendering, and this differs from non-reflection-free mode (sprintf "%+A") in the following ways:5.renders as5.sprintf "%A".{| ... |}syntax instead of{ ... }While the exact code of
Optioncouldn't be used directly (sinceanyToStringShowingNullis internal to FSharp.Core), it was easy to match it, and there are some tests that the behaviour is the same, so that if there are any changes to the above inOption(which there is some ongoing discussion about, e.g. overnullrendering), they are synced up with other DU non-reflection-based rendering.It is obviously easy to make the changes to
--reflectionfreemode a decisive improvement, since we are starting with nothing. However the intention would be that the behaviour is good enough to switch as a default in a future version of F#.Note: this PR and #19971 add an identical
v_string_operator_infoandmkCallStringOperator.