VisitLvalue [](start = 25, length = 11)

📝 The role of VisitLValue is now handled by regular Visit, by storing a l-value type in the result that we bubble up. #Resolved

Visit [](start = 12, length = 5)

📝 I'm a bit unsure whether we should use VisitRValue instead (which does Visit then Unsplit).

ImmutableArray VisitArguments [](start = 16, length = 42)

📝 here we visit arguments and store the full result, so that we can use both the result type and the l-value type later on. #Resolved

struct [](start = 16, length = 6)

readonly struct #Resolved

ResultType and _lValueType can be set.

In reply to: 258110524 [](ancestors = 258110524)

Can the struct be made immutable though?

In reply to: 258231060 [](ancestors = 258231060,258110524)

TypeSymbolWithAnnotations valueType, TypeSymbolWithAnnotations targetType [](start = 80, length = 73)

Why switch the order? #Resolved

Explained elsewhere: to put value and valueType together. The previous order of parameters regularly tripped me up.
We can discuss. I recognize the order change is not essential to this PR.

In reply to: 258133264 [](ancestors = 258133264)

I'll do the re-ordering in a separate PR after Neal merges his big change, to minimize conflicts.

In reply to: 258229068 [](ancestors = 258229068,258133264)

TypeSymbolWithAnnotations targetType, int targetSlot, TypeSymbolWithAnnotations valueType [](start = 76, length = 89)

Why switch the order? #Resolved

How do we represent the case where it is not an lvalue at all? #Resolved

In such case, LValueType gets the same value as ResultType. This emulates previous behavior, where VisitLvalue would default to calling VisitRValue.

In reply to: 258140214 [](ancestors = 258140214)

Consider: SetResultType to line up with the property of the same name. #Resolved

SetResultType doesn't seem right. This method should be named the same as SetResult(BoundExpression).
I think those two methods should be called SetResult or SetVisitResult.

In reply to: 258141377 [](ancestors = 258141377)

This is the case I'm thinking about when I asked about knowing if it's a valid lvalue before. There is no understanding here whether this is or isn't a valid lvalue. Yet we're presenting the type now as a LValueType. #Resolved

I'll take another look at an alternative and will get back to you.

In reply to: 258142088 [](ancestors = 258142088)

Changed the VisitResult to hold a default LValueType for expressions that aren't l-values.

In reply to: 258232704 [](ancestors = 258232704,258142088)

This error message reads like it applies to the input of the ref, not the fact that M2 could assign null into x3 in the body. Perhaps a better error message would be something like 'M2' can assign 'null' into 'x3'. #WontFix

This is going to be tricky to fix. This should not be a safety warning, but a W-warning. The problem is that we have a single W-warning (8600), thus a single diagnostic message...
The message isn't great, but it is functionally correct, so I'm tempted to leave as-is for now. We could file an issue... Let me know what you think.

In reply to: 258170299 [](ancestors = 258170299)

output [](start = 158, length = 6)

Having the one difference between these error messages be 19 words in makes these appear to be duplicates at first blush. Perhaps reword to be 'CL0<string?>' cannot be used an input/output type of 'CL0<string>' for parameter 'x' in 'void C.M1(ref CL0<string> x)' due to differences in the nullability of reference types. This moves the different word to 6 words in. Not perfect, but I can't think of a better wording off the top of my head that puts it first or second. #Resolved

Or, after having looked at SuppressNullableWarning_Ref_WithNestedDifferences, perhaps a wording that puts the method name at the end, but still keeps input/output as close to the front of the error as possible.

In reply to: 258173669 [](ancestors = 258173669)

I like it! Thanks for the concrete proposal.

In reply to: 258193243 [](ancestors = 258193243,258173669)

How is this first part different from MethodWithRefNullableParameter? #Resolved

How is this second part different from the added warning in PassingParameters_01, other than the extra call? If both of these aren't different, consider removing this test and adding the call to PassingParameters_01. #WontFix

Or otherwise consolidating the tests.

In reply to: 258176215 [](ancestors = 258176215)

I'll remove MethodWithRefNullableParameter which is indeed subsumed by this test. Thanks
As for PassingParameters_01, I'd prefer to leave that as-is (a little bit of redundancy, but easier to follow in my opinion).

In reply to: 258176369 [](ancestors = 258176369,258176215)

