Continue visiting bad named argument if not target-typed in NullableWalker #73326
Conversation
jjonescz
added
the
New Language Feature - Nullable Reference Types
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
jjonescz
changed the title
jjonescz
changed the title
| // We flush the completion with a plausible/dummy type and remove it. | ||
| completion(TypeWithAnnotations.Create(argument.Type)); | ||
| TargetTypedAnalysisCompletion.Remove(argumentNoConversion); | ||
|
|
||
| Debug.Assert(parameter is not null || method is ErrorMethodSymbol); |
|
@RikkiGibson @dotnet/roslyn-compiler for a second review, thanks |
| public async Task M2() => await Task.Yield(); | ||
| } | ||
| """; | ||
| CreateCompilation(source).VerifyDiagnostics( |
There was a problem hiding this comment.
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.
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.
sgtm! (no action needed in that case.)
| } | ||
| continue; | ||
| } | ||
|
|
||
| // In error recovery with named arguments, target-typing cannot work as we can get a different parameter type | ||
| // from our GetCorrespondingParameter logic than Binder.BuildArgumentsForErrorRecovery does. |
There was a problem hiding this comment.
These comments make it seem like we are working around an inconsistency between GetCorrespondingParameter and BuildArgumentsForErrorRecovery. 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.
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.
This is a follow up on #72635 - slight refactoring to only perform the short-circuit if we have a target-typed handler registered, not always. Avoids skipping visiting bad nodes in scenarios that can work. Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2045970.