Nullable extensions: Add assertion to AsMemberOfType and handle failures #79428
Conversation
| if (expr == null || _disableNullabilityAnalysis) return; | ||
| if (expr == null | ||
| // BoundExpressionWithNullability is not produced by the binder but is used within nullability analysis to pass information to internal components. | ||
| || expr.Kind == BoundKind.ExpressionWithNullability |
There was a problem hiding this comment.
At first I tried to just add this kind to s_skippedExpressions. But that just controls whether the debug visitor will visit that kind. Essentially it just states an expectation that NullableWalker won't visit something, when, what we want here is the opposite, to tolerate that NullableWalker will visit something which the debug verifier won't visit. Possibly I misunderstood something, though, and this isn't the best place to make this change, or an appropriate type to use.
There was a problem hiding this comment.
This is basically being done so that we can cook up a receiver expression for the property pattern access, and let the VisitArguments machinery handle the reinference of the extension, conversion of property receiver to extension parameter type, etc.
There was a problem hiding this comment.
I guess my question is: should we actually be peaking through the BoundExpressionWithNullability here and setting the underlying expression's state? Feels like we may miss something if we don't?
There was a problem hiding this comment.
The issue is there is no underlying expression in the bound tree for an extension property pattern. I just know what the receiver type is and would like to thread it through, having all the extension reinference and conversion checks from VisitArguments just work.
| if (expr == null || _disableNullabilityAnalysis) return; | ||
| if (expr == null | ||
| // BoundExpressionWithNullability is not produced by the binder but is used within nullability analysis to pass information to internal components. | ||
| || expr.Kind == BoundKind.ExpressionWithNullability |
There was a problem hiding this comment.
I guess my question is: should we actually be peaking through the BoundExpressionWithNullability here and setting the underlying expression's state? Feels like we may miss something if we don't?
| item = null; | ||
| } | ||
| var list = M2(item); // List<string?> |
There was a problem hiding this comment.
Consider verifying this assertion. #Resolved
| item = null; | ||
| } | ||
| var list = M2(item); // List<string?> |
There was a problem hiding this comment.
Consider verifying this assertion. #Resolved
| item = null; | ||
| } | ||
| var list = M2(item); // List<string?> |
There was a problem hiding this comment.
Consider verifying this assertion. #Resolved
…rlier * upstream/main: (217 commits) Fix tests Fix tests Fix tests Fix tests Fix tests Fix tests Reduce allocations during CommonCompletionItem.Create (dotnet#79591) Fix tests Fix tests Fix tests Fix tests Add test Fix tests Add work item Fix eol handling on the last token in a file when formatting code actions remove unchecked values from tests [main] Source code updates from dotnet/dotnet (dotnet#79599) Nullable extensions: Add assertion to AsMemberOfType and handle failures (dotnet#79428) Avoid adding dependency on System.Threading.Channels to InteractiveHost (dotnet#79594) Update debugger contracts to 18.0.0-beta.25353.1 (dotnet#79277) ...
#78828
Also handles some extension property patterns cases.
Relates to test plan #76130