Skip to content

Revert "Simplify TrimmerSingleWarn intermediate assembly update using MSBuild item Update"#127656

Merged
adamsitnik merged 1 commit intomainfrom
revert-125630-copilot/fix-feedback-implementation
May 3, 2026
Merged

Revert "Simplify TrimmerSingleWarn intermediate assembly update using MSBuild item Update"#127656
adamsitnik merged 1 commit intomainfrom
revert-125630-copilot/fix-feedback-implementation

Conversation

@sbomer
Copy link
Copy Markdown
Member

@sbomer sbomer commented May 1, 2026

Reverts #125630

The Update in a target doesn't work (see dotnet/msbuild#2835 for the MSBuild behavior).

Fixes the dependency flow issue here: dotnet/sdk#53847 (comment).

@sbomer sbomer requested review from a team and Copilot May 1, 2026 18:36
@github-actions github-actions Bot added the area-Tools-ILLink .NET linker development as well as trimming analyzers label May 1, 2026
@dotnet-policy-service dotnet-policy-service Bot added the linkable-framework Issues associated with delivering a linker friendly framework label May 1, 2026
@dotnet-policy-service
Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke, @dotnet/illink
See info in area-owners.md if you want to be subscribed.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Reverts the prior simplification that used MSBuild item Update in _PrepareTrimConfiguration, restoring a set-based approach to ensure TrimmerSingleWarn=false metadata is correctly applied to the ResolvedFileToPublish item(s) corresponding to @(IntermediateAssembly).

Changes:

  • Replace the ResolvedFileToPublish Update="@(IntermediateAssembly)" metadata update with explicit include/remove set operations to compute the intersection between @(ResolvedFileToPublish) and @(IntermediateAssembly).
  • Re-add the updated intersection items back into @(ResolvedFileToPublish) so downstream consumers observe the metadata.
Show a summary per file
File Description
src/tools/illink/src/ILLink.Tasks/build/Microsoft.NET.ILLink.targets Restores reliable MSBuild item manipulation to apply TrimmerSingleWarn metadata to the intermediate assembly’s publish items without using Update in a target.

Copilot's findings

  • Files reviewed: 1/1 changed files
  • Comments generated: 0

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 1, 2026

🤖 Copilot Code Review — PR #127656

Note

This review was generated by GitHub Copilot.

Holistic Assessment

Motivation: This PR reverts PR #125630 which simplified the TrimmerSingleWarn intermediate assembly metadata update logic. The revert is well-justified — MSBuild's Update syntax inside targets has a known bug (dotnet/msbuild#2835) where it updates all items in the list instead of only matching ones, causing a real dependency flow failure.

Approach: A clean git revert is the correct approach for undoing a broken simplification. The restored intersection-based pattern is the established workaround for this MSBuild limitation.

Summary: ✅ LGTM. This is a clean revert that restores known-working logic. The set-intersection pattern is correct and well-commented. One optional suggestion for preventing future re-simplification attempts.


Detailed Findings

✅ Clean Revert — Verified correct

The revert is clean and focused. Only the expected file is modified, and the restored code exactly matches the pre-simplification state. No unrelated changes or scope creep.

✅ Restored MSBuild Logic — Verified correct

The set-intersection pattern is mathematically sound:

  1. __SingleWarnIntermediateAssembly = ResolvedFileToPublish \\ IntermediateAssembly (set difference)
  2. _SingleWarnIntermediateAssembly = ResolvedFileToPublish \\ __SingleWarnIntermediateAssembly = ResolvedFileToPublish ∩ IntermediateAssembly
  3. Metadata is set on the intersection (only when TrimmerSingleWarn is not already set)
  4. Items are replaced in ResolvedFileToPublish via Remove+Include

This correctly limits TrimmerSingleWarn=false to only items that exist in both ResolvedFileToPublish and IntermediateAssembly, working around the MSBuild Update bug.

💡 Suggestion — Add preventive comment (follow-up, not blocking)

Future maintainers may attempt to "simplify" this code again. Consider adding a comment explaining why the simpler Update syntax cannot be used:

<!-- NOTE: Cannot use <ResolvedFileToPublish Update="@(IntermediateAssembly)"> because
     Update inside targets has a bug (dotnet/msbuild#2835) that updates ALL items
     instead of only matching ones. Use set operations instead. -->

This was flagged independently by multiple review models (Claude Sonnet 4.5, GPT-5.3-Codex).


Review contributed by: Claude Opus 4.6 (primary), Claude Sonnet 4.5, GPT-5.3-Codex

Generated by Code Review for issue #127656 ·

@sbomer
Copy link
Copy Markdown
Member Author

sbomer commented May 2, 2026

/backport to release/11.0-preview4

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 2, 2026

Started backporting to release/11.0-preview4 (link to workflow run)

Copy link
Copy Markdown
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The revert LGTM, I am approving in order to unblock the dependency flow.

@adamsitnik adamsitnik enabled auto-merge (squash) May 3, 2026 20:12
@adamsitnik
Copy link
Copy Markdown
Member

/ba-g DeadLetter is unrelated and has very high failure rate (11%)

@adamsitnik adamsitnik merged commit fcd092d into main May 3, 2026
92 of 97 checks passed
@adamsitnik adamsitnik deleted the revert-125630-copilot/fix-feedback-implementation branch May 3, 2026 20:13
@github-project-automation github-project-automation Bot moved this to Done in AppModel May 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-Tools-ILLink .NET linker development as well as trimming analyzers linkable-framework Issues associated with delivering a linker friendly framework

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants