-
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 2 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 |
---|---|---|
|
@@ -6783,27 +6783,26 @@ private ImmutableArray<VisitResult> VisitArguments( | |
(ParameterSymbol? parameter, TypeWithAnnotations parameterType, FlowAnalysisAnnotations parameterAnnotations, bool isExpandedParamsArgument) = | ||
GetCorrespondingParameter(i, parametersOpt, argsToParamsOpt, expanded, ref paramsIterationType); | ||
|
||
if (// This is known to happen for certain error scenarios, because | ||
// the parameter matching logic above is not as flexible as the one we use in `Binder.BuildArgumentsForErrorRecovery` | ||
// so we may end up with a pending conversion completion for an argument apparently without a corresponding parameter. | ||
parameter is null || | ||
// 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. | ||
node is BoundCall { HasErrors: true, ArgumentNamesOpt.IsDefaultOrEmpty: false, ArgsToParamsOpt.IsDefault: true }) | ||
// This is known to happen for certain error scenarios, because | ||
// the parameter matching logic above is not as flexible as the one we use in `Binder.BuildArgumentsForErrorRecovery` | ||
// so we may end up with a pending conversion completion for an argument apparently without a corresponding parameter. | ||
if (parameter is null) | ||
{ | ||
if (IsTargetTypedExpression(argumentNoConversion) && _targetTypedAnalysisCompletionOpt?.TryGetValue(argumentNoConversion, out var completion) is true) | ||
if (tryShortCircuitTargetTypedExpression(argument, argumentNoConversion)) | ||
{ | ||
// We've done something wrong if we have a target-typed expression and registered an analysis continuation for it | ||
// (we won't be able to complete that continuation) | ||
// 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); | ||
} | ||
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. Choose a reason for hiding this commentThe 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 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. 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. |
||
if (node is BoundCall { HasErrors: true, ArgumentNamesOpt.IsDefaultOrEmpty: false, ArgsToParamsOpt.IsDefault: true } && | ||
tryShortCircuitTargetTypedExpression(argument, argumentNoConversion)) | ||
{ | ||
continue; | ||
} | ||
|
||
// We disable diagnostics when: | ||
// 1. the containing call has errors (to reduce cascading diagnostics) | ||
// 2. on implicit default arguments (since that's really only an issue with the declaration) | ||
|
@@ -6990,6 +6989,21 @@ static void expandParamsCollection(ref ImmutableArray<BoundExpression> arguments | |
} | ||
} | ||
} | ||
|
||
bool tryShortCircuitTargetTypedExpression(BoundExpression argument, BoundExpression argumentNoConversion) | ||
{ | ||
if (IsTargetTypedExpression(argumentNoConversion) && _targetTypedAnalysisCompletionOpt?.TryGetValue(argumentNoConversion, out var completion) is true) | ||
{ | ||
// We've done something wrong if we have a target-typed expression and registered an analysis continuation for it | ||
// (we won't be able to complete that continuation) | ||
// We flush the completion with a plausible/dummy type and remove it. | ||
completion(TypeWithAnnotations.Create(argument.Type)); | ||
TargetTypedAnalysisCompletion.Remove(argumentNoConversion); | ||
return true; | ||
} | ||
|
||
return false; | ||
} | ||
} | ||
|
||
private void ApplyMemberPostConditions(BoundExpression? receiverOpt, MethodSymbol? method) | ||
|
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.
parameter is null
in this code path.