-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Continue visiting bad named argument if not target-typed in NullableWalker #73326
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -158692,5 +158692,45 @@ static void M<T>() where T : struct, I | |||||||||||
// y.Value.Item.ToString(); | ||||||||||||
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "y.Value.Item").WithLocation(15, 9)); | ||||||||||||
} | ||||||||||||
|
||||||||||||
[Fact, WorkItem("https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2045970")] | ||||||||||||
public void BadAsyncLambdaInNamedArgument() | ||||||||||||
{ | ||||||||||||
var source = """ | ||||||||||||
#nullable enable | ||||||||||||
using System; | ||||||||||||
using System.Threading.Tasks; | ||||||||||||
|
||||||||||||
class D | ||||||||||||
{ | ||||||||||||
void M() | ||||||||||||
{ | ||||||||||||
var c1 = new C(); | ||||||||||||
var c2 = new C(); | ||||||||||||
C.M1(f: async () => | ||||||||||||
{ | ||||||||||||
if (c2 != null) | ||||||||||||
{ | ||||||||||||
c1. | ||||||||||||
await c2.M2(); | ||||||||||||
} | ||||||||||||
}); | ||||||||||||
} | ||||||||||||
} | ||||||||||||
|
||||||||||||
class C | ||||||||||||
{ | ||||||||||||
public static void M1(Func<Task> f) { } | ||||||||||||
public async Task M2() => await Task.Yield(); | ||||||||||||
} | ||||||||||||
"""; | ||||||||||||
CreateCompilation(source).VerifyDiagnostics( | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: consider adjusting the scenario so that a nullable diagnostic is expected or something like that, to show that nullable analysis is definitely occurring here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you mean inside the lambda, no diagnostics will be reported there since it's a bad call and hence diagnostics are disabled inside: roslyn/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs Lines 6743 to 6747 in c23414e
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sgtm! (no action needed in that case.) |
||||||||||||
// (15,20): error CS1001: Identifier expected | ||||||||||||
// c1. | ||||||||||||
Diagnostic(ErrorCode.ERR_IdentifierExpected, "").WithLocation(15, 20), | ||||||||||||
// (15,20): error CS1002: ; expected | ||||||||||||
// c1. | ||||||||||||
Diagnostic(ErrorCode.ERR_SemicolonExpected, "").WithLocation(15, 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.
These comments make it seem like we are working around an inconsistency between
GetCorrespondingParameter
andBuildArgumentsForErrorRecovery
. Should we make some change to ensure that these methods behave consistently? Would that remove the need for some of the complexity in here?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.
That's a good point, I think we could try to do that. Probably as a follow up (if it didn't work out, this would still be a valid fix)?
EDIT: Looking into this more, it doesn't seem very easy to make GetCorrespondingParameter consistent with BuildArgumentsForErrorRecovery - the latter does lots of heuristics and works against all overloads unlike the former. In general, I'm not sure it would be worth the work given this is just about error scenarios.