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

Function pointer bug fixes #45473

Merged
6 commits merged into from
Jun 27, 2020
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
5 changes: 3 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1671,6 +1671,7 @@ private BoundFunctionPointerInvocation BindFunctionPointerInvocation(SyntaxNode

if (!overloadResolutionResult.Succeeded)
{
ImmutableArray<FunctionPointerMethodSymbol> methods = methodsBuilder.ToImmutableAndFree();
overloadResolutionResult.ReportDiagnostics(
binder: this,
node.Location,
Expand All @@ -1680,15 +1681,15 @@ private BoundFunctionPointerInvocation BindFunctionPointerInvocation(SyntaxNode
boundExpression,
boundExpression.Syntax,
analyzedArguments,
methodsBuilder.ToImmutableAndFree(),
methods,
typeContainingConstructor: null,
delegateTypeBeingInvoked: null,
returnRefKind: funcPtr.Signature.RefKind);

return new BoundFunctionPointerInvocation(
node,
boundExpression,
analyzedArguments.Arguments.SelectAsArray((expr, args) => args.binder.BindToNaturalType(expr, args.diagnostics), (binder: this, diagnostics)),
BuildArgumentsForErrorRecovery(analyzedArguments, StaticCast<MethodSymbol>.From(methods)),
jaredpar marked this conversation as resolved.
Show resolved Hide resolved
analyzedArguments.RefKinds.ToImmutableOrNull(),
LookupResultKind.OverloadResolutionFailure,
funcPtr.Signature.ReturnType,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ private void PerformMemberOverloadResolution<TMember>(
if (isFunctionPointerResolution)
{
RemoveCallingConventionMismatches(results, callingConvention);
RemoveStaticInstanceMismatches(results, requireStatic: true);
RemoveMethodsNotDeclaredStatic(results);
}

// NB: As in dev12, we do this AFTER removing less derived members.
Expand Down Expand Up @@ -387,6 +387,22 @@ private static void RemoveStaticInstanceMismatches<TMember>(ArrayBuilder<MemberR
}
}

private static void RemoveMethodsNotDeclaredStatic<TMember>(ArrayBuilder<MemberResolutionResult<TMember>> results) where TMember : Symbol
{
// RemoveStaticInstanceMistmatches allows methods that do not need a reciever but are not declared static,
// such as a local function that is not declared static. This eliminates methods that are not actually
// declared as static
for (int f = 0; f < results.Count; f++)
{
var result = results[f];
TMember member = result.Member;
if (result.Result.IsValid && !member.IsStatic)
{
results[f] = new MemberResolutionResult<TMember>(member, result.LeastOverriddenMember, MemberAnalysisResult.StaticInstanceMismatch());
}
}
}

private void RemoveConstraintViolations<TMember>(ArrayBuilder<MemberResolutionResult<TMember>> results) where TMember : Symbol
{
// When the feature 'ImprovedOverloadCandidates' is enabled, we do not include methods for which the type arguments
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1303,6 +1303,30 @@ public override BoundNode VisitDelegateCreationExpression(BoundDelegateCreationE
return base.VisitDelegateCreationExpression(node);
}

public override BoundNode VisitFunctionPointerLoad(BoundFunctionPointerLoad node)
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 26, 2020

Choose a reason for hiding this comment

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

VisitFunctionPointerLoad [](start = 34, length = 24)

Did you check other places where we override VisitDelegateCreationExpression to confirm that we don't need to add VisitFunctionPointerLoad? We probably have several visitors that introduce virtual methods with names like that. #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.

I've taken a look. I'll add an override in MethodToClassRewriter, but I can't find a case that it will actually change, as other than this it's only used for iterator and async method rewriting. Since we don't allow you to refer to the compiler-generated underlying methods of these from source, there's nothing that we can test. If we add a new thing that gets remapped in the future, though, it will function as expected.


In reply to: 445956108 [](ancestors = 445956108)

{
if (node.TargetMethod.MethodKind == MethodKind.LocalFunction)
{
Debug.Assert(node.TargetMethod is { RequiresInstanceReceiver: false, IsStatic: true });
ImmutableArray<BoundExpression> arguments = default;
ImmutableArray<RefKind> argRefKinds = default;

RemapLocalFunction(
node.Syntax,
node.TargetMethod,
out var receiver,
out var remappedMethod,
ref arguments,
ref argRefKinds);

Debug.Assert(arguments.IsDefault && argRefKinds.IsDefault && receiver.Kind == BoundKind.TypeExpression);
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 26, 2020

Choose a reason for hiding this comment

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

receiver.Kind == BoundKind.TypeExpression [](start = 77, length = 41)

What is this type? #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.

Made the var explicit and added some more verification of the remapped method.


In reply to: 445955503 [](ancestors = 445955503)


return node.Update(remappedMethod, node.Type);
}

return base.VisitFunctionPointerLoad(node);
}

public override BoundNode VisitConversion(BoundConversion conversion)
{
// a conversion with a method should have been rewritten, e.g. to an invocation
Expand Down
193 changes: 193 additions & 0 deletions src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenFunctionPointersTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7101,6 +7101,199 @@ .locals init (delegate*<System.Span<char>, System.Span<char>> V_0)
");
}

[Fact, WorkItem(45447, "https://github.com/dotnet/roslyn/issues/45447")]
public void LocalFunction_ValidStatic()
{
var verifier = CompileAndVerifyFunctionPointers(@"
unsafe class FunctionPointer
{
public static void Main()
{
delegate*<void> a = &local;
a();

static void local() => System.Console.Write(""local"");
Comment on lines +7121 to +7124
Copy link
Member

@jaredpar jaredpar Jun 26, 2020

Choose a reason for hiding this comment

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

Consider adding a test where the contents of this method body exist within a lambda expression. Verifying the rewriting works in a nested environment. #Resolved

}
}
", expectedOutput: "local");


verifier.VerifyIL("FunctionPointer.Main", @"
{
// Code size 12 (0xc)
.maxstack 1
IL_0000: ldftn ""void FunctionPointer.<Main>g__local|0_0()""
IL_0006: calli ""delegate*<void>""
IL_000b: ret
}
");
}

[Fact, WorkItem(45447, "https://github.com/dotnet/roslyn/issues/45447")]
public void LocalFunction_InvalidNonStatic()
{
var comp = CreateCompilationWithFunctionPointers(@"
unsafe class FunctionPointer
{
public static void M()
{
int local = 1;

delegate*<void> first = &noCaptures;
delegate*<void> second = &capturesLocal;

void noCaptures() { }
void capturesLocal() { local++; }
}
}");

comp.VerifyDiagnostics(
// (8,34): error CS8759: Cannot create a function pointer for 'noCaptures()' because it is not a static method
// delegate*<void> first = &noCaptures;
Diagnostic(ErrorCode.ERR_FuncPtrMethMustBeStatic, "noCaptures").WithArguments("noCaptures()").WithLocation(8, 34),
// (9,35): error CS8759: Cannot create a function pointer for 'capturesLocal()' because it is not a static method
// delegate*<void> second = &capturesLocal;
Diagnostic(ErrorCode.ERR_FuncPtrMethMustBeStatic, "capturesLocal").WithArguments("capturesLocal()").WithLocation(9, 35)
);
}

[Fact, WorkItem(45418, "https://github.com/dotnet/roslyn/issues/45418")]
public void RefMismatchInCall()
{
var comp = CreateCompilationWithFunctionPointers(@"
unsafe class Test
{
void M1(delegate*<ref string, void> param1)
{
param1(out var l);
string s = null;
param1(s);
param1(in s);
}

void M2(delegate*<in string, void> param2)
{
param2(out var l);
string s = null;
param2(s);
param2(ref s);
}

void M3(delegate*<out string, void> param3)
{
string s = null;
param3(s);
param3(ref s);
param3(in s);
}

void M4(delegate*<string, void> param4)
{
param4(out var l);
string s = null;
param4(ref s);
param4(in s);
}
}");

comp.VerifyDiagnostics(
// (6,20): error CS1620: Argument 1 must be passed with the 'ref' keyword
// param1(out var l);
Diagnostic(ErrorCode.ERR_BadArgRef, "var l").WithArguments("1", "ref").WithLocation(6, 20),
// (8,16): error CS1620: Argument 1 must be passed with the 'ref' keyword
// param1(s);
Diagnostic(ErrorCode.ERR_BadArgRef, "s").WithArguments("1", "ref").WithLocation(8, 16),
// (9,19): error CS1620: Argument 1 must be passed with the 'ref' keyword
// param1(in s);
Diagnostic(ErrorCode.ERR_BadArgRef, "s").WithArguments("1", "ref").WithLocation(9, 19),
// (14,20): error CS1615: Argument 1 may not be passed with the 'out' keyword
// param2(out var l);
Diagnostic(ErrorCode.ERR_BadArgExtraRef, "var l").WithArguments("1", "out").WithLocation(14, 20),
// (17,20): error CS1615: Argument 1 may not be passed with the 'ref' keyword
// param2(ref s);
Diagnostic(ErrorCode.ERR_BadArgExtraRef, "s").WithArguments("1", "ref").WithLocation(17, 20),
// (23,16): error CS1620: Argument 1 must be passed with the 'out' keyword
// param3(s);
Diagnostic(ErrorCode.ERR_BadArgRef, "s").WithArguments("1", "out").WithLocation(23, 16),
// (24,20): error CS1620: Argument 1 must be passed with the 'out' keyword
// param3(ref s);
Diagnostic(ErrorCode.ERR_BadArgRef, "s").WithArguments("1", "out").WithLocation(24, 20),
// (25,19): error CS1620: Argument 1 must be passed with the 'out' keyword
// param3(in s);
Diagnostic(ErrorCode.ERR_BadArgRef, "s").WithArguments("1", "out").WithLocation(25, 19),
// (30,20): error CS1615: Argument 1 may not be passed with the 'out' keyword
// param4(out var l);
Diagnostic(ErrorCode.ERR_BadArgExtraRef, "var l").WithArguments("1", "out").WithLocation(30, 20),
// (32,20): error CS1615: Argument 1 may not be passed with the 'ref' keyword
// param4(ref s);
Diagnostic(ErrorCode.ERR_BadArgExtraRef, "s").WithArguments("1", "ref").WithLocation(32, 20),
// (33,19): error CS1615: Argument 1 may not be passed with the 'in' keyword
// param4(in s);
Diagnostic(ErrorCode.ERR_BadArgExtraRef, "s").WithArguments("1", "in").WithLocation(33, 19)
);
}

[Fact]
public void MismatchedInferredLambdaReturn()
{
var comp = CreateCompilationWithFunctionPointers(@"
unsafe class C
{
public void M(delegate*<System.Func<string>, void> param)
{
param(a => a);
}
}");

comp.VerifyDiagnostics(
// (6,15): error CS1593: Delegate 'Func<string>' does not take 1 arguments
// param(a => a);
Diagnostic(ErrorCode.ERR_BadDelArgCount, "a => a").WithArguments("System.Func<string>", "1").WithLocation(6, 15)
);

var tree = comp.SyntaxTrees[0];
var model = comp.GetSemanticModel(tree);

var lambda = tree.GetRoot().DescendantNodes().OfType<LambdaExpressionSyntax>().Single();

Assert.Equal("a => a", lambda.ToString());

var info = model.GetSymbolInfo(lambda);
var lambdaSymbol = (IMethodSymbol)info.Symbol!;
Assert.NotNull(lambdaSymbol);
Assert.Equal("System.String", lambdaSymbol.ReturnType.ToTestDisplayString(includeNonNullable: false));
Assert.True(lambdaSymbol.Parameters.Single().Type.IsErrorType());
}

[Fact, WorkItem(45418, "https://github.com/dotnet/roslyn/issues/45418")]
public void OutDeconstructionMismatch()
{
var comp = CreateCompilationWithFunctionPointers(@"
unsafe class Test
{
void M1(delegate*<string, void> param1, object o)
{
param1(o is var (a, b));
param1(o is (var c, var d));
}
}");

comp.VerifyDiagnostics(
// (6,25): error CS1061: 'object' does not contain a definition for 'Deconstruct' and no accessible extension method 'Deconstruct' accepting a first argument of type 'object' could be found (are you missing a using directive or an assembly reference?)
// param1(o is var (a, b));
Diagnostic(ErrorCode.ERR_NoSuchMemberOrExtension, "(a, b)").WithArguments("object", "Deconstruct").WithLocation(6, 25),
// (6,25): error CS8129: No suitable 'Deconstruct' instance or extension method was found for type 'object', with 2 out parameters and a void return type.
// param1(o is var (a, b));
Diagnostic(ErrorCode.ERR_MissingDeconstruct, "(a, b)").WithArguments("object", "2").WithLocation(6, 25),
// (7,21): error CS1061: 'object' does not contain a definition for 'Deconstruct' and no accessible extension method 'Deconstruct' accepting a first argument of type 'object' could be found (are you missing a using directive or an assembly reference?)
// param1(o is (var c, var d));
Diagnostic(ErrorCode.ERR_NoSuchMemberOrExtension, "(var c, var d)").WithArguments("object", "Deconstruct").WithLocation(7, 21),
// (7,21): error CS8129: No suitable 'Deconstruct' instance or extension method was found for type 'object', with 2 out parameters and a void return type.
// param1(o is (var c, var d));
Diagnostic(ErrorCode.ERR_MissingDeconstruct, "(var c, var d)").WithArguments("object", "2").WithLocation(7, 21)
);
}

private static readonly Guid s_guid = new Guid("97F4DBD4-F6D1-4FAD-91B3-1001F92068E5");
private static readonly BlobContentId s_contentId = new BlobContentId(s_guid, 0x04030201);

Expand Down