Add string ctor to MemberNotNull/When #42712
Conversation
jcouv
added
Area-Compilers
New Language Feature - Nullable Reference Types
| { | ||
| if (targetSignature[signatureByteIndex - 1] != (byte)SignatureTypeCode.SZArray) | ||
| { | ||
| return false; |
There was a problem hiding this comment.
📝 there was a bug here. This method could not distinguish properly between the string case and the string[] case. To tighten that logic, I had to refactor a bit (extract a local function).
| { | ||
| var memberName = member.DecodeValue<string>(SpecialType.System_String); | ||
| string? memberName = value.DecodeValue<string>(SpecialType.System_String); |
There was a problem hiding this comment.
Won't this throw in the case where the customer uses a version of MemberNotNull where there is a ctor with a non-string argument? I'm unsure what our tolerance here is for malformed attributes. Know @RikkiGibson is hitting this with the obsolete change.
There was a problem hiding this comment.
We should get here in such case. This method is only called once we've found the attribute we're interested in by AttributeDescription (which verifies one of two signatures we want was matched)
| { | ||
| return null; | ||
| if (TryExtractStringValueFromAttribute(ai.Handle, out var extracted)) |
There was a problem hiding this comment.
var [](start = 74, length = 3)
Please be explicit here: string
| var result = ArrayBuilder<string>.GetInstance(attrInfos.Count); | ||
|
|
||
| foreach (var ai in attrInfos) | ||
| else if (TryExtractStringArrayValueFromAttribute(ai.Handle, out var extracted2)) |
There was a problem hiding this comment.
var [](start = 80, length = 3)
Same here string[], and two places in the next method.
| Diagnostic(ErrorCode.WRN_MemberNotNullWhen, "true").WithArguments("field2", "True").WithLocation(29, 16), | ||
| // (29,16): warning CS8775: Member 'field3' may not have a null value when exiting with `True`. | ||
| // get => true; | ||
| Diagnostic(ErrorCode.WRN_MemberNotNullWhen, "true").WithArguments("field3", "True").WithLocation(29, 16) | ||
| ); | ||
| } |
There was a problem hiding this comment.
} [](start = 8, length = 1)
Please add a test (if there isn't one already) with multiple applications of the params version of MemberNotNull.
Please add a test with [MemberNotNull(member: null)] and [MemberNotNull(members: null)] including repetitions.
Similarly for MemberNotNullWhen.
|
Looks good (Iteration 1) but please add a couple of tests as noted. |
ghost
modified the milestones:
* upstream/master: (697 commits) Update comment for clarity More solution API tests (dotnet#42451) Task queue refactoring (dotnet#42610) Add string ctor to MemberNotNull/When (dotnet#42712) Use the same block structure for code and Metadata As Source Add work items When generating abstract members, generate them as protected, not internal Avoid operations on disposed workspace (dotnet#42768) Remove duplicate validation logic Speed up regex comment detector pattern. Stop walking up once we hit a statement Speed up regex comment detector pattern. Strengthen the condition in which nodes are determined to be in the same block Remove the SolutionPopulator incremental analyzer. Update Visualizer version again to fix bug, actually use invariant culture. Formatting: force a space after attribute on parameter (dotnet#42466) Add test for different case VB Update src/EditorFeatures/VisualBasicTest/CodeActions/ReplaceMethodWithProperty/ReplaceMethodWithPropertyTests.vb Update src/EditorFeatures/CSharpTest/CodeActions/ReplaceMethodWithProperty/ReplaceMethodWithPropertyTests.cs GenerateUniqueName for ReplaceMethodWithProperty ...
Follow-up on PR #41675 (Add support for MemberNotNull/When attributes)
The constructors with
string[]are not CLS-compliant and that causes problems for the BCL team. From email discussion with LDM, we'll add simple constructors that take a single member name at a time, and we'll allow the attributes to be instantiated multiple times.Relates to #41438 (Implement
MemberNotNullnullability attribute)Corresponding BCL change: dotnet/runtime#33864