GenerateType isn't aware of nullability

[jcouv]

The refactoring is successful, but nullability annotations (?) are missing in the generated code.

Note: this screenshot doesn't actually demonstrate the bug, since x2 and y2 are both non-null at that point in the code.

FYI @dpoeschl

[CyrusNajmabadi]

Julien, how is this expected to work? What is the type of the 'x2' local? Is it string? (because that's how it was declared), or is it string, because flow analysis determined it is never null?

if the former, is this just a matter of SyntaxGenerator needing to be updated to just put in the question? If it's the latter, is this actually a bug? Seems fine that this would generate 'string' if we had provent he value is not nullable at that point.

[jcouv]

My expectation the latter: we should use the null-state of the expression (rather than declared nullability for the few expressions where that is applicable). For example, generating M from M(MakeArray(nullableString)) should generate void M(string?[] x).

The reason why is because you're generating from a point where we see that your passing null, so the generated API should accept null (that's likely what you want, based on usage). Otherwise the refactoring results in diagnostics.

[CyrusNajmabadi]

My expectation the latter: we should use the null-state of the expression (rather than declared nullability for the few expressions where that is applicable).

Ok agreed. In that case, isn't this working correctly? both x2 and y2 are non-null thanks to flow analysis. x2 is assigned a non-null string, and y2 is assigned x2, so it would be non-null. So we shuld generate the type without nullability annotations...

[CyrusNajmabadi]

another concern i just realized. should we be taking [NonNull] annotations into account? i.e. if you're in a type that has this annotation, and we generate a new type, should we add this attribute to it?

[jcouv]

Your correct, I made a mistake in this example (the ! on null makes it non-null).
Yes, we'll have to deal with context. As a side-note, the context is going to switch to using some form of directive syntax (#nonnull or some such), which may further complicate refactorings.

[jasonmalinowski]

This works now -- we'll close out the bug once we add tests.