Skip to content

Analyze flow in receiver of extension method group in delegate creation #60093

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

Merged
merged 4 commits into from
Mar 16, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1513,15 +1513,15 @@ public override BoundNode VisitDelegateCreationExpression(BoundDelegateCreationE
var methodGroup = node.Argument as BoundMethodGroup;
if (methodGroup != null)
{
if ((object)node.MethodOpt != null && node.MethodOpt.RequiresInstanceReceiver)
if (node.MethodOpt?.OriginalDefinition is LocalFunctionSymbol localFunc)
{
EnterRegionIfNeeded(methodGroup);
VisitRvalue(methodGroup.ReceiverOpt);
LeaveRegionIfNeeded(methodGroup);
VisitLocalFunctionUse(localFunc, node.Syntax, isCall: false);
}
else if (node.MethodOpt?.OriginalDefinition is LocalFunctionSymbol localFunc)
else if (node.MethodOpt is { } method && methodGroup.ReceiverOpt is { } receiver && !ignoreReceiver(receiver, method))
{
VisitLocalFunctionUse(localFunc, node.Syntax, isCall: false);
EnterRegionIfNeeded(methodGroup);
VisitRvalue(receiver);
LeaveRegionIfNeeded(methodGroup);
}
}
else
Expand All @@ -1530,6 +1530,12 @@ public override BoundNode VisitDelegateCreationExpression(BoundDelegateCreationE
}

return null;

static bool ignoreReceiver(BoundExpression receiver, MethodSymbol method)
{
// ignore the implicit `this` receiver on a static method
return method.IsStatic && receiver is { Kind: BoundKind.ThisReference, WasCompilerGenerated: true };
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we should add a comment here:
we may need to visit the receiver / can't ignore static methods at all because of extension methods.

Copy link
Contributor

@AlekseyTs AlekseyTs Mar 14, 2022

Choose a reason for hiding this comment

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

Kind: BoundKind.ThisReference, WasCompilerGenerated: true }

I think we can have this as a receiver for a legitimate extension method reference. We have a special property IsExtensionMethod clearly identifying extension methods. Consider using it instead and, if it is not set, ignoring receivers for a static method. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason I used this logic was to align with a previous PR that skips such implicit this receivers on static methods: #56395

Just to clarify, I think you're proposing return method.IsStatic && !method.IsExtensionMethod;.
I'd be fine with that. @333fred, any concern?

Copy link
Member

Choose a reason for hiding this comment

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

That might be too broad. AnalyzeDataFlow should still see the invalid receiver as being read for extract method to work as intended.

Copy link
Contributor

Choose a reason for hiding this comment

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

AnalyzeDataFlow should still see the invalid receiver as being read for extract method to work as intended.

I am not sure if I agree with that. Clearly, the receiver for static methods (excluding extension scenario) is not going to be evaluated, so I think we have a flexibility in terms of how flow analysis should work in the error scenario.
The current implementation is incorrect in my opinion. Primarily because the PR description provides no clarity about what is our philosophy around visiting and not visiting receivers. Also, it is not clear to me why do we want to handle receivers differently by comparison to BoundCall. Perhaps the behavior is the same, but it is not obvious from the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

In general, I do not see us actually making sure that region analysis will work for any misplaced expression. For example, there are BoundLocalDeclaration.ArgumentsOpt and it looks like flow analysis never visits them. Therefore, it is not clear to me why we should visit erroneous (misplaced) receiver. BTW, the issue that we are trying to fix is about failure for a valid scenario.

}
}

public override BoundNode VisitTypeExpression(BoundTypeExpression node)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9175,5 +9175,223 @@ public void F(int x)
}");
Assert.Equal("x, a", GetSymbolNamesJoined(analysis.DataFlowsIn));
}