Not sure I agree with this one not getting a warning? Unlike the case below, the ! on the ref isn't suppressing any warnings. It feels wrong that putting a ! where there isn't any warning changes top-level nullability. #Resolved

The definition of the suppression operator has two aspects: (1) it changes the top-level nullability, and (2) it suppresses warnings. Presently, the former is unconditional and isn't dependent on the latter.
https://github.com/dotnet/roslyn/blob/master/docs/features/nullable-reference-types.md#suppression-operator-

In the case of a ref argument, we could discuss whether the ! should change the top-level nullability on the way in, or on the way out. But we already discussed that for out arguments with LDM (ie. the suppression does affect the value on the way out), so it seems logical to do the same for ref.

We can discuss with Chuck/Neal at next opportunity, or add an open issue to list dotnet/csharplang#2201

In reply to: 258177260 [](ancestors = 258177260)

I added a question to the linked issue.

In reply to: 258237238 [](ancestors = 258237238,258177260)

This one I do agree with, since it was declared to be not nullable and there would otherwise be a warning on the ref s2 parameter. #Resolved

This error is much clearer than the ref local assignment error. #Resolved

Should we track a pair of { Type, Slot } instead, where VisitRvalue would re-interpret the pair to an r-value result?

Tracking { Type, Slot } might also allow removing MakeSlot(BoundExpression) since the slot would already be calculated. #Resolved

I expect the VisitResult type will evolve. I think we'll use it to store multiple slots in conditional L-values. But that is beyond the present PR.

In reply to: 258182079 [](ancestors = 258182079)

From discussion with Chuck, we need to think about this alternative design some more. Will keep that separate from present PR.

In reply to: 258237980 [](ancestors = 258237980,258182079)

Same comment as above about a ! where there's no warning affecting top-level nullability. #Resolved

Nit: consider // 1, 2 #Resolved

string.Empty [](start = 20, length = 12)

I am slightly concerned by the use of string.Empty throughout the tests, as this api is unannotated. Have we standardized oblivious handling with Mads' spec in the walker yet? Consider replacing with literals. #Resolved

It feels like we should get a warning here. ref doesn't require assignment, so there's no guarantee at all that s had its state changed to string! #Resolved

I'll add an open issue. Chuck thought pretty strongly that we should assume that ref always assigns.
But we could take a more conservative approach: only treat ref arguments as assigned if the parameter is nullable.

In reply to: 258184852 [](ancestors = 258184852)

I think that this warning about the assignment to s as part of s = null part? This test seems needlessly complicated there, consider pulling that assignment out. #Resolved

This test structure is essential, as it demonstrates that the ref argument (F(s = null)) is only reported on once. This test used to visit/report twice.

In reply to: 258186794 [](ancestors = 258186794)

ResultType [](start = 54, length = 10)

Should this be called RValueType for consistency? #Resolved

parameterValue = new BoundParameter(argument.Syntax, parameter) [](start = 28, length = 63)

It's not clear why we need a BoundParameter here rather than using argument. Please add a comment. #Resolved

// #29962 Method should be called VisitValue rather than VisitLvalue. [](start = 30, length = 108)

Comment can be removed. #Resolved

SetResult(result, result); [](start = 12, length = 26)

Is this an l-value only? #Resolved

Yes, discards can only be written to, but cannot be read from.
But for IDE, you should be able to hover over it and see its type, so I think it's ok to set for the RValueType and the LValueType.

In reply to: 258673009 [](ancestors = 258673009)

public void M(out string? x) => throw null!; [](start = 4, length = 44)

Not used. #Resolved

CatchException_NullableType [](start = 20, length = 27)

Consider grouping this with the other catch tests at the beginning of this block. #Resolved

} [](start = 8, length = 1)

Perhaps:

static void M<T>() where T : Exception?
{
    try
    {
    }
    catch (T e)
    {
        e.ToString();
    }
}
``` #Resolved

That scenario worked ok, but I happened to debug it and notice that local e is T+"unknown". Filed a follow-up issue.

In reply to: 258687555 [](ancestors = 258687555)

Typo: unknown

Shouldn't e and e2 both be T!, so no warning on // 1?

T! is not a speakable type. The var should just be a T (therefore be possibly nullable). But it is a T~ (which is speakable with use of #nullable disable).

I'll add e2 = default to further illustrate the issue. Ignore that, I realized that adding e2 = default doesn't help.