Don't find witnesses for typars with conditional constraints

[Smaug123]

Description

Cope with conditional constraints in witness passing. See extensive inline comments for the reasoning.

The initial three commits were created by Claude Opus 4.5. I then reviewed it, in the process adding some fairly extensive docstrings (all docstrings and comments in this PR come from me, except the big one in the test file). My understanding of the witness generation code is still fairly limited, and I was relying on Opus to point me to the right places to understand it; I believe my mental model is coherent, but I'm still not completely sure that it's accurate.

If you would like to simply close this without review on grounds of LLM generation by a non-domain-expert then go for it, although I don't think I could do any better with my human brain.

Fixes #19118

For reference, additional context I gave Claude as well as the text of #19118

Here is the stack trace at the point where we set the "supports comparison" constraint on the type, which we eventually try to find a witness for (and apparently fail), if that helps; but I don't know where the bug actually comes from.

🔴 TypedTree.Typar.SetConstraints() in FSharp.Compiler, FSharp.Compiler.Service.dll
◻ [email protected]() in FSharp.Compiler, FSharp.Compiler.Service.dll
◻ ListModule.loop@248-3<FSharp.Compiler.TypedTree.Typar, FSharp.Compiler.TypedTree.Typar>() in Microsoft.FShar...
◻ ListModule.Iterate2<FSharp.Compiler.TypedTree.Typar, FSharp.Compiler.TypedTree.Typar>() in Microsoft.FShar...
◻ TypeHierarchy.FixupNewTypars() in FSharp.Compiler, FSharp.Compiler.Service.dll
◻ ConstraintSolver.FreshenAndFixupTypars() in FSharp.Compiler, FSharp.Compiler.Service.dll
◻ ConstraintSolver.FreshenTypeInst() in FSharp.Compiler, FSharp.Compiler.Service.dll
◻ ConstraintSolver.CodegenWitnessesForTyparInst() in FSharp.Compiler, FSharp.Compiler.Service.dll
🔴 FSharpExprConvert.GetWitnessArgs() in FSharp.Compiler.Symbols, FSharp.Compiler.Service.dll
◻ FSharpExprConvert.ConvModuleValueOrMemberUseLinear() in FSharp.Compiler.Symbols, FSharp.Compiler.Service.dll
◻ [email protected]() in FSharp.Compiler.Symbols, FSharp.Compiler.Service.dll
◻ FSharpExpr.get_E() in FSharp.Compiler.Symbols, FSharp.Compiler.Service.dll
◻ FSharpExpr.get_ImmediateSubExpressions() in FSharp.Compiler.Symbols, FSharp.Compiler.Service.dll
◻ [email protected]() in CompilerDirectives, FSharp.Compiler.ComponentTests.dll [2]
◻ [email protected]() in CompilerDirectives, FSharp.Compiler.ComponentTests.dll [1]
◻ Line.demo of [email protected]() in CompilerDirectives, FSharp.Compiler.ComponentTests.dll

Checklist

• Test cases added
• Performance benchmarks added in case of performance changes
• Release notes entry updated:
  I believe release notes aren't necessary for this change, since it's purely a bugfix. I don't seem to be able to add labels, though?

[github-actions]

❗ Release notes required

@Smaug123,

Caution

No release notes found for the changed paths (see table below).

Please make sure to add an entry with an informative description of the change as well as link to this pull request, issue and language suggestion if applicable. Release notes for this repository are based on Keep A Changelog format.

The following format is recommended for this repository:

* <Informative description>. ([PR #XXXXX](https://github.com/dotnet/fsharp/pull/XXXXX))

See examples in the files, listed in the table below or in th full documentation at https://fsharp.github.io/fsharp-compiler-docs/release-notes/About.html.

If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request.

You can open this PR in browser to add release notes: open in github.dev

Change path	Release notes path	Description
src/Compiler	docs/release-notes/.FSharp.Compiler.Service/10.0.200.md	No release notes found or release notes format is not correct

[Smaug123]

I think this change doesn't need release notes; it's a pure bugfix.

[Smaug123]

As Opus says, I believe correctly:

So phantom type parameters cannot have ComparisonConditionalOn = true - they're never visited by checkIfFieldTypeSupportsComparison because they don't appear in any field.

Indeed, the code contains this:

match tryDestTyparTy g ty with
| ValueSome tp when tp |> HasConstraint _.IsSupportsComparison -> true
| ValueSome tp ->                    
                    // Within structural types, type parameters can be optimistically assumed to have comparison
                    // We record the ones for which we have made this assumption.
                    if tycon.TyparsNoRange |> List.exists (fun tp2 -> typarRefEq tp tp2) then 
                        assumedTyparsAcc <- assumedTyparsAcc.Add(tp.Stamp)
                        true                    
                    else
                        false

and this:

                            // Check the structural dependencies
                            (tinst, tcref.TyparsNoRange) ||> List.lengthsEqAndForall2 (fun ty tp -> 
                                if tp.ComparisonConditionalOn || assumedTypars.Contains tp.Stamp then 
                                    checkIfFieldTypeSupportsComparison tycon ty 
                                else 
                                    true) 

[Smaug123]

This assertion is very hard for me to verify; proving a negative is difficult.

[Smaug123]

I guess we could check up front for this case rather than catching the error - that might be nicer and more precise, but would incur the check much more frequently.

[T-Gro]

Does it affect codegen (in the style EmittedIlTests ) or behavior of the witness at runtime?

I haven't search for the most appropriate test suite for exercising witness behavior, but I do not trust my "by reading code" judgement for witnesses and would prefer a demonstration via tests that acutally make use of the witness (via quotations), to make sure things still work at runtime.

Apart from that, nicely done! 👍

[Smaug123]

Just to check - so you'd like some snapshots of the emitted IL, including calls to the comparison methods, like in https://github.com/dotnet/fsharp/blob/6ef4403f32ecc35634dc6d73c6e106d1c4866682/tests/FSharp.Compiler.ComponentTests/EmittedIL/TupleElimination.fs (for example), and I'll check that they haven't changed before and after this fix?

By "quotations" here - are you referring to just the fact that the F# code is in strings in e.g. the EmittedIL snapshot tests? Or did you have something else in mind?

[T-Gro]

Quotations are the reason witness methods exists.

Normally, all SRTP functions are inlined at place of usage, so their original definition would not have to be in the IL at all.

With Quotations, you can construct an expression programmatically and then attempt to evaluate it.

https://github.com/fsharp/fslang-design/blob/main/FSharp-5.0/FS-1071-witness-passing-quotations.md

Vanilla sample:

let inline negate x = -x
<@ negate 1.0 @>  |> eval

[Smaug123]

So I guess two main classes of test case: evaluate a quotation of an SRTP method that takes a parameter that gets statically resolved to the DU type, vs reflectively invoke the $W-named witness-taking version of that method? I guess also in the two cases of "generic parameter to the DU is specialised to something comparable" vs "not"? (Recording a prediction: my current mental model predicts that this PR doesn't change the behaviour in either case, because we weren't generating the witness before due to the same error we are catching and handling now.)

And these test cases should be simply behavioural evaluation of F# code, not assertions about the generated IL or the state of the compiler?

[T-Gro]

Yes, just eval it.
(eval will exercise the IL already - it is true that in this case we are not focused on the actual shape of the IL, since conditional constraints do not have an equivalent. But we care about not crashing)