Fix a regression to collection asserting in AssemblyChecker#125443
Merged
sbomer merged 1 commit intodotnet:mainfrom Mar 11, 2026
Merged
Fix a regression to collection asserting in AssemblyChecker#125443sbomer merged 1 commit intodotnet:mainfrom
AssemblyChecker#125443sbomer merged 1 commit intodotnet:mainfrom
Conversation
github-actions
Bot
added
the
area-Tools-ILLink
dotnet-policy-service
Bot
added
linkable-framework
Contributor
|
Tagging subscribers to this area: @agocke, @dotnet/illink |
The introduction of `ToHashSet()` + `SetEquals` in `AssemblyChecker` caused a regression in the validation in various things. fixes validation of stubbed methods Expand attribute verification to allow for type checking of parameters. This is needed to deal with assertions in the TypeMap step.
mrvoorhe
force-pushed
the
linker-fix-seq-comparison
branch
from
6c203d4 to
9061e9d
Compare
Contributor
Author
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the ILLink test infrastructure to avoid masking regressions when asserting collections (particularly method body/instruction sequences) by removing ToHashSet()-based deduplication and SetEquals()-based order-insensitive comparisons in AssemblyChecker. It also extends KeptAttributeAttribute assertions so tests can match specific attribute instances by constructor arguments (needed for the TypeMap coverage), and adds new test cases to validate the updated behavior.
Changes:
- Replace
ToHashSet()/SetEquals()comparisons with sequence-based comparisons inAssemblyChecker. - Extend
KeptAttributeAttributeexpectations to optionally include constructor argument matching; updateTypeMapassertions accordingly. - Add new test cases covering locals/body changes and parameterized
KeptAttributeAttributeassertions; update generated test list.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tools/illink/test/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs | Removes dedup/order-insensitive comparisons; adds attribute-argument-aware matching logic. |
| src/tools/illink/test/Mono.Linker.Tests.Cases/TestFramework/VerifyLocalsAreChanged.xml | Linker substitution file to force body stubbing for locals verification. |
| src/tools/illink/test/Mono.Linker.Tests.Cases/TestFramework/VerifyLocalsAreChanged.cs | New test case validating locals/body sequence assertions. |
| src/tools/illink/test/Mono.Linker.Tests.Cases/TestFramework/VerifyKeptAttributeAttributeWorks.cs | New test case validating KeptAttributeAttribute matching (including duplicates/args). |
| src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/TypeMap.cs | Updates assembly-level KeptAttributeAttribute assertions to include constructor arguments. |
| src/tools/illink/test/Mono.Linker.Tests.Cases.Expectations/Assertions/KeptAttributeAttribute.cs | Adds a new constructor for parameter-aware assertions and documents intended usage. |
| src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/generated/ILLink.RoslynAnalyzer.Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/TestFrameworkTests.g.cs | Generated test list updated to include the new test cases. |
sbomer
approved these changes
Mar 11, 2026
Member
|
/ba-g "timeouts" |
Copilot AI
pushed a commit
that referenced
this pull request
Mar 13, 2026
#97605 introduced a pattern that regressed test coverage and in some cases broke asserting. There were a few different regressions. 1) The introduction of `ToHashSet()` results in deduplication. In some cases this was harmless, but it others it causes very real problems. `VerifyBodyProperties` is used by `VerifyInstructions` which means the test framework was deduplicating before asserting bodies were unchanged. 2) `SetEquals` removed ordering requirements. This was never necessary and in some places really undermines coverage. If a method body had it's instructions scrambled the test would still happily pass. This PR removes the `ToHashSet()` and `SetEquals()` usages from `AssemblyChecker`. There's another issue. #116355 added the `TypeMap` test. This test introduced a dependency on the deduplication behavior. Roslyn will deduplicate assembly attributes at compile time. This meant that there was no way to get this test passing using the existing pattern of defining `[KeptAttributeAttribute(typeof(TypeMapAttribute<UsedType>))]` multiple times in order to tell the test framework that multiple instances of an attribute should survive. To solve this, we've upgrade the `KeptAttributeAttribute` assertion ability to allow for specifying the attribute parameters that you expect to survive. This allows for matching up the `KeptAttributeAttribute` with the exact attribute instance. This new ability is then used in `TypeMap` to update the assertions at the assembly level to pass with the deduplication removed. Some new tests have been added to `TestFramework`. Co-authored-by: Adriano Carlos Verona <adriano@unity3d.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#97605 introduced a pattern that regressed test coverage and in some cases broke asserting. There were a few different regressions.
The introduction of
ToHashSet()results in deduplication. In some cases this was harmless, but it others it causes very real problems.VerifyBodyPropertiesis used byVerifyInstructionswhich means the test framework was deduplicating before asserting bodies were unchanged.SetEqualsremoved ordering requirements. This was never necessary and in some places really undermines coverage. If a method body had it's instructions scrambled the test would still happily pass.This PR removes the
ToHashSet()andSetEquals()usages fromAssemblyChecker. There's another issue.#116355 added the
TypeMaptest. This test introduced a dependency on the deduplication behavior. Roslyn will deduplicate assembly attributes at compile time. This meant that there was no way to get this test passing using the existing pattern of defining[KeptAttributeAttribute(typeof(TypeMapAttribute<UsedType>))]multiple times in order to tell the test framework that multiple instances of an attribute should survive. To solve this, we've upgrade theKeptAttributeAttributeassertion ability to allow for specifying the attribute parameters that you expect to survive. This allows for matching up theKeptAttributeAttributewith the exact attribute instance. This new ability is then used inTypeMapto update the assertions at the assembly level to pass with the deduplication removed.Some new tests have been added to
TestFramework.