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

Implement lower and upper bound inference in function pointer parameters #50249

Merged
merged 12 commits into from
Jan 28, 2021
Merged

Implement lower and upper bound inference in function pointer parameters #50249

merged 12 commits into from
Jan 28, 2021

Conversation

333fred
Copy link
Member

@333fred 333fred commented Jan 5, 2021 •
edited
Loading

#43041 implemented exact inference of function pointer parameters and bounds only, and left a few errors in our tests that needed to be handled. This is also the cause of #50096. This implements bounds inference, using the same variance rules that function pointer to function pointer conversions do. Note that for parameters and return types, I'm not considering delegate*->void* conversions, as T cannot be either void or void*.

Spec PR: dotnet/csharplang#4310

Fixes #50096.

@333fred
Implement lower and upper bound inference in function pointer parameters
d2cfaf7 
dotnet#43041 implemented exact inference of function pointer parameters and bounds only, and left a few errors in our tests that needed to be handled. This is also the cause of dotnet#50096. This implements bounds inference, using the same variance rules that function pointer to function pointer conversions do. I still need to spec the rules here, and may need to make a couple of adjustments around handling of delegate*->void* in bounds inference. At least initially, this isn't supported, but I'll work out what the correct rules should be when I spec it and update this PR accordingly.

Fixes dotnet#50096.
333fred
333fred commented Jan 5, 2021
View reviewed changes
src/Compilers/CSharp/Test/Semantic/Semantics/FunctionPointerTests.cs
@@ -1563,7 +1667,6 @@ public static void M()
}
}");

// Some of these errors should become CS0306 after variant conversions are implemented, tracked by https://github.com/dotnet/roslyn/issues/39865
Copy link
Member Author

Choose a reason for hiding this comment

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

None of the remaining 0411 errors can be inferred: they're between function pointers with int and string parameters. There is no reference conversion or implicit pointer conversion, so nothing can be inferred.

333fred
333fred commented Jan 5, 2021
View reviewed changes
src/Compilers/CSharp/Test/Semantic/Semantics/FunctionPointerTests.cs
@@ -1651,7 +1754,6 @@ public static void M()
}
}");

// These should all work: https://github.com/dotnet/roslyn/issues/39865
Copy link
Member Author

Choose a reason for hiding this comment

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

These could only be inferred if we supported array covariance for implicit pointer conversions. The spec makes no mention of such support, and we do not support int*[] being converted to void*[] today, so this is not expected to be supported.

@333fred 333fred mentioned this pull request Jan 7, 2021
Merged
@333fred
Add addtional testing for pointer conversions. T cannot appear vari…
80ce5f8 
…antly inside a pointer, and any inferences on such `T`s should be done with exact inference.
@333fred
Skip unmanaged pointer peverify on framework.
d8afd5a
@333fred 333fred marked this pull request as ready for review January 7, 2021 19:20
@333fred 333fred requested a review from a team as a code owner January 7, 2021 19:21
@333fred
Copy link
Member Author

333fred commented Jan 7, 2021

@dotnet/roslyn-compiler for review.

@333fred 333fred added the Feature - Function Pointers Adding Function Pointers label Jan 7, 2021
@333fred
Copy link
Member Author

333fred commented Jan 8, 2021

@dotnet/roslyn-compiler for review.

@333fred
PR feedback
f656b06 
* Missed parameter rename.
* Ensure error condition is explicity rejected.
@333fred
Copy link
Member Author

333fred commented Jan 15, 2021 •
edited
Loading

@AlekseyTs I believe I've addressed your feedback. I didn't see a review complete comment, so for reference your comments were on commit 3. #Closed

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jan 15, 2021 •
edited
Loading

@333fred It looks like there are some legitimate test failures. #Closed

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jan 15, 2021 •
edited
Loading

Done with review pass (commit 4), tests are not looked at. #Closed

@333fred
Copy link
Member Author

333fred commented Jan 15, 2021

@AlekseyTs addressed feedback.

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

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jan 16, 2021 •
edited by 333fred
Loading

addressed feedback.

@333fred It looks like there are some legitimate test failures. It looks like that was a web page refresh issue. #Closed

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jan 16, 2021 •
edited
Loading

Done with review pass (commit 6) #Closed

This was referenced Jan 18, 2021
Closed
Closed
@jcouv jcouv self-assigned this Jan 21, 2021
@333fred
Copy link
Member Author

333fred commented Jan 22, 2021

@AlekseyTs addressed feedback.

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

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jan 23, 2021 •
edited
Loading

Done with review pass (commit 7) #Closed

@333fred
Copy link
Member Author

333fred commented Jan 25, 2021

@AlekseyTs addressed feedback.

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

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jan 25, 2021 •
edited
Loading

Done with review pass (commit 9) #Closed

@333fred
Copy link
Member Author

333fred commented Jan 25, 2021

@AlekseyTs addressed feedback.

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

AlekseyTs
AlekseyTs approved these changes Jan 25, 2021
View reviewed changes
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 12), assuming CI is passing.

jcouv
jcouv reviewed Jan 28, 2021
View reviewed changes
src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/MethodTypeInference.cs
@@ -1281,60 +1291,79 @@ private bool MethodGroupReturnTypeInference(Binder binder, BoundExpression sourc
// SPEC: yields a single method with return type U then a lower-bound
// SPEC: inference is made from U to Tb.
Copy link
Member

Choose a reason for hiding this comment

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

nit: Consider updating the spec comments to reflect changes.

jcouv
jcouv reviewed Jan 28, 2021
View reviewed changes
src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/MethodTypeInference.cs
@@ -2067,6 +2132,54 @@ private void LowerBoundTypeArgumentInference(NamedTypeSymbol source, NamedTypeSy
targetTypeArguments.Free();
}

#nullable enable
private bool LowerBoundFunctionPointerTypeInference(TypeSymbol source, TypeSymbol target, ref HashSet<DiagnosticInfo> useSiteDiagnostics)
Copy link
Member

Choose a reason for hiding this comment

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

LowerBoundFunctionPointerTypeInference [](start = 21, length = 38)

Adding new spec comments would help

jcouv
jcouv approved these changes Jan 28, 2021
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 12)

@333fred 333fred changed the base branch from master to release/dev16.9 January 28, 2021 19:45
@333fred
Copy link
Member Author

333fred commented Jan 28, 2021

@jcouv I'm going to follow up on your comment suggestions in master to get this in now.

@333fred 333fred merged commit f67e419 into dotnet:release/dev16.9 Jan 28, 2021
@333fred 333fred deleted the function-pointer-bounds branch January 28, 2021 21:29
@333fred 333fred mentioned this pull request Jan 29, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlekseyTs AlekseyTs AlekseyTs approved these changes

@jcouv jcouv jcouv approved these changes

Assignees

@jcouv jcouv

Labels
Area-Compilers Feature - Function Pointers Adding Function Pointers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The type parameters aren't correctly inferred on function pointers
4 participants
@333fred @AlekseyTs @jcouv @Dotnet-GitSync-Bot