Use SDK ILLink analyzer for OOBs for downlevel TFMs - #114983
Conversation
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
Files not reviewed (3)
- eng/liveILLink.targets: Language not supported
- src/libraries/Directory.Build.props: Language not supported
- src/libraries/Directory.Build.targets: Language not supported
ghost
added
the
- Use IsTargetFrameworkCompatible - Move EnableAOTAnalyzer to targets - Rename ExplicitlyImportCustomILLinkTargets to ImportCustomILLinkTargets and infer value
- Fix typo - Use IsTargetFrameworkCompatible - Import illink.targets only for netcoreapp To prevent netfx TFMs from attempting to use ILLink tasks that aren't available when targeting netfx. - Only use live ILLIink for current TFM
dotnet-policy-service
Bot
added
the
linkable-framework
ViktorHofer
left a comment
ViktorHofer
left a comment
There was a problem hiding this comment.
The TFM changes look incorrect.
Co-authored-by: sbomer <787361+sbomer@users.noreply.github.com>
These files had existing suppressions modified in PR #114983, but those modifications were specific to build infrastructure changes (switching to SDK ILLink analyzer) which should not be ported. Co-authored-by: sbomer <787361+sbomer@users.noreply.github.com>
When using downlevel ILLink to trim individual libraries for warning validation, we shouldn't use custom ILLink options that were introduced in later versions.
Backport of the suppressions from #114983 to release/10.0 ## Customer Impact - [x] Customer reported - [ ] Found internally Customer reported unexpected trim warnings in a .NET 8 app that references a 10.0 System.Text.Json. Customer issues: - #114307 - #122035 ## Regression - [x] Yes - [ ] No Referencing a previous version of the STJ library would not have this problem. Effectively a regression in the OOB package when referenced from a .NET 8 app. ## Testing In main, we are making build infrastructure changes to validate this when producing the OOBs. Also validated locally that this fixes the customer-reported issue. The build infrastructure changes aren't part of this backport to avoid risk (and because the changes require other analyzer fixes in .NET 8/.NET 9 to flow first). ## Risk Low: the fix only suppresses trim warnings. <hr> main PR #114983 # Description Ports trimming warning suppressions from PR #114983 to the release/10.0 branch. **Changes ported:** - **System.Text.Json EnumConverter.cs**: IL2090 suppression for GetFields() helper (enum fields preserved by trimming) - **System.Text.Json EnumConverterFactory.cs**: Unconditional IL2071 suppression (original) + conditional IL2070 suppression for !NET9_0_OR_GREATER - **System.Text.Json NullableConverterFactory.cs**: Unconditional IL2071 suppression (original) + conditional IL2070 suppression for !NET9_0_OR_GREATER - **System.Text.Json ReflectionEmitMemberAccessor/ReflectionMemberAccessor.cs**: IL2046 suppressions for CreateAddMethodDelegate (type has RequiresUnreferencedCode) **Not ported:** - Microsoft.XmlSerializer.Generator.csproj and Sgen.cs - changes were not needed for release/10.0 branch. The suppression approach for EnumConverterFactory and NullableConverterFactory keeps the original IL2071 suppression unconditionally and adds a new IL2070 suppression only for pre-.NET 9.0 targets. This ensures both warning codes are suppressed for downlevel target frameworks (net8.0, net462, netstandard2.0) while maintaining the original suppression for all target frameworks. # Customer Impact Without these suppressions, builds may show trimming analyzer warnings for code patterns that are safe and intentional. # Regression No - these are suppressions for legitimate code patterns, not fixes for actual issues. # Testing - System.Text.Json builds successfully with 0 warnings and 0 errors - Manual verification confirms all suppressions are correctly applied - Validated that both IL2070 and IL2071 are suppressed for downlevel TFMs # Risk Low - only adds warning suppressions with justifications, no behavioral changes. <!-- START COPILOT CODING AGENT TIPS --> --- ✨ Let Copilot coding agent [set things up for you](https://github.com/dotnet/runtime/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: sbomer <787361+sbomer@users.noreply.github.com>
# Conflicts: # eng/liveILLink.targets
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "653990b018cd2c7be2925cd3f4569dc453a199ef",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "2ff39b5f21a304c04ee0726c3e2ae56e7b23c904",
"last_reviewed_commit": "653990b018cd2c7be2925cd3f4569dc453a199ef",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "2ff39b5f21a304c04ee0726c3e2ae56e7b23c904",
"last_recorded_worker_run_id": "29673724303",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "653990b018cd2c7be2925cd3f4569dc453a199ef",
"review_id": 4730108682
}
]
} |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Holistic Review
Motivation: Out-of-band (OOB) library packages that multi-target downlevel TFMs (netstandard2.0, netframework, older netcoreapp) were being analyzed with the repo's live ILLink analyzer even for TFMs the shipping trimming/AOT tooling doesn't support. This produced spurious or missed diagnostics (e.g. #114307). The goal is to use the SDK's ILLink analyzer for downlevel TFMs and reserve the live analyzer for the current TFM, while still routing OOB assemblies through the live linker via the OOB trim step.
Approach: The _RequiresLiveILLink property is renamed to a public-facing UseLiveILLink opt-in/out knob, and its default computation in eng/liveILLink.targets is now gated on IsTargetFrameworkCompatible($(TargetFramework), $(NetCoreAppCurrent)), so downlevel TFMs fall back to the SDK analyzer. illink.targets no longer force-sets live ILLink and is imported only for TFMs >= net6.0 through a new ImportCustomILLinkTargets property (replacing ExplicitlyImportCustomILLinkTargets). The IsAotCompatible default moves from Directory.Build.props to Directory.Build.targets (so it observes per-project TFM/opt-out settings), and a source-build-only _SilenceIsAotCompatibleUnsupportedWarning silences the expected NETSDK1210 gap. Individual projects opt back into the live task via UseLiveILLink=true where they need resource-substitution generation or shim metadata, or opt out of AOT/trim where they contain no analyzable code. Finally, the --preserve-symbol-paths ILLink arg is gated to net9.0+ (when the option was introduced), and several System.Text.Json trim-analysis suppressions are made TFM-conditional (IL2071 on net9+, IL2070 downlevel; new IL2046/IL2090 suppressions for the SDK analyzer path) since the SDK analyzer emits different warning codes than the live analyzer.
Summary: This is a focused, low-risk build-infrastructure change authored by the repository's ILLink/trimming domain expert and already reviewed and approved by area maintainers. The renames are applied consistently across eng/*.targets, Directory.Build.*, oob-trim.proj, sfx-finish.proj, and the cdac-build-tool props. The TFM-compatibility gates and version-conditional ILLink args/suppressions are the correct mechanism for aligning warning behavior between the SDK and live analyzers across multi-targeted OOBs. The System.Text.Json C# changes are annotation-only (conditional UnconditionalSuppressMessage codes and a refactor of GetFields() into a locally-suppressed static local function) and do not change runtime behavior. Correctness of the property-flow changes is best validated by the existing trimming/source-build CI legs rather than by inspection alone; nothing in the diff suggests a behavioral regression. I found no actionable issues. Verdict: LGTM.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 66.8 AIC · ⌖ 14.8 AIC · ⊞ 10K
Don't use live ILLink analyzer for OOBs when targeting downlevel TFMs. Should help catch issues like #114307.
Note that some OOBs don't target the current TFM - these will not be analyzed using the live analyzer at all. This should be mitigated by our OOB trim step which runs all of the OOB assemblies through the live linker.