Suppress nullable conversion warnings in default arguments #51631
Conversation
|
@dotnet/roslyn-compiler Please review |
|
@dotnet/roslyn-compiler Please review |
| defaultValue.Syntax, | ||
| defaultValue, | ||
| conversion, | ||
| isCast: conversion.IsExplicit, |
There was a problem hiding this comment.
From our offline discussion: is it possible to observe this change?
There was a problem hiding this comment.
It does not appear to be observable through either public API or through differences in diagnostics.
There was a problem hiding this comment.
Oddly enough I think the IsImplicit property on the IConversionOperation ends up being true if either the node is compiler generated, or if the conversion is not from a cast
There was a problem hiding this comment.
I made a pass over the usages of 'BoundConversion.IsExplicitCastInCode' and I wasn't able to come up with a scenario where behavior for a default argument would change.
| defaultValue, | ||
| conversion, | ||
| isCast: conversion.IsExplicit, | ||
| new ConversionGroup(conversion, parameter.TypeWithAnnotations), |
There was a problem hiding this comment.
From our offline discussion: consider avoiding this allocation when isCast is false
Closes #51622
The issue is basically that the binder creates a bound node for the argument with a conversion whose "isCast" is false. It seems like the nullable walker will try to recreate such conversions with nullable types re-inferred to try and decide if they're valid. We try to re-classify the conversion with the assumption that the conversion must be implicit, since there seemingly is no cast.
In effect,
MyEnum? e = MyEnum.Valuecurrently works properly whenMyEnum.Valuehas constant value 0, but but not when the constant value is non-zero. This is because the default argument that gets inserted is of the underlying type converted to the parameter type--MyEnum? e = 0is allowed, butMyEnum? e = 1is not. You have to useMyEnum? e = (MyEnum?)1It's also not enough to just always set isCast: true--this ends up breaking certain tests where flow analysis depends on the values of default arguments due to attributes. So I decided to set it based on whether the default argument conversion that we found is an explicit conversion.
Ultimately, nullable diagnostics are never supposed to be reported on implicit default arguments, including the conversions of such arguments, and that's the only change in this PR that's strictly needed to resolve this bug. However, it also seems reasonable to adjust the creation of default arguments for consistency in the public API.