Conversation
❗ Release notes requiredCaution 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:
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
Warning No PR link found in some release notes, please consider adding it.
|
There was a problem hiding this comment.
Pull request overview
Adds preview support for resolving SRTP member constraints against .NET (IL) fields (C# classes/structs), including persistence of the solution in TAST pickling and codegen for trait witnesses.
Changes:
- Extend member-constraint solving to consider IL fields (gated by
/langversion:previewfeatureSupportILFieldsInSRTP) and record the solution as a newTraitConstraintSln.ILFieldSln. - Add witness generation for IL-field get/set using
TOp.ILAsminstructions. - Add component tests for C# field scenarios, update CompilerCompat sample, and add the language feature string + preview release note.
Reviewed changes
Copilot reviewed 41 out of 41 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/projects/CompilerCompat/CompilerCompatLib/Library.fs | Adds SRTP helpers/types intended to exercise member-constraint compatibility. |
| tests/projects/CompilerCompat/CompilerCompatApp/Program.fs | Runs additional SRTP compatibility checks in the compat app. |
| tests/FSharp.Compiler.ComponentTests/ConstraintSolver/MemberConstraints.fs | Adds component tests covering IL-field SRTP resolution (get/set, static/instance, readonly, etc.). |
| src/Compiler/Checking/ConstraintSolver.fs | Implements IL-field search/solution recording for SRTP member constraints (preview-gated). |
| src/Compiler/Checking/MethodCalls.fs | Generates ILAsm witness expressions for ILFieldSln solutions. |
| src/Compiler/TypedTree/TypedTreePickle.fs | Adds pickling/unpickling support for the new ILFieldSln solution case. |
| src/Compiler/TypedTree/TypedTreeOps.fs | Adds remapping + free-variable accumulation for ILFieldSln. |
| src/Compiler/TypedTree/TypedTree.fsi | Exposes TraitConstraintSln.ILFieldSln in the public TAST signature. |
| src/Compiler/TypedTree/TypedTree.fs | Implements TraitConstraintSln.ILFieldSln in the TAST. |
| src/Compiler/Symbols/SymbolHelpers.fs | Ensures XML-doc lookup logic handles ILFieldSln. |
| src/Compiler/Facilities/LanguageFeatures.fsi | Adds SupportILFieldsInSRTP language feature. |
| src/Compiler/Facilities/LanguageFeatures.fs | Wires SupportILFieldsInSRTP into preview and feature-string retrieval. |
| src/Compiler/FSStrings.resx | Adds a resource string for featureSupportILFieldsInSRTP. |
| src/Compiler/FSComp.txt | Adds the feature description string used by FSComp.SR.featureSupportILFieldsInSRTP. |
| src/Compiler/xlf/FSStrings.cs.xlf | Adds featureSupportILFieldsInSRTP localization entry. |
| src/Compiler/xlf/FSStrings.de.xlf | Adds featureSupportILFieldsInSRTP localization entry. |
| src/Compiler/xlf/FSStrings.es.xlf | Adds featureSupportILFieldsInSRTP localization entry. |
| src/Compiler/xlf/FSStrings.fr.xlf | Adds featureSupportILFieldsInSRTP localization entry. |
| src/Compiler/xlf/FSStrings.it.xlf | Adds featureSupportILFieldsInSRTP localization entry. |
| src/Compiler/xlf/FSStrings.ja.xlf | Adds featureSupportILFieldsInSRTP localization entry. |
| src/Compiler/xlf/FSStrings.ko.xlf | Adds featureSupportILFieldsInSRTP localization entry. |
| src/Compiler/xlf/FSStrings.pl.xlf | Adds featureSupportILFieldsInSRTP localization entry. |
| src/Compiler/xlf/FSStrings.pt-BR.xlf | Adds featureSupportILFieldsInSRTP localization entry. |
| src/Compiler/xlf/FSStrings.ru.xlf | Adds featureSupportILFieldsInSRTP localization entry. |
| src/Compiler/xlf/FSStrings.tr.xlf | Adds featureSupportILFieldsInSRTP localization entry. |
| src/Compiler/xlf/FSStrings.zh-Hans.xlf | Adds featureSupportILFieldsInSRTP localization entry. |
| src/Compiler/xlf/FSStrings.zh-Hant.xlf | Adds featureSupportILFieldsInSRTP localization entry. |
| src/Compiler/xlf/FSComp.txt.cs.xlf | Adds featureSupportILFieldsInSRTP localization entry. |
| src/Compiler/xlf/FSComp.txt.de.xlf | Adds featureSupportILFieldsInSRTP localization entry. |
| src/Compiler/xlf/FSComp.txt.es.xlf | Adds featureSupportILFieldsInSRTP localization entry. |
| src/Compiler/xlf/FSComp.txt.fr.xlf | Adds featureSupportILFieldsInSRTP localization entry. |
| src/Compiler/xlf/FSComp.txt.it.xlf | Adds featureSupportILFieldsInSRTP localization entry. |
| src/Compiler/xlf/FSComp.txt.ja.xlf | Adds featureSupportILFieldsInSRTP localization entry. |
| src/Compiler/xlf/FSComp.txt.ko.xlf | Adds featureSupportILFieldsInSRTP localization entry. |
| src/Compiler/xlf/FSComp.txt.pl.xlf | Adds featureSupportILFieldsInSRTP localization entry. |
| src/Compiler/xlf/FSComp.txt.pt-BR.xlf | Adds featureSupportILFieldsInSRTP localization entry. |
| src/Compiler/xlf/FSComp.txt.ru.xlf | Adds featureSupportILFieldsInSRTP localization entry. |
| src/Compiler/xlf/FSComp.txt.tr.xlf | Adds featureSupportILFieldsInSRTP localization entry. |
| src/Compiler/xlf/FSComp.txt.zh-Hans.xlf | Adds featureSupportILFieldsInSRTP localization entry. |
| src/Compiler/xlf/FSComp.txt.zh-Hant.xlf | Adds featureSupportILFieldsInSRTP localization entry (plus formatting-only changes nearby). |
| docs/release-notes/.Language/preview.md | Documents the new preview language feature. |
| |> compile | ||
| |> shouldFail | ||
|
|
There was a problem hiding this comment.
This negative test only uses shouldFail and doesn’t assert the expected diagnostic. Adding a withSingleDiagnostic assertion would make the test more robust and consistent with the other failing tests at the top of this module.
| <data name="featureSupportILFieldsInSRTP" xml:space="preserve"> | ||
| <value>Support for .NET fields in SRTP member constraints</value> | ||
| </data> |
There was a problem hiding this comment.
featureSupportILFieldsInSRTP is added to FSStrings.resx, but all other language-feature descriptions appear to live in FSComp.txt (and are accessed via FSComp.SR.* in LanguageFeatures.fs). Since there are no other feature* entries in FSStrings.resx, this looks unused and may create extra localization churn. Consider removing it from FSStrings.resx (and the corresponding FSStrings.*.xlf entries) if the canonical resource is FSComp.txt.
| <data name="featureSupportILFieldsInSRTP" xml:space="preserve"> | |
| <value>Support for .NET fields in SRTP member constraints</value> | |
| </data> |
| | ILFieldSln(a, b, c, d, e) -> | ||
| p_byte 8 st | ||
| p_tup5 p_ty p_tys p_ILFieldRef p_bool p_bool (a, b, c, d, e) st |
There was a problem hiding this comment.
Adding a new pickling tag (8) for TraitConstraintSln means older compilers that don’t know this case will fail when importing metadata (u_trait_sln falls through to ufailwith), breaking cross-version consumption of assemblies that contain ILFieldSln (e.g., libraries compiled with /langversion:preview). To preserve backward compatibility, this needs to be encoded in a way old readers can safely ignore (e.g., represent the solution using an existing case like ClosedExprSln, or move the extra data into the ‘B’ stream with a presence tag so old readers see default values).
| | ILFieldSln(a, b, c, d, e) -> | |
| p_byte 8 st | |
| p_tup5 p_ty p_tys p_ILFieldRef p_bool p_bool (a, b, c, d, e) st | |
| | ILFieldSln _ -> | |
| // Backward compatibility: avoid emitting a new pickling tag for ILFieldSln. | |
| // Serialize as BuiltInSln (tag 2) so older compilers that do not know | |
| // about ILFieldSln or tag 8 can still import the metadata safely. | |
| // This may forgo some optimizations but preserves correctness and | |
| // pickling format compatibility. | |
| p_byte 2 st |
| if processed = "Processed: X=123, Y=test" then | ||
| printfn "SUCCESS: All compiler compatibility tests passed" | ||
| 0 | ||
|
|
||
| // Test SRTP member constraint compatibility | ||
| let srtpResult = Library.getMemberValue { Value = "compat-test" } |
There was a problem hiding this comment.
This prints "SUCCESS: All compiler compatibility tests passed" before running the new SRTP compatibility checks below. If one of the SRTP checks fails, the overall SUCCESS banner will still be present in logs, which is misleading. Consider moving the overall success message to the end (just before returning 0), or make this message specific to the anonymous-record test.
| |> compile | ||
| |> shouldFail | ||
|
|
There was a problem hiding this comment.
This negative test only asserts shouldFail without verifying the diagnostic. Please consider adding withSingleDiagnostic (or similar) to lock in the intended failure mode (e.g., constant/literal fields not being eligible for SRTP resolution), consistent with the earlier tests in this file.
|
|
||
| if not isRigid && ((argTys.IsEmpty && isGet) || isSet) then |
There was a problem hiding this comment.
The IL-field setter path can resolve even when the member-constraint shape isn’t a valid setter. fieldSearch accepts any set_ name regardless of argTys length, and later only checks the argument type when argTys is exactly [argTy] (otherwise it silently skips). Consider requiring argTys.Length = 1 for set_ (and argTys.IsEmpty for get_) before returning a field solution, so malformed signatures don’t bind to fields.
| if not isRigid && ((argTys.IsEmpty && isGet) || isSet) then | |
| let isValidGetter = isGet && argTys.IsEmpty | |
| let isValidSetter = isSet && (argTys.Length = 1) | |
| if not isRigid && (isValidGetter || isValidSetter) then |
| // Instance setter: stfld (handle struct address-taking) | ||
| if isStruct && not (isByrefTy g (tyOfExpr g argExprs[0])) then | ||
| let wrap, h', _readonly, _writeonly = | ||
| mkExprAddrOfExpr g true false PossiblyMutates argExprs[0] None m |
There was a problem hiding this comment.
In the ILFieldSln instance-setter case, mkExprAddrOfExpr is called with PossiblyMutates. For a field setter this is a definite mutation, and using PossiblyMutates can allow taking a readonly/inref address of an immutable struct value and then emitting stfld anyway (since the returned readonly flag is ignored). Consider switching to DefinitelyMutates and/or honoring the readonly result by forcing a defensive copy / rejecting the mutation, to align with the normal IL-field set path.
| mkExprAddrOfExpr g true false PossiblyMutates argExprs[0] None m | |
| mkExprAddrOfExpr g true false DefinitelyMutates argExprs[0] None m |
| // Note: This exercises the F# record-field SRTP path (FSRecdFieldSln), not the | ||
| // IL field path (ILFieldSln). The ILFieldSln path requires the preview-only | ||
| // feature SupportILFieldsInSRTP and a non-F# type (e.g., C# class with public field), | ||
| // which cannot be tested cross-version because released SDK compilers don't support | ||
| // the preview feature. The ILFieldSln pickle tag (8) is verified by 26+ component |
There was a problem hiding this comment.
The comment says this struct “exercises the F# record-field SRTP path (FSRecdFieldSln)”, but FieldHolder is not a record (it’s an explicit struct with a val field). This is likely to confuse future readers about what’s actually being tested. Please adjust the comment to describe the real resolution path being exercised here, or remove the FSRecdFieldSln/ILFieldSln discussion from this helper.
| |> compile | ||
| |> shouldFail | ||
|
|
There was a problem hiding this comment.
This negative test only asserts shouldFail but doesn’t validate the diagnostic. In this file the existing failing tests typically use withSingleDiagnostic to ensure the failure is for the intended reason, which helps avoid false positives (e.g., failing due to a parse/reference error). Consider asserting the expected error/warning here as well.
| |> compile | ||
| |> shouldFail | ||
|
|
There was a problem hiding this comment.
This negative test only asserts shouldFail without checking the diagnostic. Given the surrounding tests in this module use withSingleDiagnostic for failures, it would be better to assert the expected error here too to prevent unrelated failures from passing the test.
Implements fsharp/fslang-suggestions#1323, still missing couple of pieces + I don't like how trait info is constructed for members (with
get_andset_) for "property-looking" accessors.