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

Continue visiting bad named argument if not target-typed in NullableWalker #73326

Merged
merged 3 commits into from
May 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 29 additions & 15 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Debug.Assert(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.
Copy link
Contributor

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 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?

Copy link
Member Author

@jjonescz jjonescz May 9, 2024

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.

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)
Expand Down Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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:

// 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)
var previousDisableDiagnostics = _disableDiagnostics;
_disableDiagnostics |= node.HasErrors || defaultArguments[i];

Copy link
Contributor

@RikkiGibson RikkiGibson May 9, 2024

Choose a reason for hiding this comment

The 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));
}
}
}