-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Implement lambda discard parameters #38786
Conversation
src/Compilers/CSharp/Portable/Binder/Binder.QueryUnboundLambdaState.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Binder/WithLambdaParametersBinder.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Symbols/Source/SourceSimpleParameterSymbol.cs
Outdated
Show resolved
Hide resolved
src/EditorFeatures/CSharpTest/Classification/SyntacticClassifierTests_Preprocessor.cs
Show resolved
Hide resolved
src/EditorFeatures/CSharpTest/QuickInfo/SemanticQuickInfoSourceTests.cs
Outdated
Show resolved
Hide resolved
FYI @mavasani, as this is could require some updates to unused parameter analysis. #Resolved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done review pass (commit 5)
@333fred Please take another look. Thanks #Closed |
src/Compilers/CSharp/Test/Semantic/Semantics/LambdaDiscardParametersTests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (commit 9), but we need LDM to follow up on the out discard question.
Ping @gafter @dotnet/roslyn-compiler for a second review. Thanks #Closed |
src/Compilers/CSharp/Portable/Symbols/Wrapped/WrappedParameterSymbol.cs
Outdated
Show resolved
Hide resolved
I don't see any tests for the anonymous_method_expression, e.g. Func<int, int, int> f = delegate (int _, int _) { return 1; };
``` #Resolved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks nice and clean. One small bug in WrappedParameterSymbol
and also needs testing (possibly also implementation work) for delegate
expressions.
It's covered in DiscardParameters_InDelegates. Maybe I misnamed the test though? In reply to: 538120141 [](ancestors = 538120141) |
@gafter I addressed your feedback. Thanks #Closed |
Pending decisions on the newly added tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compiler changes LGTM (iteration 20)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (commit 21), but there are test failures.
@333fred Thanks. It's a merge conflict. I'll go ahead and fix #Resolved |
@dotnet/roslyn-ide for review. This PR includes a small tweak to displaying discards in QuickInfo, and tests for new discard scenarios. Thanks #Resolved |
{ | ||
System.Func<short, short, long> f1 = (_, a) => | ||
{ | ||
int _ = 0; // 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably good idea to test for no regression on C# 8.0 shadowing with underscore?
Action<int> f1 = (_) =>
{
_.ToString(); // ok
Action<int> g2 = (_) => _.ToString(); // ok
};
Action<int, int> f2 = (_, _) =>
{
_.ToString(); // error
Action<int> g2 = (_) => _.ToString(); // ok
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added DiscardParameters_Shadowing
@dotnet/roslyn-ide for review. This PR includes a small tweak to displaying discards in QuickInfo, and tests for new discard scenarios. Thanks #Resolved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks fine so far, although I defer to @dpoeschl about the Change Signature tests.
Do we need some additional tests around rename? I can't decide if the behavior should just be you can't rename a discard, or you can and it's just renaming the use which isn't terribly useful but whatever.
(Holding signoff until we add a test on that, whichever way the test goes.)
src/VisualStudio/Core/Test/ChangeSignature/ChangeSignatureViewModelTests.vb
Outdated
Show resolved
Hide resolved
Do we also need to do anything special in completion for these? Would _ appear as an entry there if these are in scope? |
I imagine it would, but it should also do that today so I don't think there's anything special to change here. |
Discards do not introduce any names in any scopes. Added some completion tests to illustrate. This is similar to an existing scenario: void M()
{
var (_, x) = (1, 2);
{
int i = 1 + $$ // no symbol named `_` appears in completion at this position
}
} |
Thanks for adding the completion tests. I could easily imagine LookupSymbols accidentally returning the "name" of this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signing off, with the caveat that @dpoeschl still knows the change signature stuff better than I.
I added a test for InlineRename. I'll go ahead and merge once CI is green. Thanks |
void M() | ||
{ | ||
C _ = null; | ||
System.Func<int, string, int> f = (int _, string [|$$_|]) => { _ = null; return 1; }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasonmalinowski @dpoeschl I'm not sure if this is the correct way to test this, but when I test manually, I see that Rename isn't offered on discards, which is what I'd expect (GetDeclaredSymbol
return null
for discards, so renaming wouldn't be possible as it's not even clear what symbol would be renamed).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think blocking is a completely reasonable decision: I didn't want us marching on with a symbol that had no name or a strange name and then crashing. That said if it's blocking we need to put a test at a different layer, but that's not a problem. We'll clean this one up.
Didn't we identify the need for a specification in the test plan review? I would think that is a prerequisite to a complete review of the test plan (because we need to know what we're testing) and of the implementation (because we need to know what is implemented). |
In the new language version, when more than one parameter is an underscore, we allow it and bind them as discard parameter symbols (which are like regular parameters, but doesn't add any name in any scope).
dotnet/csharplang#111
Test plan: #38820