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

Add parameters in scope in nameof in certain attributes #60642

Merged
merged 7 commits into from
Apr 15, 2022

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Apr 8, 2022

Made a corresponding change to speclet: dotnet/csharplang#5995
Test plan #40524

@jcouv jcouv self-assigned this Apr 8, 2022
@@ -49,14 +51,42 @@ private bool IsNameofOperator

internal override void LookupSymbolsInSingleBinder(LookupResult result, string name, int arity, ConsList<TypeSymbol> basesBeingResolved, LookupOptions options, Binder originalBinder, bool diagnose, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
{
bool foundParameter = false;
Copy link
Member Author

@jcouv jcouv Apr 8, 2022

Choose a reason for hiding this comment

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

📝 See LookupSymbolsInternal for context on this change #Resolved

@jcouv jcouv marked this pull request as ready for review April 8, 2022 16:22
@jcouv jcouv requested a review from a team as a code owner April 8, 2022 16:22
@jcouv
Copy link
Member Author

jcouv commented Apr 8, 2022

@AlekseyTs @RikkiGibson @dotnet/roslyn-compiler for review. Thanks

@RikkiGibson RikkiGibson self-assigned this Apr 8, 2022
@jcouv
Copy link
Member Author

jcouv commented Apr 11, 2022

@AlekseyTs @RikkiGibson for review. Thanks
This should be the last PR for nameof scoping changes.

@jcouv jcouv requested review from AlekseyTs and RikkiGibson April 11, 2022 23:20
@jcouv
Copy link
Member Author

jcouv commented Apr 12, 2022

Updated following decision from email thread. Thanks @RikkiGibson

comp = CreateCompilation(source);
comp.VerifyDiagnostics();

VerifyParameter(comp, 0, "System.Int32 C.this[System.Int32 parameter].get");
Copy link
Member Author

@jcouv jcouv Apr 12, 2022

Choose a reason for hiding this comment

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

📝 I don't have a strong opinion on whether the indexer parameter or the accessor parameter should be found. I went with accessor parameter because that seems more consistent with methods and other targets. #Resolved

@jcouv
Copy link
Member Author

jcouv commented Apr 12, 2022

@AlekseyTs @RikkiGibson This is ready for another look. Thanks

@AlekseyTs
Copy link
Contributor

@RikkiGibson I'll let you take the first pass at the latest changes.

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 5)

@RikkiGibson
Copy link
Contributor

@RikkiGibson I'll let you take the first pass at the latest changes.

Sorry, it looks like I missed the comment this morning. 😓

";
comp = CreateCompilation(source, parseOptions: TestOptions.RegularNext);
comp.VerifyDiagnostics(
// (2,49): warning CS8981: The type name 'parameter' only contains lower-cased ascii characters. Such names may become reserved for the language.
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 13, 2022

Choose a reason for hiding this comment

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

warning CS8981

Consider suppressing this warning (#pragma, a name change) in order to reduce the unnecessary noise. #Resolved

comp.VerifyDiagnostics(
// (8,20): warning CS8981: The type name 'parameter' only contains lower-cased ascii characters. Such names may become reserved for the language.
// void local<parameter>([My(nameof(parameter))] int parameter) => throw null;
Diagnostic(ErrorCode.WRN_LowerCaseTypeName, "parameter").WithArguments("parameter").WithLocation(8, 20),
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 13, 2022

Choose a reason for hiding this comment

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

WRN_LowerCaseTypeName

Same comment for this warning #Resolved

VerifyTParameter(comp, 0, "void local<TParameter>(System.Int32 TParameter)");
VerifyTParameter(comp, 1, "void C.M2<TParameter>(System.Int32 TParameter)");

source = @"
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 13, 2022

Choose a reason for hiding this comment

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

source

Is it necessary to change the value? I think it would be clearer to observe behavior change for the same source #Resolved


VerifyTParameter(comp, 0, "MyDelegate<TParameter>");

source = @"
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 13, 2022

Choose a reason for hiding this comment

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

source = @"

Is it necessary to change the value? I think it would be clearer to observe behavior change for the same source #Resolved

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 think it is. There's some differences between VerifyParameter and VerifyTParameter, for instance around SymbolKind check and LookupSymbols expectations.

Copy link
Member Author

Choose a reason for hiding this comment

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

Correction: I can share the same source with a minor tweak to VerifyParameter to configure the name of the parameter.

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 6)

@jcouv jcouv merged commit 1270eeb into dotnet:features/nameof-parameter Apr 15, 2022
@jcouv jcouv deleted the nameof-scope2 branch April 15, 2022 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants