Skip to content

Conversation

@jjonescz
Copy link
Member

Fixes #80217.

@jjonescz jjonescz marked this pull request as ready for review September 11, 2025 09:45
@jjonescz jjonescz requested a review from a team as a code owner September 11, 2025 09:45
{
var source = """
var (a, b, c) = 42;
var ((d, _, _), (_, e, _), (_, _, f)) = 42;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe have tests where the outer or inner counts don't match the deconstruct out args count? Maybe tests where there are multiple deconstructs?

Copy link
Member Author

@jjonescz jjonescz Sep 11, 2025

Choose a reason for hiding this comment

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

Added, thanks.

}
}
""";
CompileAndVerify(source, expectedOutput: "1 2 3 4 5 4 5").VerifyDiagnostics();
Copy link
Member

Choose a reason for hiding this comment

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

wild that this works :) i didn't know wwe had this capability!

@AlekseyTs
Copy link
Contributor

Consider using more descriptive/specific title that closely reflects the nature of the change. What specific component is affected, etc. For example, "RefSafetyAnalysis: Fix handling of nested deconstruction utilizing modern extensions "

var (placeholder, placeholderConversion) = conversion.DeconstructConversionInfo[i];
var underlyingConversion = BoundNode.GetConversion(placeholderConversion, placeholder);
VisitDeconstructionArguments(nestedVariables, syntax, underlyingConversion, right: invocation.Arguments[i + offset]);
VisitDeconstructionArguments(nestedVariables, syntax, underlyingConversion, right: methodInvocationInfo.ArgsOpt[i + offset]);
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 11, 2025

Choose a reason for hiding this comment

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

methodInvocationInfo.ArgsOpt[i + offset]

Was the original code correct for all not nested scenarios? #Closed

}
}
""";
CreateCompilation(source).VerifyDiagnostics(
Copy link
Contributor

Choose a reason for hiding this comment

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

VerifyDiagnostics

VerifyEmitDiagnostics?

}
}
""";
CreateCompilation(source).VerifyDiagnostics(
Copy link
Contributor

Choose a reason for hiding this comment

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

VerifyDiagnostics

VerifyEmitDiagnostics?

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 2)

@jjonescz jjonescz changed the title Fix nested extension deconstruction RefSafetyAnalysis: Fix handling of nested deconstruction utilizing modern extensions Sep 11, 2025
@jjonescz jjonescz merged commit de9d72e into dotnet:main Sep 11, 2025
24 checks passed
@jjonescz jjonescz deleted the 80217-ExtensionDeconstruction branch September 11, 2025 13:23
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Sep 11, 2025
@jcouv jcouv added the Feature - Extension Everything The extension everything feature label Sep 11, 2025
333fred added a commit to 333fred/roslyn that referenced this pull request Sep 11, 2025
* upstream/main: (233 commits)
  Extensions: add SyntaxGenerator support and AssociatedExtensionImplementation API (dotnet#80170)
  Fix error when hoisting a non-ref call (dotnet#80138)
  Ensure that refkinds are rewritten for complex methods (dotnet#79916)
  Revert
  Do not go through the workspace to access services
  DefiniteAssignmentPass.MarkFieldsUsed - avoid infinite recursion due to generic substitution (dotnet#80135)
  Reduce allocations in AnalyzerDriver.TryExecuteSymbolEndActions (dotnet#79855)
  RefSafetyAnalysis: Fix handling of nested deconstruction utilizing modern extensions (dotnet#80231)
  Extensions: adjust rewriting of anonymous type property symbols (dotnet#80211)
  Extensions: Move public APIs into INamedTypeSymbol (dotnet#80230)
  Extensions: improve error recovery in older language versions (dotnet#80206)
  Fall back to `dotnet exec` if apphost does not exist (dotnet#80153)
  Update dependencies from https://github.com/dotnet/dotnet build 282708 (dotnet#80228)
  Add a workaround for microsoft/vs-mef#620
  Revert "FailFast if the MEF composition is clearly broken"
  switch from windows combobox to visualstudio combobox (dotnet#80219)
  Update System.Text.Json in packages which use 4.12 Roslyn (dotnet#80197)
  add flags to unblock CI (dotnet#80222)
  Move static members to another type - qualifies static member references in the moved members (dotnet#80178)
  Fix broken link for C# 14 lambda parameter modifiers
  ...
@akhera99 akhera99 modified the milestones: Next, 18.0 P1, 18.0 P2 Sep 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Compilers Feature - Extension Everything The extension everything feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Nested variable deconstruction comsuming modern extension methods causes a compiler crash

5 participants