Fix param-nullchecking build/test failures #57375
Conversation
ghost
added
the
| // void M0(string name !!=null) { } | ||
| Diagnostic(ErrorCode.WRN_NullCheckedHasDefaultNull, "name").WithArguments("name").WithLocation(5, 20), | ||
| // (6,20): warning CS8993: Parameter 'name' is null-checked but is null by default. | ||
| // void M1(string name! !=null) { } |
There was a problem hiding this comment.
I'm surprised that we're issuing a warning for this. Presumably we're error recovering the !! so now emitting the warning too, but it seems... odd to me.
There was a problem hiding this comment.
Yeah, I didn't look extremely deeply into why--may relate to exactly where I moved the calls ParameterHelpers.AddNullCheckErrorsToParameter. Basically if expediency makes these warnings go away, or makes them stick around in the presence of syntax errors, I'm fine either way.
src/Compilers/CSharp/Portable/Symbols/Source/ParameterHelpers.cs
Outdated
Show resolved
Hide resolved
| { | ||
| [CompilerTrait(CompilerFeature.IOperation)] | ||
| public class IOperationTests : SemanticModelTestBase | ||
| public partial class IOperationTests : SemanticModelTestBase |
There was a problem hiding this comment.
It looks like the IOperation tests added in the feature were not updated to match the new convention. I was able to remove this partial modifier after making the new tests use a conventional name.
| { | ||
| var nullableHasValue = ((IMethodSymbol)_compilation.CommonGetSpecialTypeMember(SpecialMember.System_Nullable_T_get_HasValue))?.Construct(parameter.Type); | ||
| if (nullableHasValue is null) | ||
| // PROTOTYPE(param-nullchecking): is there a better way to get the HasValue symbol here? |
There was a problem hiding this comment.
We can't construct the HasValue member, we have to construct Nullable<T> and then somehow get HasValue off of it. Normally in the compiler we would construct the Nullable<{parameter.Type}>, get the original definition of Nullable<T>.HasValue, then use the AsMemberOfType helper to get the construction of HasValue within the constructed Nullable<{parameter.Type}>.
There was a problem hiding this comment.
I was actually a bit surprised by the original code. It seems like it probably never worked.
| namespace Microsoft.CodeAnalysis.CSharp.UnitTests | ||
| { | ||
| public partial class IOperationTests : SemanticModelTestBase | ||
| public partial class IOperationTests_NullCheckedParameters : SemanticModelTestBase |
| } | ||
|
|
||
| [Fact] | ||
| [Fact(Skip = "PROTOTYPE(param-nullchecking): MakeMemberMissing doesn't work as expected with our method of obtaining Nullable<T>.HasValue in this scenario")] |
There was a problem hiding this comment.
Will address in a later PR.
|
|
||
| ImmutableArray<string> names = default; | ||
| ImmutableArray<RefKind> refKinds = default; | ||
| ImmutableArray<bool> nullCheckedOpt = default; |
There was a problem hiding this comment.
Consider using a BitVector for this.
There was a problem hiding this comment.
Seems reasonable. Will look at this in a follow-up PR.
4 participants
Related to #36024