Added notes about interpolated strings in nullable flow analysis #5550
Conversation
|
@AlekseyTs @RikkiGibson for a look. This is documenting the limitations around nullable flow analysis from the roslyn PR. |
| types to inform generic type inference for type parameters in the containing method. An example of where this can have an impact is: | ||
|
|
||
| ```cs | ||
| string s = """"; |
There was a problem hiding this comment.
Was it intended to have doubled quote marks here and below? This looks like it might have been left over from pasting in a test.
| ```cs | ||
| string s = """"; | ||
| C c = new C(); | ||
| c.M(s, $"""", c.ToString(), s.ToString()); // No warnings on c.ToString() or s.ToString(), as the `MaybeNull` does not flow back. |
There was a problem hiding this comment.
I wouldn't expect [MaybeNull] to have any impact here (interpolated strings are no). That attribute should only be impacting the input position, not the output position. What am I missing?
There was a problem hiding this comment.
AllowNull is for inputs, MaybeNull is for outputs.
There was a problem hiding this comment.
Huh, seems odd for that to have value in non-ref parameters. That isn't an output
There was a problem hiding this comment.
Consider Assert.NotNull. That will use a NotNull attribute on a by-val parameter and throw/assert if it's null.
There was a problem hiding this comment.
Correct. But it's reasonable for an API to strengthen our understanding of a piece of nullable state. The implementation of a method can do all manner of tricks to verify that a piece of input is not null. Effectively elevating the state from "maybe null" to "not null".
Having [MaybeNull] impact the state is saying that an API can downgrade the state. Taking something the compiler believes is "not null" and making it "maybe null". Having trouble seeing how an API can make such an assertion. Basically because you passed A to me you should assume A is now possible null.
There was a problem hiding this comment.
I don't see a real reason to have it not impact things, and I don't think that this spec is the place to make the argument to change the feature in any case 😀
There was a problem hiding this comment.
Don't want my debate to be about changing the feature. Want to make sure I understand [MaybeNull] so I can better evaluate this change.
There was a problem hiding this comment.
Basically, in an ideal world, this feature would behave exactly as if you called all these methods in order manually, and that would mean any nullability post-condition attributes flowing back to the original expression (MaybeNull included). For implementation simplicity, we have avoided this.
|
Looks like this is ready to go just pending minor fixes to the quotes in code examples. |
Followup from dotnet/roslyn#57780.
Relates to test plan dotnet/roslyn#51499