[Fact, WorkItem(59738, "https://github.com/dotnet/roslyn/issues/59738")]
public void TestDataFlowsOfIdentifierWithDelegateConversion()
{
var results = CompileAndAnalyzeDataFlowExpression(@"
using System;

internal static class NoExtensionMethods
{
internal static Func<T> AsFunc<T>(this T value) where T : class
{
return new Func<T>(/*<bind>*/ value /*</bind>*/.Return);
}

private static T Return<T>(this T value)
{
return value;
}

static void Main()
{
Console.WriteLine(((object)42).AsFunc()());
}
}
");
Assert.True(results.Succeeded);
Assert.Null(GetSymbolNamesJoined(results.Captured));
Assert.Null(GetSymbolNamesJoined(results.CapturedInside));
Assert.Null(GetSymbolNamesJoined(results.CapturedOutside));
Assert.Null(GetSymbolNamesJoined(results.VariablesDeclared));
Assert.Null(GetSymbolNamesJoined(results.DataFlowsOut));
Assert.Equal("value", GetSymbolNamesJoined(results.DefinitelyAssignedOnEntry));
Assert.Equal("value", GetSymbolNamesJoined(results.DefinitelyAssignedOnExit));
Assert.Equal("value", GetSymbolNamesJoined(results.ReadInside));
Assert.Null(GetSymbolNamesJoined(results.WrittenInside));
Assert.Null(GetSymbolNamesJoined(results.ReadOutside));
Assert.Equal("value", GetSymbolNamesJoined(results.WrittenOutside));
Assert.Null(GetSymbolNamesJoined(results.UsedLocalFunctions));
}

[Fact]
public void TestDataFlowsOfIdentifierWithDelegateConversionCast()
{
var analysis = CompileAndAnalyzeDataFlowExpression(@"
using System;

internal static class NoExtensionMethods
{
internal static Func<T> AsFunc<T>(this T value) where T : class
{
return (Func<T>)/*<bind>*/ value /*</bind>*/.Return;
}

private static T Return<T>(this T value)
{
return value;
}
}
");
Assert.True(analysis.Succeeded);
Assert.Null(GetSymbolNamesJoined(analysis.AlwaysAssigned));
Assert.Null(GetSymbolNamesJoined(analysis.Captured));
Assert.Null(GetSymbolNamesJoined(analysis.CapturedInside));
Assert.Null(GetSymbolNamesJoined(analysis.CapturedOutside));
Assert.Equal("value", GetSymbolNamesJoined(analysis.DataFlowsIn));
Assert.Equal("value", GetSymbolNamesJoined(analysis.DefinitelyAssignedOnEntry));
Assert.Equal("value", GetSymbolNamesJoined(analysis.DefinitelyAssignedOnExit));
Assert.Equal("value", GetSymbolNamesJoined(analysis.ReadInside));
Assert.Null(GetSymbolNamesJoined(analysis.ReadOutside));
Assert.Null(GetSymbolNamesJoined(analysis.VariablesDeclared));
Assert.Null(GetSymbolNamesJoined(analysis.WrittenInside));
Assert.Equal("value", GetSymbolNamesJoined(analysis.WrittenOutside));
}

[Fact]
public void TestDataFlowsOfIdentifierWithDelegateConversionTarget()
{
var analysis = CompileAndAnalyzeDataFlowExpression(@"
using System;

internal static class NoExtensionMethods
{
internal static Func<T> AsFunc<T>(this T value) where T : class
{
Func<T> result =/*<bind>*/ value /*</bind>*/.Return;
return result;
}

private static T Return<T>(this T value)
{
return value;
}
}
");
Assert.True(analysis.Succeeded);
Assert.Null(GetSymbolNamesJoined(analysis.AlwaysAssigned));
Assert.Null(GetSymbolNamesJoined(analysis.Captured));
Assert.Null(GetSymbolNamesJoined(analysis.CapturedInside));
Assert.Null(GetSymbolNamesJoined(analysis.CapturedOutside));
Assert.Equal("value", GetSymbolNamesJoined(analysis.DataFlowsIn));
Assert.Equal("value", GetSymbolNamesJoined(analysis.DefinitelyAssignedOnEntry));
Assert.Equal("value", GetSymbolNamesJoined(analysis.DefinitelyAssignedOnExit));
Assert.Equal("value", GetSymbolNamesJoined(analysis.ReadInside));
Assert.Equal("result", GetSymbolNamesJoined(analysis.ReadOutside));
Assert.Null(GetSymbolNamesJoined(analysis.VariablesDeclared));
Assert.Null(GetSymbolNamesJoined(analysis.WrittenInside));
Assert.Equal("value, result", GetSymbolNamesJoined(analysis.WrittenOutside));
}

[Fact, WorkItem(59738, "https://github.com/dotnet/roslyn/issues/59738")]
public void DefiniteAssignmentInReceiverOfExtensionMethodInDelegateCreation()
{
var comp = CreateCompilation(@"
using System;
bool b = true;
_ = new Func<string>((b ? M(out var i) : i.ToString()).ExtensionMethod);

string M(out int i)
{
throw null;
}

static class Extension
{
public static string ExtensionMethod(this string s) => throw null;
}
");
comp.VerifyDiagnostics(
// (4,42): error CS0165: Use of unassigned local variable 'i'
// _ = new Func<string>((b ? M(out var i) : i.ToString()).ExtensionMethod);
Diagnostic(ErrorCode.ERR_UseDefViolation, "i").WithArguments("i").WithLocation(4, 42)
);
}

[Fact, WorkItem(59738, "https://github.com/dotnet/roslyn/issues/59738")]
public void DefiniteAssignmentShouldSkipImplicitThisInStaticMethodConversion()
{
var comp = CreateCompilation(@"
using System;
public struct C
{
private object field;
public C(Action a)
{
// implicit `this` receiver should be ignored in definite assignment
a = new(M);
field = 1;
}

public C(Action a, int ignored)
{
// implicit `this` receiver should be ignored in definite assignment
a = new Action(M);
field = 1;
}

public void Method1(Action a)
{
// explicit `this` disallowed
a = new Action(this.M);
}

public void Method2(Action a, C c)
{
// instance receiver disallowed
a = new Action(c.M);
}

private static void M()
{
}
}
");
comp.VerifyDiagnostics(
// (23,24): error CS0176: Member 'C.M()' cannot be accessed with an instance reference; qualify it with a type name instead
// a = new Action(this.M);
Diagnostic(ErrorCode.ERR_ObjectProhibited, "this.M").WithArguments("C.M()").WithLocation(23, 24),
// (29,24): error CS0176: Member 'C.M()' cannot be accessed with an instance reference; qualify it with a type name instead
// a = new Action(c.M);
Diagnostic(ErrorCode.ERR_ObjectProhibited, "c.M").WithArguments("C.M()").WithLocation(29, 24)
);
}

[Fact, WorkItem(59738, "https://github.com/dotnet/roslyn/issues/59738")]
public void DefiniteAssignmentWithExplicitThisInStaticMethodConversion()
{
var comp = CreateCompilation(@"
using System;
public struct C
{
private object field;
public void Method1(Action a)
{
a = new Action(this.M);
field = 1;
}

public void Method2(Action a, C c)
{
a = new Action(c.M);
}
}
public static class Extension
{
public static void M(this C c)
{
}
}
");
comp.VerifyDiagnostics(
// (8,24): error CS1113: Extension method 'Extension.M(C)' defined on value type 'C' cannot be used to create delegates
// a = new Action(this.M);
Diagnostic(ErrorCode.ERR_ValueTypeExtDelegate, "this.M").WithArguments("Extension.M(C)", "C").WithLocation(8, 24),
// (14,24): error CS1113: Extension method 'Extension.M(C)' defined on value type 'C' cannot be used to create delegates
// a = new Action(c.M);
Diagnostic(ErrorCode.ERR_ValueTypeExtDelegate, "c.M").WithArguments("Extension.M(C)", "C").WithLocation(14, 24)
);
}
}
}