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.

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

Disallow more locals of restricted type in async methods #66264

Merged
merged 5 commits into from Jan 12, 2023

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Jan 5, 2023

Fixes #62747

The bug was that we allowed declaring locals for restricted type in async methods inside foreach. For example, foreach (Span<int> s in ...) in async methods.
Looking around, I noticed a couple other scenarios with the same issue:

  1. deconstruction declarations,
  2. using (expression) which lowers to use a temp

@jcouv jcouv self-assigned this Jan 5, 2023
@jcouv jcouv added this to the 17.6 milestone Jan 5, 2023
@jcouv jcouv marked this pull request as ready for review January 5, 2023 19:36
@jcouv jcouv requested a review from a team as a code owner January 5, 2023 19:36
@jcouv
Copy link
Member Author

jcouv commented Jan 10, 2023

@dotnet/roslyn-compiler for second review. Thanks

@@ -298,6 +298,11 @@ private BoundForEachStatement BindForEachPartsWorker(BindingDiagnosticBag diagno
SourceLocalSymbol local = this.IterationVariable;
local.SetTypeWithAnnotations(declType);

if (CheckRestrictedTypeInAsyncMethod(this.ContainingMemberOrLambda, declType.Type, diagnostics, typeSyntax))
{
hasErrors = true;
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 11, 2023

Choose a reason for hiding this comment

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

hasErrors = true;

What is our intent here? We don't need to record every possible error in a bound node. The only requirement to set HasError flag is when the node is completely broken. I do not think it is the case here. We were able to bind all things. #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

Setting HasError without a good reason is likely to disable some useful analysis

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 was out of consistency with other callers of CheckRestrictedTypeInAsyncMethod. But I agree this error doesn't prevent us from binding.

Debug.Assert(expressionOpt is not null);
if (expressionOpt.Type is not null)
{
CheckRestrictedTypeInAsyncMethod(originalBinder.ContainingMemberOrLambda, expressionOpt.Type, diagnostics, expressionOpt.Syntax);
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 11, 2023

Choose a reason for hiding this comment

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

CheckRestrictedTypeInAsyncMethod(originalBinder.ContainingMemberOrLambda, expressionOpt.Type, diagnostics, expressionOpt.Syntax);

Is the wording of the error going to be confusing here?There is no user declared local here #Closed

public async Task M()
{
(Span<int> s1, Span<int> s2) = new Program(); // 1, 2
var (s3, s4) = new Program(); // 3, 4
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 11, 2023

Choose a reason for hiding this comment

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

var (s3, s4) = new Program();

Consider also testing scenario like (var s5, var s6) = new Program(); #Closed

";
var comp = CreateCompilation(src, targetFramework: TargetFramework.Net70);
comp.VerifyDiagnostics(
// (8,16): error CS4012: Parameters or locals of type 'RS' cannot be declared in async methods or async lambda expressions.
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 11, 2023

Choose a reason for hiding this comment

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

// (8,16): error CS4012: Parameters or locals of type 'RS' cannot be declared in async methods or async lambda expressions.

We probably should add a dedicated error for this case, that doesn't talk about locals and parameters #Closed

using (default(RS)) { } // 1
using (var s1 = default(RS)) { } // 2
using (RS s2 = default(RS)) { } // 3
using RS s3 = default(RS); // 4
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 11, 2023

Choose a reason for hiding this comment

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

using RS s3 = default(RS);

Consider testing var case as well #Closed

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 1)

@jcouv jcouv requested a review from AlekseyTs January 12, 2023 00:53
";
var comp = CreateCompilation(src, targetFramework: TargetFramework.Net70);
comp.VerifyDiagnostics(
// (8,16): error CS9104: Values of type 'RS' cannot be used in async methods or async lambda expressions.
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 12, 2023

Choose a reason for hiding this comment

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

Values of type 'RS' cannot be used in async methods or async lambda expressions.

I think that this wording is also confusing, and quite possibly plain incorrect. Values of RS can be used in an async method. For example, the following code compiles today, and I think will continue compiling with changes in this PR, but it is using value of RS type in an async method:
https://sharplab.io/#v2:EYLgtghglgdgNAFxFANgHwAIAYAEGCMArANwCwAUBRgMx4BMe+A7BQN4U6d60YAceANhwBZABQBKDl3bkucvAE5BAOgAiAUxQQAnqPziys+ZwDi6hACUAyhLVQAzgAcA9vfUTDxzhiaeuAXylOIMYhaxwzSxtxHABeAD4cABN1ADMIAFcUBENAynIaHAAnNJx7BCKMgGMEHGs2EMKMABYcVQcXNwkcVhw8/yA===

using System.Threading.Tasks;

public class Program
{
    public async Task M()
    {
        await Task.Delay(1);
        GetRS().Dispose();
        return;
    }
    
    static RS GetRS() => default;
}

public ref struct RS
{
    public void Dispose() { }
}

The problem is that we cannot spill these types, not every usage requires spilling though.

So, I think the wording should be more specific to the situation. I would use something like: "A using statement resource of type 'RS' cannot be used in async methods or async lambda expressions." #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

Or, if we want to have a more general wording, something like: "Values of type 'RS' cannot be used in async methods or async lambda expressions in this context."

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 3)

@jcouv jcouv requested a review from AlekseyTs January 12, 2023 16:53
/// </summary>
internal static bool CheckRestrictedTypeInAsyncMethod(Symbol containingSymbol, TypeSymbol type, BindingDiagnosticBag diagnostics, SyntaxNode syntax)
internal static void CheckRestrictedTypeInAsyncMethod(Symbol containingSymbol, TypeSymbol type, BindingDiagnosticBag diagnostics, SyntaxNode syntax, bool reportOnTemp = false)
Copy link
Contributor

Choose a reason for hiding this comment

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

reportOnTemp

forUsingExpression?

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 4), with a parameter rename suggestion

@jcouv jcouv enabled auto-merge (squash) January 12, 2023 17:08
@jcouv jcouv merged commit 7dde777 into dotnet:main Jan 12, 2023
@ghost ghost removed this from the 17.6 milestone Jan 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Compiler: Julien's umbrellas
Active/Investigating
Development

Successfully merging this pull request may close these issues.

bug either in Roslyn or in the C# language spec — example with ReadOnlySpan in a foreach expansion
4 participants