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

Fix type parameter scoping for local functions #59968

Closed
wants to merge 3 commits into from
Closed

Fix type parameter scoping for local functions #59968

wants to merge 3 commits into from

Conversation

Youssef1313
Copy link
Member

Fixes #59775

@RikkiGibson Tagging you since you were assigned the issue.

Previously we were using WithMethodTypeParametersBinder for binding, including local function attributes. Now I'm using _withTypeParamsBinder when needed, ie, for everything except attributes (which uses SignatureBinder).

Does this look correct?

@Youssef1313 Youssef1313 requested a review from a team as a code owner March 5, 2022 18:36
@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Mar 5, 2022
@Youssef1313
Copy link
Member Author

Youssef1313 commented Mar 5, 2022 •
edited
Loading

@RikkiGibson Can you look at TestLocalFunctionParameterAndTypeParameterScope? The test was written for #16801 and matches TestMethodParameterAndTypeParameterScope. For TestMethodParameterAndTypeParameterScope, I don't understand why LookupPosition doesn't match the regular scoping.

#Resolved (hopefully 😄 )

Youssef1313
Youssef1313 commented Mar 5, 2022
View reviewed changes
src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs
@@ -358,7 +358,7 @@ private static Binder GetEnclosingBinderInternalWithinRoot(SyntaxNode node, int
LocalFunctionSymbol function = GetDeclaredLocalFunction(binder, ownerOfTypeParametersInScope.Identifier);
if ((object)function != null)
{
binder = function.SignatureBinder;
binder = function.ParameterBinder;
Copy link
Member Author

Choose a reason for hiding this comment

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

SignatureBinder is used for attribute binding, hence, it doesn't have type parameters in scope. This code path used by LookupPosition wants to have type parameters in scope, so I switched to using ParameterBinder here.

@RikkiGibson
Copy link
Contributor

@jcouv I was hoping this would be fixed as part of 'nameof(param)'--and that we would allow it, i.e. 'nameof(T)' within an attribute. Is that the plan?

@jcouv
Copy link
Member

jcouv commented Mar 8, 2022

@RikkiGibson Whether to allow nameof(T) as part of the nameof(parameter) feature is an open issue. The spec initially included it, but I'm still waiting for a motivating scenario. Feel free to add in dotnet/csharplang#5829

@Youssef1313
Copy link
Member Author

@jcouv @RikkiGibson Is there a conclusion yet on whether this would be allowed or not?

@RikkiGibson
Copy link
Contributor

RikkiGibson commented Mar 10, 2022 •
edited
Loading

The internal thread concluded that both type parameter names and parameter names should be available in 'nameof' on attributes. Therefore, I think we shouldn't take the change here, and should just correct the behavior as part of the language feature implementation--possibly adding a LangVersion diagnostic for referencing type parameter names in local functions pre-C# 11, for example.

@Youssef1313
Copy link
Member Author

That would be for regular methods too? It's currently not allowed.

@RikkiGibson
Copy link
Contributor

Yes, that's another place we would add a LangVersion diagnostic.

@Youssef1313
Copy link
Member Author

Great. I'll close this PR and see if I can get another PR for the lang version checks and regular methods.

@Youssef1313 Youssef1313 deleted the 59775-typeparameter-scoping branch March 10, 2022 18:28
@Youssef1313 Youssef1313 mentioned this pull request Mar 10, 2022
Closed
@jcouv
Copy link
Member

jcouv commented Mar 10, 2022

@Youssef1313 I would not close this PR. What Rikki meant is that in C# 11, we'll end up allowing this as part of nameof(parameter) feature. This fix is still needed to establish correct C# 10 baseline (ie. should be disallowed now).

@jcouv
Copy link
Member

jcouv commented Mar 10, 2022

Let me chat with Rikki. I see his point.

@jcouv
Copy link
Member

jcouv commented Mar 10, 2022

From chat with @RikkiGibson we still think this PR is useful to establish correct C# 10 baseline. Let's re-open please

@jcouv
Copy link
Member

jcouv commented Mar 10, 2022

I'll just integrate this close PR into my WIP PR for C# 11: #40525. That'll reduce need to coordinate around this issue. Thanks

@jcouv jcouv mentioned this pull request Mar 11, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unexpected scoping on type parameter of local function
3 participants
@Youssef1313 @RikkiGibson @jcouv