Compiled ToStrings under -reflectionfree for DUs and Records#19976
Compiled ToStrings under -reflectionfree for DUs and Records#19976charlesroddie wants to merge 26 commits into
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 |
|
🔍 Tooling Safety Check — Affects-Compiler-Output
|
…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>
|
|
||
| override x.ToString() = | ||
| match x with | ||
| | Choice1Of6 v -> String.Concat("Choice1Of6(", anyToStringShowingNull v, ")") |
There was a problem hiding this comment.
Wouldn't' the newly added IlxGen feature codegen the same code that was now added by hand?
Thinking out loud:
Maybe this could be a separate feature (which would gent turned on automatically if you had --reflectionfree), but we could apply it even without it - either by project or by type.. ?
There was a problem hiding this comment.
Yes it would, if you turned on --reflectionfree, which would then break FSharp.Core since --reflectionfree bans "%A" usage. A separate feature would work for this purpose, at the cost of more plumbing.
The bigger question is when will consumers (both FSharp.Core and end users) who don't set --reflectionfree get the compiled ToStrings in the future. Turning on the flag by default would be dangerous because of the %A ban.
Perhaps the options are:
- Introduce a new feature now, use it in FSharp.Core straight away, and in the future it could be on by default.
- Stick with --reflectionfree, and in future make it on by default but remove the %A ban in it at that point (with %A possibly getting safer in future and with trim/AOT warnings available from the regular toolchain). Once this happens these explicit overrides can be removed from FSharp.Core.
There was a problem hiding this comment.
How about this @T-Gro :
- Make a
LanguageFeature.CompiledToStrings FSharp.Coreopts into this and then instead of adding manualToStrings, this PR removes them fromOptionandValueOptionsince they are now auto-generated.- The implementation checks for either
LanguageFeature.CompiledToStringsor--reflectionfree
There was a problem hiding this comment.
I like that.
Just instead of a LanguageFeature, it shall be a dedicated compiler flag flowing via CompilerConfig.
LanguageFeatures go via a mapping from language version to sets of features, we cannot easily cherry pick one by one (unless this functionality is added).
CompilerConfig is better for dedicated flags that work orthogonal to versions of the language.
Can we still guarantee no breaking changes for regular , non reflectionfree , users of ToString() ?
There was a problem hiding this comment.
Happy to implement this. It's an extra flag that is hopefully temporary but presumably that's a small cost and it doesn't need to be publicized to end-users.
Used in FSharp.Core, the affected types will be:
Option: this will haveToStringremoved and output will be unchanged.ResultandChoice...types will go back to having no explicitToStrings.Ref<'T>is affected and will need to be tested in the same way asResultbut with record syntax.
Map and Set and List are unaffected since they have explicit ToStrings.
ValueOption has an explicit ToString but that should be removed since it's inconsistent: Some(0).ToString() gives "Some(0)" while ValueSome(0).ToString() gives "0" which should be corrected to "ValueSome(0)".
The behavioural changes for end-users will all be consistency improvements.
There was a problem hiding this comment.
OK good. Yes once done (in future work) the above FSharp.Core changes will make reflection-free codegen more useful, but it will also make anything that takes a string path more useful, including interpolated strings. E.g. currently $"{Ok 0}" does not give a useful result. So I'd classify it as a major bug at the moment, and I'll try to get a language suggestion out soon so we can start do deal with this.
There was a problem hiding this comment.
The FSharp.Core changes are reverted now.
There was a problem hiding this comment.
This thread resolved now @T-Gro ? Anything more to do in this PR?
There was a problem hiding this comment.
I created fsharp/fslang-suggestions#1468 which covers the points here.
There was a problem hiding this comment.
@charlesroddie : I will post a few more things I found when testing this.
Will also suggest a comparison table to make the differences clear (and possibly use them to suggest followup work in future PRs) , and also something to be used when communicating the feature.
…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>
Addresses review feedback: keep the column-aligned layout of the surrounding intrinsic table. Also makes these two lines byte-identical to the same intrinsic added by dotnet#19971, so a future merge resolves cleanly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…cords and recursion Covers DU field shapes (multiple fields vs a single tuple field), explicit vs unnamed field names rendering identically, struct unions/records, anonymous and struct anonymous records, and finite recursive/nesting types. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Locks in the IL emitted under --reflectionfree: each field is boxed and rendered through Operators.ToString with a null guard, and the parts are joined with String.Concat (array form for the record, 3-arg form for the single-field union case). Nullary union cases return the bare case name. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The structural ToString for --reflectionfree records and unions was built in IlxGen, after the optimizer, so its per-field 'string' operator calls were never inlined: each value-type field was boxed and rendered through the generic Operators.ToString, behind a null guard that is dead for a value type. Move the generation into the type-augmentation phase (alongside Equals/GetHashCode/CompareTo) so the body flows through the optimizer. The 'string' operator is now specialised - a value-type field renders via a direct, allocation-free invariant-culture ToString with no boxing and no null guard (reference fields keep the guard so null still renders as "null"). The shared body builders live in AugmentTypeDefinitions; anonymous record types are synthesized too late for augmentation, so they keep generating in IlxGen but reuse the same builder. Output is unchanged; the EmittedIL baselines are updated to the leaner IL. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
07df74b to
6a2199c
Compare
The augmentation-generated structural ToString recurses into fields, so a deeply nested value can exhaust the stack with an uncatchable StackOverflowException. Emit RuntimeHelpers.EnsureSufficientExecutionStack() at method entry (as C# records do in PrintMembers) so it throws a catchable InsufficientExecutionStackException instead, when the runtime provides the method. The guard is skipped for types whose every field is a flat primitive (integer/float/decimal/string/char/bool/unit/enum), which cannot recurse. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A 1,000,000-deep value's generated ToString throws a catchable InsufficientExecutionStackException rather than hard-crashing the process. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
cf70b83 to
e130e50
Compare
…ToString Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| let thisv, body = | ||
| AugmentTypeDefinitions.mkRecdToString (g, tcref, tcref.Deref, openBrace, closeBrace) | ||
|
|
||
| EmitToStringMethodDef(cenv, mgbuf, eenv, thisv, body) |
There was a problem hiding this comment.
Anon records skip the recursion guard that nominal records/unions get. mkRecdToString here bypasses MakeBindingsForToStringAugmentation, where mightRecurse prepends EnsureSufficientExecutionStack().
// --reflectionfree
let mutable o : obj = box 0
for _ in 1..2_000_000 do o <- box {| Next = o |}
printfn "%s" (o.ToString())Nominal { Next: obj } → catchable InsufficientExecutionStackException. Anon → hard Stack overflow., process abort (exit 134); the cycle is Operators.ToString ↔ AnonymousType.ToString(), no guarded nominal type in the loop. New crash vector vs. old reflectionfree (which emitted no anon ToString).
Suggestion: apply the same mightRecurse guard on this path.
| |> List.mapi (fun i fspec -> | ||
| let fref = tcref.MakeNestedRecdFieldRef fspec | ||
| let value = mkFieldToString (g, m, mkRecdFieldGetViaExprAddr (thise, fref, tinst, m)) | ||
| let nameEq = mkString g m (fspec.DisplayName + " = ") |
There was a problem hiding this comment.
DisplayName escapes special names with backticks; %A uses the raw name. Same at the union sites (lines 1759, 1773 ucase.DisplayName).
type U = ``My Case`` of int
// %A : My Case 5
// --reflectionfree : ``My Case``(5)Suggestion: use DisplayNameCore.
| string, | ||
| string) | ||
| IL_0038: ret | ||
| }"""] |
There was a problem hiding this comment.
Baselines only cover a ref record + ref union. Add:
- a
[<Struct>]record/union baseline (struct field-get path differs) - an anonymous-record baseline (separate
GenRecordToStringMethodcodegen path — and the one missing the recursion guard, see IlxGen comment)
| * Debug: rework for expressions stepping ([PR #19894](https://github.com/dotnet/fsharp/pull/19894)) | ||
| * Debug: rework conditional erasure, fix stepping over literals ([PR #19897](https://github.com/dotnet/fsharp/pull/19897)) | ||
| * Debug: fix if and match condition sequence points ([PR #19932](https://github.com/dotnet/fsharp/pull/19932)) | ||
| * Under `--reflectionfree`, discriminated unions and records now get a generated `ToString` (rendering each field like `Option` does) instead of falling back to the namespace-qualified type name. ([PR #19976](https://github.com/dotnet/fsharp/pull/19976)) |
There was a problem hiding this comment.
The --reflectionfree rendering diverges from %+A in enough ways to be worth documenting/announcing as acceptance wording. Not release-note material — suggest a durable docs/reflectionfree-printing.md this note can link to. Also currently omits anonymous records.
| F# | %+A (default) |
--reflectionfree |
|---|---|---|
{ X = 1; Y = 2 } |
{ X = 1⏎ Y = 2 } |
{ X = 1; Y = 2 } |
B 5 |
B 5 |
B(5) |
C (3, "hi") |
C (3, "hi") |
C(3, hi) |
Wrap "hi" |
Wrap "hi" |
Wrap(hi) |
| char field | 'a' |
a |
{ P = None } |
None |
null |
{ A = [|1;2;3|] } |
[|1; 2; 3|] |
System.Int32[] |
{ V = ValueSome 2 } |
ValueSome 2 |
2 |
U () |
U () |
U(null) |
5.0 |
5.0 |
5 |
250uy / 42n / 1.5M |
250uy / 42n / 1.5M |
250 / 42 / 1.5 |
[<StructuredFormatDisplay("Custom<{X}>")>] |
Custom<5> |
{ X = 5 } (ignored) |
``My Case`` 5 |
My Case 5 |
``My Case``(5) (see AugmentWithHashCompare comment) |
There was a problem hiding this comment.
It could go to https://learn.microsoft.com/en-us/dotnet/fsharp/language-reference/plaintext-formatting#a-formatting to communicate the differences.
I also want the table in this PR so that people can come in and express how acceptable the differences are (vs building on top of this PR and replicating reflection-based %A in codegen by switching over field type).
(pinging people who interacted with this PR: @kerams , @ShalokShalom , @smoothdeveloper , @jwosty )
There was a problem hiding this comment.
Wrap(hi)
I think quotes should be preserved around strings so that there is no ambiguity without consulting the type's shape.
System.Int32[]
If we had something like tostring levels, then we could choose how complex the emission should be (should ToString generate code to iterate over a portion of any collection?). Then again, with --reflectionfree people supposedly care more about AOT than these strings, and one can always write a custom ToString, so maybe adding levels to this is needless complexity.
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.It also implements
ToStringonResultandChoicetypes, and consolidates behaviour to matchOption.Fixes fsharp/fslang-suggestions#919
Behavioural changes (ungated)
ToStrings on FSharp.Core types are being discussed in a thread below.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.