Add dataflow support for deconstruction assignments - #131624
Conversation
Fixes dotnet#123767: the ILLink Roslyn analyzer's dataflow visitor did not handle IDeconstructionAssignmentOperation, so DynamicallyAccessedMembers annotations were silently dropped across deconstruction assignments and foreach-variable deconstruction. - LocalDataFlowVisitor: add VisitDeconstructionAssignment, which uses the semantic model's GetDeconstructionInfo (IOperation does not expose the Deconstruct-method/conversion recipe on IDeconstructionAssignmentOperation) together with the existing Target IOperation tree to evaluate all source values before assigning any target, preserving correct swap semantics such as (first, second) = (second, first). - TrimAnalysisVisitor: add GetTupleElementValue override consumed by the new evaluation path. - Add DeconstructTupleSwapSuccess test case alongside the existing DeconstructTupleSwap warning case, covering the no-warning path where annotations propagate correctly across a tuple swap. Assisted-by: Copilot:claude-sonnet-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4dc4554-6607-4109-ab22-56a5815b294d
Deconstruction assignment targets can have side-effecting sub-expressions that identify a write location, e.g. the receiver of a property/indexer target or the array reference/index of an array-element target. Roslyn's own lowering evaluates these before touching the right-hand side (GetAssignmentTargetsAndSideEffects), matching left-to-right evaluation order, but our previous implementation only visited them as part of performing the write in AssignDeconstruction, which runs after the entire source side has already been evaluated. Add VisitDeconstructionTargetSideEffects, a pre-pass over the target tree that visits these sub-expressions (without performing any write) before the source is visited. The same sub-expressions are visited again later when AssignDeconstruction performs the actual write; revisiting the same IOperation node is safe because the trim analysis pattern store merges patterns keyed by IOperation identity, the same tolerance already relied on elsewhere in this file (see ProcessAssignment for flow-capture targets with multiple captured references). Add a regression test (DeconstructPropertyTargetSideEffect) covering a property target whose receiver is a side-effecting, warning-producing expression, to lock in that the warning fires exactly once. Assisted-by: Copilot:claude-sonnet-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4dc4554-6607-4109-ab22-56a5815b294d
Extract TryVisitAssignmentTargetInstance, a shared helper covering the subset and order of a target operation's side-effecting sub-expressions (receiver instance, and/or index/argument sub-expressions) that ProcessSingleTargetAssignment already visits before evaluating the value being assigned. This is a pure extract-method refactor of ProcessSingleTargetAssignment with no behavior change: the same sub-expressions are visited in the same order as before, for every target kind, including IPropertyReferenceOperation's explicit indexer Arguments, which are intentionally left out of the shared helper because they are only visited after the value today (a known, pre-existing ordering quirk in ProcessSingleTargetAssignment, distinct from the implicit System.Index-based indexer and array-element cases, which already visit their index arguments before the value). VisitDeconstructionTargetSideEffects now calls this same shared helper instead of a hand-duplicated switch. This means deconstruction targets evaluate side effects using the exact same subset/order as ordinary assignment targets, including the explicit-indexer-Arguments gap, rather than the previous, inconsistent behavior where deconstruction pre-visited indexer Arguments that ordinary assignment does not. If the underlying evaluation-order gap is fixed later, fixing it in this one shared place will fix it for both assignment forms. No test or behavior changes; verified zero regressions across the analyzer, linker, ILTrim, and ILC/NativeAOT DataFlow test suites. Assisted-by: Copilot:claude-sonnet-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4dc4554-6607-4109-ab22-56a5815b294d
…nstruction" This reverts commit 9e5a8f0.
…nstruction targets VisitDeconstructionTargetSideEffects previously pre-visited an explicit indexer's Arguments before the source, but ProcessSingleTargetAssignment (used for ordinary assignments) only visits those same Arguments after the value due to a pre-existing ordering quirk. This mismatch meant deconstruction assignment targets and ordinary assignment targets could report side effects in a different relative order for the same shape of target (e.g. 'this[F()] = ...' vs '(this[F()], b) = ...'). Rather than fixing the pre-existing quirk (out of scope here) or sharing logic via a new helper (reverted in the prior commit as too broad a refactor for this fix), simply stop pre-visiting the indexer Arguments in VisitDeconstructionTargetSideEffects. This makes deconstruction targets consistent with ordinary assignment targets again, preserving today's (admittedly imperfect) evaluation order rather than introducing a new, narrower divergence. Verified no regressions across all four toolchains: - ILLink.RoslynAnalyzer.Tests DataFlow: 81 passed, 2 skipped - Mono.Linker.Tests DataFlowTests: 80 passed - ILTrim.Tests DataFlowTests: 43 passed, 37 skipped - ILCompiler.Trimming.Tests DataFlow (Release): 80 passed Assisted-by: Copilot:claude-sonnet-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4dc4554-6607-4109-ab22-56a5815b294d
…ites VisitDeconstructionTargetSideEffects, EvaluateDeconstruction, and AssignDeconstruction each already unwrap their target parameter as the first statement on entry (required for their recursive calls, which pass targetTuple.Elements[i] without pre-unwrapping). The top-level calls from VisitDeconstructionAssignment were redundantly unwrapping operation.Target before passing it in, duplicating work each method already does itself. Pass operation.Target directly and let each method's own unwrap-on-entry handle it, matching the recursive calls. Verified no regressions across all four toolchains: - ILLink.RoslynAnalyzer.Tests DataFlow: 81 passed, 2 skipped - Mono.Linker.Tests DataFlowTests: 80 passed - ILTrim.Tests DataFlowTests: 43 passed, 37 skipped - ILCompiler.Trimming.Tests DataFlow (Release): 80 passed Assisted-by: Copilot:claude-sonnet-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4dc4554-6607-4109-ab22-56a5815b294d
DeconstructPropertyTargetSideEffect covers a parameterless property target's receiver side effect, but no test exercised an explicit indexer target (IPropertyReferenceOperation with Arguments) - exactly the case the last two commits changed the evaluation order for. Add DeconstructIndexerTargetSideEffect, which uses an indexer target whose receiver and index argument are both side-effecting [RequiresUnreferencedCode] calls, verifying each is visited exactly once (no duplicate warnings) despite being visited from two different places (the target-side-effects pre-pass for the receiver, and the actual write for the index argument). Verified across all four toolchains: - ILLink.RoslynAnalyzer.Tests DataFlow: 81 passed, 2 skipped - Mono.Linker.Tests DataFlowTests: 80 passed - ILTrim.Tests DataFlowTests: 43 passed, 37 skipped - ILCompiler.Trimming.Tests DataFlow (Release): 80 passed Assisted-by: Copilot:claude-sonnet-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4dc4554-6607-4109-ab22-56a5815b294d
Closes the rest of the test-coverage gaps identified for deconstruction dataflow: field target, parameter target, discard target, array-element target (with side-effect ordering, like the indexer case), implicit System.Index-based indexer target, and a source element requiring a user-defined conversion operator (verifying EvaluateDeconstruction's conversion.MethodSymbol-is-not-null branch runs without crashing or producing an unexpected warning). DeconstructFieldTarget exposes a pre-existing, unrelated ILTrim limitation in static field write dataflow tracking (the same class of issue that already causes the whole, unrelated FieldDataFlow.cs test to be entirely skipped for ILTrim - see ILTrimExpectedFailures.txt). Both the Roslyn analyzer and NativeAOT ILCompiler correctly detect this case; only ILTrim.Core's separate dataflow engine does not. Since ILTrim's known-limitation mechanism only supports skipping a whole test case (not one assertion within it), and Tool.NativeAot-based ExpectedWarning filtering can't distinguish ILTrim from ILC (they share the flag), add ConstructedTypesDataFlow to ILTrimExpectedFailures.txt rather than weakening the assertion or dropping the test - preserving full verification under the other three toolchains. Verified across all four toolchains: - ILLink.RoslynAnalyzer.Tests DataFlow: 81 passed, 2 skipped - Mono.Linker.Tests DataFlowTests: 80 passed - ILTrim.Tests DataFlowTests: 42 passed, 38 skipped (ConstructedTypesDataFlow now a known failure) - ILCompiler.Trimming.Tests DataFlow (Release): 80 passed Assisted-by: Copilot:claude-sonnet-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4dc4554-6607-4109-ab22-56a5815b294d
|
Azure Pipelines: Successfully started running 4 pipeline(s). 12 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.
Pull request overview
This PR extends the ILLink Roslyn analyzer’s local dataflow analysis to understand deconstruction assignments (IDeconstructionAssignmentOperation), and updates trimming/dataflow test coverage to exercise the new behavior across multiple toolchains.
Changes:
- Implement deconstruction assignment evaluation + assignment sequencing in
LocalDataFlowVisitor, including tuple element handling. - Extend
TrimAnalysisVisitorto model tuple-element values for tuple-typed sources. - Add/adjust regression tests in
ConstructedTypesDataFlow.cs, and mark the ILTrim suite as an expected failure.
Show a summary per file
| File | Description |
|---|---|
| src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs | Adds VisitDeconstructionAssignment support and supporting helpers for deconstruction evaluation/assignment. |
| src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs | Implements tuple-element value modeling needed by deconstruction-from-tuple-type sources. |
| src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ConstructedTypesDataFlow.cs | Adds deconstruction regression coverage across many target/source forms and adjusts expected diagnostics. |
| src/coreclr/tools/ILTrim.Tests/ILTrimExpectedFailures.txt | Marks the full DataFlow.ConstructedTypesDataFlow suite as an expected ILTrim failure. |
Copilot's findings
- Files reviewed: 4/4 changed files
- Comments generated: 1
EvaluateDeconstruction keyed the synthesized Deconstruct() call on 'source ?? target'. If the source expression already had its own tracked call pattern (e.g. it's itself a method call), reusing that IOperation as the key made the pattern store merge two unrelated calls (mismatched called method/argument count), crashing in Release and asserting in Debug. Flagged by Copilot's review of dotnet#131624. Fix by keying on 'target' (the tuple pattern) instead, which is never used as a call operation key elsewhere and is unique per nesting level. Call HandleMethodCall directly rather than through HandleMethodCallHelper, since the latter's DoesNotReturnIf handling assumes an IInvocationOperation/IObjectCreationOperation. Added DeconstructMethodCallSource, a regression test with a [RequiresUnreferencedCode] method call as the Deconstruct() source. Verified it crashes the analyzer without the fix and passes cleanly with it. Verified across all four toolchains: - ILLink.RoslynAnalyzer.Tests DataFlow: 81 passed, 2 skipped - Mono.Linker.Tests DataFlowTests: 80 passed - ILTrim.Tests DataFlowTests: 42 passed, 38 skipped (unchanged) - ILCompiler.Trimming.Tests DataFlow (Release): 80 passed Assisted-by: Copilot:claude-sonnet-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4dc4554-6607-4109-ab22-56a5815b294d
Two review fixes. 1. A deconstruction element that goes through a user-defined conversion operator produced TopValue. TopValue is the empty ValueSet, and TrimAnalysisAssignmentPattern.ReportDiagnostics iterates over the source values, so an empty source meant zero checks and zero diagnostics - silently dropping the annotation check. Assigning such a value to an annotated target produced no warning, while the equivalent non-deconstruction assignment correctly warned IL2074. Model the converted value the same way VisitConversion does, as the conversion operator's return value, via a new GetConversionValue hook. This path is only reachable when the conversion is described by the DeconstructionInfo (a tuple-typed source); for a tuple literal the conversion appears as an IConversionOperation in the tree and was already handled by VisitConversion. DeconstructWithUserDefinedConversion couldn't catch this because its target type isn't tracked for dataflow, so it passed regardless of the value produced. Added DeconstructUserDefinedConversionToAnnotatedTarget, which fails without this fix. 2. Re-disable the two Debug.Asserts in VisitFlowCaptureReference and VisitPropertyReference. Deconstruction was not the only thing reaching them: increment/decrement and coalescing assignment are not handled and fall back to the base visitor, which visits the write target directly, so a plain 'obj.Property++', 'obj.Property ??= x' or 'obj[i]++' fails the assert. Since the analyzer is built in the repo's own configuration, that turns into an AD0001 analyzer exception and breaks the build in Debug/Checked builds. Handling those operations is unrelated to deconstruction, so leave the asserts off and record what still needs to happen before they can be enabled. Verified across all four toolchains: - ILLink.RoslynAnalyzer.Tests DataFlow: 81 passed, 2 skipped - Mono.Linker.Tests DataFlowTests: 80 passed - ILTrim.Tests DataFlowTests: 42 passed, 38 skipped - ILCompiler.Trimming.Tests DataFlow (Release): 80 passed Assisted-by: Copilot:claude-opus-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4dc4554-6607-4109-ab22-56a5815b294d
VisitConversion and GetConversionValue had the same logic, with a comment asking to keep them in sync. Have VisitConversion call GetConversionValue instead, so there's a single implementation and the two paths can't drift. Verified: full ILLink.RoslynAnalyzer.Tests suite, 1201 passed, 9 skipped. Assisted-by: Copilot:claude-opus-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4dc4554-6607-4109-ab22-56a5815b294d
A deconstruction evaluates the sub-expressions that identify a target
location (a receiver, or an index) before reading any source value, but
the write can only happen after all source values have been read. The
write went back and visited those same sub-expressions a second time,
which meant they were analyzed twice with the state from before and
after the source. Both observations end up merged into the same
operation's analysis, so a value that the expression can never actually
see is treated as if it could reach it.
For example, in
Type type = annotated;
(GetHolder(type).AnnotatedProperty, other) = ((type = unannotated), new object());
GetHolder is called before the source runs, so only the annotated value
can ever reach it, but the analyzer also saw the second visit where
'type' holds the unannotated value and reported a warning for a call
that can't happen.
Remember the values produced when evaluating the target sub-expressions
and reuse them when performing the write, instead of evaluating the
expressions again. Ordinary assignments evaluate the target and the
value in one pass and never record anything, so they are unaffected.
Verified across all four toolchains:
- ILLink.RoslynAnalyzer.Tests: 1201 passed, 9 skipped (full suite)
- Mono.Linker.Tests DataFlowTests: 80 passed
- ILTrim.Tests DataFlowTests: 42 passed, 38 skipped
- ILCompiler.Trimming.Tests DataFlow (Release): 80 passed
Assisted-by: Copilot:claude-opus-5
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: b4dc4554-6607-4109-ab22-56a5815b294d
Keep each deconstruction's saved target values in a local dictionary and pass them only through its target evaluation and write paths. Nested deconstructions naturally use their own call-frame state, removing the visitor field and save/restore logic. Assisted-by: Copilot:gpt-5.6-sol Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4dc4554-6607-4109-ab22-56a5815b294d
Move the two unsupported ILTrim deconstruction scenarios into standalone shared test cases so ConstructedTypesDataFlow continues to run. Keep strict analyzer checks for the moved warnings. Assisted-by: Copilot:gpt-5.6-sol Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4dc4554-6607-4109-ab22-56a5815b294d
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4dc4554-6607-4109-ab22-56a5815b294d Assisted-by: GitHub Copilot CLI:gpt-5.6-sol
There was a problem hiding this comment.
Copilot's findings
Suppressed comments (2)
src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs:769
- The synthesized Deconstruct() call is dispatched via HandleMethodCall directly, which bypasses the base visitor’s DoesNotReturn handling. If a user-defined Deconstruct method is annotated with [DoesNotReturn], the current implementation would still proceed to evaluate nested values and assign targets, potentially producing warnings/state changes after a non-returning call.
Consider explicitly honoring DoesNotReturn here by resetting state to Top and stopping deconstruction assignment processing when the attribute is present.
HandleMethodCall(
deconstructMethod,
instanceValue,
arguments.MoveToImmutable(),
target,
src/coreclr/tools/ILTrim.Tests/ILTrimExpectedFailures.txt:87
- The PR description says only DeconstructFieldTarget is added to ILTrimExpectedFailures.txt due to an ILTrim limitation, but this change also adds DeconstructUserDefinedConversion. Either the description should be updated to explain why this additional case is expected to fail under ILTrim, or the extra entry should be removed if it’s unintended.
DataFlow.DeconstructFieldTarget
DataFlow.DeconstructUserDefinedConversion
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
Map C# extension-block receivers using metadata parameter positions so synthesized Deconstruct calls propagate receiver annotations correctly. Assisted-by: GitHub Copilot:gpt-5.6-sol Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4dc4554-6607-4109-ab22-56a5815b294d
Propagate DoesNotReturn through nested deconstruction evaluation, suppress target assignments on unreachable paths, and cover direct and nested cases across analyzer and IL-based tools. Assisted-by: GitHub Copilot:gpt-5.6-sol Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4dc4554-6607-4109-ab22-56a5815b294d
There was a problem hiding this comment.
Copilot's findings
Suppressed comments (2)
src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalStateLattice.cs:80
- New public types were introduced into the analyzer's public surface (
CapturedTargetKey,CapturedTargetValue<TValue>,CapturedTargetValueLattice<,>). If this code isn't intended to be a supported public API, these should be non-public (and/or the containing public state types reshaped) to avoid expanding API surface without approval.
public readonly struct CapturedTargetKey : IEquatable<CapturedTargetKey>
{
private readonly IOperation Operation;
public CapturedTargetKey(IOperation operation) => Operation = operation;
public bool Equals(CapturedTargetKey other) => Operation == other.Operation;
public override bool Equals(object obj)
=> obj is CapturedTargetKey inst && Equals(inst);
public override int GetHashCode() => Operation.GetHashCode();
}
public readonly struct CapturedTargetValue<TValue> : IEquatable<CapturedTargetValue<TValue>>, IDeepCopyValue<CapturedTargetValue<TValue>>
where TValue : IEquatable<TValue>
{
public readonly bool HasValue;
public readonly TValue Value;
public CapturedTargetValue(TValue value) => (HasValue, Value) = (true, value);
public bool Equals(CapturedTargetValue<TValue> other) =>
HasValue == other.HasValue &&
(!HasValue || EqualityComparer<TValue>.Default.Equals(Value, other.Value));
public override bool Equals(object obj)
=> obj is CapturedTargetValue<TValue> inst && Equals(inst);
public override int GetHashCode() => HasValue ? EqualityComparer<TValue>.Default.GetHashCode(Value) : 0;
public CapturedTargetValue<TValue> DeepCopy() =>
HasValue
? new CapturedTargetValue<TValue>(
Value is IDeepCopyValue<TValue> copyValue ? copyValue.DeepCopy() : Value)
: default;
}
public readonly struct CapturedTargetValueLattice<TValue, TValueLattice> : ILattice<CapturedTargetValue<TValue>>
where TValue : IEquatable<TValue>
where TValueLattice : ILattice<TValue>
src/coreclr/tools/ILTrim.Tests/ILTrimExpectedFailures.txt:87
- PR description says only
DeconstructFieldTargetneeded adding toILTrimExpectedFailures.txt, but this file adds bothDataFlow.DeconstructFieldTargetandDataFlow.DeconstructUserDefinedConversion. Either update the PR description/testing notes to reflect both expected failures, or drop the extra expected-failure entry if it isn't required.
DataFlow.DeconstructFieldTarget
DataFlow.DeconstructUserDefinedConversion
- Files reviewed: 9/9 changed files
- Comments generated: 1
Fixes #123767.
The ILLink Roslyn analyzer's dataflow visitor did not handle
IDeconstructionAssignmentOperation, soRequiresUnreferencedCodeandDynamicallyAccessedMembersdiagnostics were silently skipped for values flowing through deconstruction, including assignments and deconstruction inforeach.Summary of changes
Deconstructmethods, nested deconstruction, and user-defined conversions.Deconstructcalls have been evaluated.Deconstructmethods.[DoesNotReturn] Deconstructcalls as unreachable and suppress every target assignment on that path.Testing
Added regression coverage for deconstruction sources, targets, evaluation ordering, flow captures, extension receivers, user-defined conversions, and direct and nested
[DoesNotReturn]methods. The DataFlow suites pass across:ILLink.RoslynAnalyzer.TestsMono.Linker.TestsILCompiler.Trimming.TestsILTrim.TestsTwo unsupported ILTrim scenarios are isolated as standalone expected failures instead of excluding the shared deconstruction coverage:
DeconstructFieldTargetexposes an existing ILTrim.Core gap in static-field write dataflow.DeconstructUserDefinedConversionexposes an existing ILTrim.Core gap in tracking a user-defined conversion operator's return value.The Roslyn analyzer, linker, and NativeAOT tests retain coverage for both cases. ILLink/ILTrim and NativeAOT also conservatively report the expected warning after
[DoesNotReturn]calls because the IL-based tools do not treat that attribute as a reachability contract; those expectations are scoped to the IL-based tools while the Roslyn analyzer verifies the path is unreachable.Note
This content was created with assistance from AI.