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

Binder Check for Unbound Generics in Methods #45033

Merged
merged 10 commits into from
Jun 23, 2020
Merged

Binder Check for Unbound Generics in Methods #45033

merged 10 commits into from
Jun 23, 2020

Conversation

kevinsun-dev
Copy link
Contributor

@kevinsun-dev kevinsun-dev commented Jun 10, 2020 •
edited
Loading

Based off @RikkiGibson's #43130, a patch for issue #41779.
Thanks to @jpd30 for reporting the issue!

This patch checks for unbound types in the Binder, throwing a CS7003 error without needing to run EmitDiagnostics.

Changelog:

Sidenote: Apologies for the messy git changes to some of those tests. The newer outputs from the testing suite contained more information in comments that I thought would be worthwhile to leave in.

@kevinsun-dev kevinsun-dev self-assigned this Jun 10, 2020
@RikkiGibson
Copy link
Contributor

RikkiGibson commented Jun 10, 2020 •
edited by jcouv
Loading

Looks like we fail an assert in DefiniteAssignmentPass. This should be reproducible locally by running the Microsoft.CodeAnalysis.CSharp.Symbol tests in Debug configuration. #Resolved

@kevinsun-dev kevinsun-dev marked this pull request as ready for review June 11, 2020 16:07
@kevinsun-dev kevinsun-dev requested a review from a team as a code owner June 11, 2020 16:07
@333fred
Copy link
Member

333fred commented Jun 12, 2020 •
edited
Loading

Done review pass (commit 3). This doesn't seem like the right fix to me: It markedly degrades error experiences with unbound generics in some cases and doesn't hugely improve the scenario in the bug. I think we need to use a better error here. #Closed

@kevinsun-dev
Copy link
Contributor Author

kevinsun-dev commented Jun 16, 2020 •
edited
Loading

Done review pass (commit 3). This doesn't seem like the right fix to me: It markedly degrades error experiences with unbound generics in some cases and doesn't hugely improve the scenario in the bug. I think we need to use a better error here.

Edited the check to throw allow binding errors to occur, but adds a diagnostic to catch the new case. All instances where information density was reduced have been corrected, with the only issue being that there's occasionally one more descriptive error in the mix. #Resolved

333fred
333fred reviewed Jun 16, 2020
View reviewed changes
src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs Outdated
if (right is GenericNameSyntax genericNameRight && rightHasTypeArguments && typeArgumentsSyntax.Any(SyntaxKind.OmittedTypeArgument) && !IsUnboundTypeAllowed(genericNameRight))
{
diagnostics.Add(ErrorCode.ERR_UnexpectedUnboundGenericName, genericNameRight.Location);
}
Copy link
Member

@333fred 333fred Jun 16, 2020 •
edited by kevinsun-dev
Loading

Choose a reason for hiding this comment

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

Nit: newline #Resolved

Copy link
Contributor Author

@kevinsun-dev kevinsun-dev Jun 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Broke the line at a reasonable point to improve readability. #Resolved

333fred
333fred reviewed Jun 16, 2020
View reviewed changes
src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs Outdated
@@ -6062,6 +6062,11 @@ private bool IsUsingAliasInScope(string name)
}
default:
{
// Ensure that there is an error thrown when an Unbound Generic is found.
if (right is GenericNameSyntax genericNameRight && rightHasTypeArguments && typeArgumentsSyntax.Any(SyntaxKind.OmittedTypeArgument) && !IsUnboundTypeAllowed(genericNameRight))
Copy link
Member

@333fred 333fred Jun 16, 2020 •
edited by kevinsun-dev
Loading

Choose a reason for hiding this comment

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

Do we need to do this for every default case, or just the cases that return from here? #Resolved

Copy link
Contributor Author

@kevinsun-dev kevinsun-dev Jun 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

You're right, only the ones that return from here need to be affected. Moved to a more specific scope. #Resolved

@333fred
Copy link
Member

333fred commented Jun 16, 2020 •
edited by kevinsun-dev
Loading

Done review pass (commit 4) #Resolved

333fred
333fred approved these changes Jun 16, 2020
View reviewed changes
Copy link
Member

@333fred 333fred 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 5), assuming tests pass.

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jun 16, 2020 •
edited
Loading

Done with review pass (iteration 5) #Closed

@kevinsun-dev
Copy link
Contributor Author

kevinsun-dev commented Jun 18, 2020 •
edited
Loading

Moved check code to BindInstanceMemberAccess, the last part of the stack that has error checks when binding method groups. Integrated it into existing BadArity check code to report more accurate results. Added new test cases for variants of the original error case. #Closed

AlekseyTs
AlekseyTs reviewed Jun 18, 2020
View reviewed changes
src/Compilers/CSharp/Test/Symbol/Symbols/GenericConstraintTests.cs

static void M(I provider)
{
provider.GetService<>();
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

provider.GetService<>(); [](start = 8, length = 24)

This is not an invocation of a static method. A static method is qualified with a type name rather than with an instance. #Closed

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jun 18, 2020 •
edited
Loading

Done with review pass (iteration 6) #Closed

@333fred 333fred self-requested a review June 18, 2020 21:01
Quick PR Fixes - Part 2 of 2
9b7bcce 
Added new error, broadened error throwing to include all instances of OmittedTypeArgument in method binding.
@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jun 21, 2020 •
edited
Loading

Done with review pass (iteration 8). Implementation looks good to me. Consider adding recommended tests. #Closed

@kevinsun-dev
Copy link
Contributor Author

Still getting used to CodeFlow, couldn't seem to find a way to drop a comment as the PR Author, gonna ask about that.
Expanded testing to include binding to generics and non-generics for instance, extension, and static methods, as well as testing methods requiring no generic types, one generic type, and two generic types for all cases. Added translated test matrix to VB as well.

333fred
333fred reviewed Jun 22, 2020
View reviewed changes
src/Compilers/VisualBasic/Test/Symbol/SymbolsTests/UnboundGenericType.vb
~
</expected>)

End Sub
Copy link
Member

Choose a reason for hiding this comment

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

Nit: extra newline below.

333fred
333fred approved these changes Jun 22, 2020
View reviewed changes
Copy link
Member

@333fred 333fred 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 10)

AlekseyTs
AlekseyTs approved these changes Jun 23, 2020
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 (iteration 10)

@kevinsun-dev kevinsun-dev merged commit 35b67f8 into dotnet:master Jun 23, 2020
@ghost ghost added this to the Next milestone Jun 23, 2020
@kevinsun-dev kevinsun-dev mentioned this pull request Jun 24, 2020
Closed
@dibarbet dibarbet modified the milestones: Next, 16.7.P4 Jun 30, 2020
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

@333fred 333fred 333fred approved these changes

Assignees

@kevinsun-dev kevinsun-dev

Labels
Area-Compilers Bug
Projects
None yet
Milestone
16.7.P4
Development

Successfully merging this pull request may close these issues.
5 participants
@kevinsun-dev @RikkiGibson @333fred @AlekseyTs @dibarbet