ILLink Analyzer: Don't assume Tuple operations compile to creation of a ValueTuple - #134377
jtschuster wants to merge 4 commits into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2c75aa0b-5f23-4371-8668-fa093efefe3c
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2c75aa0b-5f23-4371-8668-fa093efefe3c
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2c75aa0b-5f23-4371-8668-fa093efefe3c
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @agocke, @dotnet/illink |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Direct tuple operations currently discard computed element values, causing data-flow regressions and masked warnings.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Updates ILLink tuple data-flow analysis to avoid assuming tuple expressions lower to ValueTuple operations.
Changes:
- Tracks tuple-producing flow captures.
- Uses conservative values for ambiguous lowering paths.
- Adds and updates deconstruction regression tests.
| File | Summary |
|---|---|
src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/DeconstructFieldTarget.cs |
Updates expected warning behavior for deconstruction scenarios. |
src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ConstructedTypesDataFlow.cs |
Adds tuple and deconstruction data-flow regression coverage. |
src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs |
Adjusts tuple operation and flow-capture modeling. |
|
This doesn't look right. I think we want to break down the lattice state into top/scalar/multi-value (tuple). Something like And then instead of we swap out |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2c75aa0b-5f23-4371-8668-fa093efefe3c
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Tuple flow captures may lose element-level dataflow and suppress required warnings.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Resolved since last review (1)
| useTopForTupleElements | ||
| ? TopValue | ||
| : GetTupleElementValue(tupleElement), |


The trim analyzer assumed that all
ITupleOperations lowered to a creation of aValueTuple, and deconstruction would be a reference to theItemNfield. This isn't always true. Instead, don't make any assumptions and avoid warning by creating TopValue for elements of an ITupleOperaion.We track all flow captures that have any
ITupleOperationsin order to resolve which IFlowCaptureReferenceOperations resolve to a Tuple, then for directITupleOperations we construct a (potentially nested) tuple ofTopValues.Fixes #133911