Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use correct get / set inside ref safety analysis #73842

Merged
merged 26 commits into from
Jul 12, 2024
Merged

Use correct get / set inside ref safety analysis #73842

merged 26 commits into from
Jul 12, 2024

Conversation

jaredpar
Copy link
Member

@jaredpar jaredpar commented Jun 4, 2024 •
edited
Loading

This fixes a few cases where our code looked at the wrong accessor when doing ref safety analysis.

closes #35606
closes #73550
related #73872
related dotnet/csharplang#8253

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jun 4, 2024
@jaredpar
Remove unnecesary code
64f5e1d 
This code path for checking the receiver is unnecessary. The receiver is
included in `escapeArguments` below where the same check happens.

Added a test that hit the old code path and failed and verified it still
fails after this change.

closes dotnet#73550
jaredpar
jaredpar commented Jun 13, 2024
View reviewed changes
src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs
@@ -409,10 +518,12 @@ private BoundExpression CheckValue(BoundExpression expr, BindValueKind valueKind
switch (expr.Kind)
{
case BoundKind.PropertyGroup:
expr = BindIndexedPropertyAccess((BoundPropertyGroup)expr, mustHaveAllOptionalParameters: false, diagnostics: diagnostics);
if (expr is BoundIndexerAccess indexerAccess)
{
Copy link
Member Author

Choose a reason for hiding this comment

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

Put this inside { ... } blocks as it's now declaring variables.

jaredpar
jaredpar commented Jun 13, 2024
View reviewed changes
src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs
@@ -2474,17 +2615,6 @@ private static string GetInvocationParameterName(ParameterSymbol? parameter)
Error(diagnostics, errorCode, syntax, symbol, parameterName);
}

private bool UseUpdatedEscapeRulesForInvocation(Symbol symbol)
Copy link
Member Author

@jaredpar jaredpar Jun 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Moved to MethodInfo.UseUpdatedEscapeRules

jaredpar
jaredpar commented Jun 13, 2024
View reviewed changes
src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs
// https://github.com/dotnet/roslyn/issues/73550:
// We do not have a test that demonstrates that the statement below makes a difference.
// If it is commented out, not a single test fails
escapeTo = GetValEscape(receiverOpt, scopeOfTheContainingExpression);
Copy link
Member Author

Choose a reason for hiding this comment

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

This code is indeed unnecessary. Thinking through the check for IsReceiverRefReadOnly is what caused me to dig into the get / set issues with indexers though.

jaredpar
jaredpar commented Jun 13, 2024
View reviewed changes
src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs
@@ -1961,21 +2091,6 @@ bool isRefEscape
return escapeScope;
}

private static bool ReturnsRefToRefStruct(Symbol symbol)
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to MethodInfo.ReturnsRefToRefStruct

@jaredpar jaredpar marked this pull request as ready for review June 13, 2024 04:33
@jaredpar jaredpar requested a review from a team as a code owner June 13, 2024 04:33
@jaredpar
Copy link
Member Author

@dotnet/roslyn-compiler PTAL

1 similar comment
@jaredpar
Copy link
Member Author

@dotnet/roslyn-compiler PTAL

cston
cston previously approved these changes Jun 15, 2024
View reviewed changes
Copy link
Member

@cston cston left a comment

Choose a reason for hiding this comment

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

A few suggestions, but nothing blocking.

@jaredpar jaredpar mentioned this pull request Jul 10, 2024
Merged
@jaredpar
Copy link
Member Author

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jaredpar jaredpar requested a review from a team as a code owner July 12, 2024 11:41
@build-analysis build-analysis bot mentioned this pull request Jul 12, 2024
Open
3 tasks
@jaredpar jaredpar merged commit 5c77803 into dotnet:main Jul 12, 2024
28 checks passed
@jaredpar jaredpar deleted the ref-fix branch July 12, 2024 17:07
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Jul 12, 2024
This was referenced Jul 16, 2024
Closed
Closed
@RikkiGibson RikkiGibson modified the milestones: Next, 17.12 P1 Jul 29, 2024
@dotnet-bot dotnet-bot mentioned this pull request Aug 2, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlekseyTs AlekseyTs AlekseyTs left review comments

@jjonescz jjonescz jjonescz left review comments

@cston cston cston approved these changes

@RikkiGibson RikkiGibson RikkiGibson approved these changes

Assignees

@RikkiGibson RikkiGibson

Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Milestone
17.12 P1
5 participants
@jaredpar @AlekseyTs @jjonescz @RikkiGibson @cston