Repository navigation
fix(core): preserve optional F# async support without AOT warnings - #7526
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. Walkthrough
Merge Risk:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bdb39953fa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
[Medium impact] The PR appears safe to merge; no actionable new issues were found. SummaryPreserves optional F# async support through a packaged linker descriptor instead of string-based
Reviews (3) · Last reviewed commit: "fix(core): preserve runtime-only FSharp ..." · Reviewed by Greptile |
|
Review: looks good. The change fixes the IL2035 warnings in #7523 by removing the two Two minor points, neither blocking:
Your validation covers an AOT publish with no IL2035 warnings and the F# async tests passing. |
|
Review dispositions for 35d8a4d:
Validation details and limitations are recorded in the updated PR description. CI and fresh automated review are pending on the new head; auto-merge remains disabled. |
Review of #7526The design is sound. A packaged linker descriptor, added only when trimming is on, AOT is off and FSharp.Core is referenced, fixes the IL2007/IL2035 warnings for C# AOT apps. It also keeps Minor points, none blocking:
Looks good to merge once CI package validation passes. |
|
Pushed e4c4769 to address the runtime-only FSharp.Core review finding; the inline reply records the regression and negative-control checks. Fresh CI and automated reviews are pending, and auto-merge remains disabled. Claude follow-ups: the previous head's nuget-package-tests-windows job (run 41710901082) passed PackTUnitFilesModule and the F# package tests. This confirms package creation and consumption, but does not independently assert the descriptor archive entries; the earlier MSBuild package-item evaluation remains the direct layout evidence. A permanent package-layout regression test is a nonblocking follow-up suggestion. The existing documentation states the Native AOT support boundary; the additional failure-mode note is optional. CodeRabbit's docstring coverage advisory concerns existing code with removed attributes and does not require a functional change here. |
ReviewThe approach is sound. Replacing the string-based Checked:
Minor points, none blocking:
Looks good to merge once CI is green. |
|
Final review disposition for e4c4769:
All current CI checks are successful or intentionally skipped. Proceeding through the repository merge wrapper. |
Closes #7523.
C# applications without FSharp.Core now publish under Native AOT without unresolved optional-assembly warnings. Trimmed JIT applications that reference FSharp.Core continue to execute F# async values.
Removed the string-based DynamicDependency attributes and replaced them with a packaged linker descriptor preserving FSharpAsync.StartAsTask. TUnit.Core.targets adds that descriptor before PrepareForILLink only when PublishTrimmed is enabled, Native AOT is disabled, and the resolved references include FSharp.Core. Native AOT remains supported for C# test projects, as documented in engine-modes.md.
Validation on .NET SDK 10.0.403, macOS arm64:
The focused F# publish reports existing FSharp.Core IL2040 resource warnings, tracked in dotnet/fsharp#15261; reproduction details were added there. Publishing the complete F# test project with a global PublishTrimmed property cannot validate this configuration because that property also reaches netstandard2.0 analyzer dependencies (NETSDK1124), so the focused app isolates the relevant trimming path.
Summary by CodeRabbit
FSharp.Coreis referenced. The required async functionality is now preserved during trimming, helping these applications continue to work as expected.