Fix analysis of parameter default values#49009
Conversation
| ImmutableArray<LocalSymbol>.Empty, | ||
| // note that if the parameter type conflicts with the default value from attributes, | ||
| // we will just get a bad constant value above and return early. | ||
| new BoundLiteral(parameterSyntax, DefaultValueFromAttributes, Type)); |
There was a problem hiding this comment.
DefaultValueFromAttributes [](start = 50, length = 26)
defaultValue
|
Oh, we introduced new warnings in Roslyn :)
|
|
@dotnet/roslyn-compiler Please review |
|
@dotnet/roslyn-compiler Please review |
|
@dotnet/roslyn-compiler Please review |
| { | ||
| if (binder is not null && parameterEqualsValue is not null && !_lazyDefaultSyntaxValue.IsBad) | ||
| { | ||
| NullableWalker.AnalyzeIfNeeded(binder, parameterEqualsValue, diagnostics); |
There was a problem hiding this comment.
Why are we analyzing here and below?
There was a problem hiding this comment.
In this implementation, _lazyDefaultSyntaxValue must be initialized on parameters before performing nullable analysis to avoid a stack overflow.
In my subsequent PR #49186 I modify DefaultSyntaxValue to return ConstantValue.Unset if we have not finished initializing the default value, for example when binding the default argument to F() in void F(object param = F()) { }.
Given that, we could clean this up to move analysis of the syntax value to within MakeDefaultExpression and move analysis of the attribute value to perhaps GetAttributesBag on this symbol in a follow-up.
|
|
||
| private void NullableAnalyzeParameterDefaultValueFromAttributes(DiagnosticBag diagnostics) | ||
| { | ||
| var defaultValue = DefaultValueFromAttributes; |
There was a problem hiding this comment.
Consider skipping all this work if we don't need to do nullable analysis.
There was a problem hiding this comment.
Happy to do this in the follow-up at the same time as moving this check to a more appropriate spot.
| // we will just get a bad constant value above and return early. | ||
| new BoundLiteral(parameterSyntax, defaultValue, Type)); | ||
|
|
||
| NullableWalker.AnalyzeIfNeeded(binder, parameterEqualsValue, diagnostics); |
There was a problem hiding this comment.
How is this information getting to the semantic model? IE, hovering over the default keyword for a NRT should have a nullable state of MaybeNull, are there tests for this?
There was a problem hiding this comment.
I actually have no idea, but I will try to add some tests that I think cover this gap for this as well as for attributes which use a similar approach for nullable analysis.
| Diagnostic(ErrorCode.WRN_DefaultValueForUnconsumedLocation, "x").WithArguments("x").WithLocation(6, 34), | ||
| // (6,38): warning CS8625: Cannot convert null literal to non-nullable reference type. | ||
| // static partial void G(object x = null, object? y = null) { } | ||
| Diagnostic(ErrorCode.WRN_NullAsNonNullable, "null").WithLocation(6, 38), |
There was a problem hiding this comment.
I assume you have a test insertion, since we're adding new warnings?
There was a problem hiding this comment.
I kicked it off now, thanks. I'll link the insertion once it gets created.
There was a problem hiding this comment.
https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/286574
Looks like there are 8 new nullable warnings. Some require just adding ? to a CallerMemberName parameter. The rest probably require adding a ! e.g. M<T>(T defaultValue = default!), because the project targets C# 8 and nullability attributes don't appear to be available.
Closes #48844
Closes #48847
Closes #48848
(note that these are really just 3 test cases, and expected behavior stated in the issues is not met by this PR. We changed the expectation as described in nullable-parameter-default-value-analysis.md)
Related to #47352
Summary: parameter default values no longer affect initial state. A parameter default value which is incompatible with the parameter type is always a safety warning.