Unassigned variable warning reported when passing variables intended as `Out` arguments

[gafter]

@paul1956 commented on Wed Jun 08 2016

Version Used: VS 2015 Update 2 Steps to Reproduce:

1. Declare Function with <Out()> attribute.

    Public Function Return6(<Out()> ByRef LastErrorMessage As String) As Integer
        LastErrorMessage = "None"
        Return 6
    End Function

1. Call Function

Six = Return6 (ReturnError)

Expected Behavior: No Warnings, I don't want to have to assign a default value to an Out variable like ReturnError to make the warning disappear. Actual Behavior:

Warning BC42030 Variable 'ReturnError' is passed by reference before it has been assigned a value. A null reference exception could result at runtime.

[HeinziAT]

That would be great, since the BCL uses Out parameters quite heavily. Currently, VB users need to clutter their code with useless variable initializations before using the Try... methods:

Dim myDouble As Double = 0   ' Just initialized to avoid the warning
If Double.TryParse(myString, myDouble) Then ...

This, in turn, leads to the "just initialize all variables with some dummy value" anti-pattern, which, in turn leads to more bugs:

Dim foos As Int32 = 0

If someCondition Then
    foos = 3
Else
    ' Oops, forgot to set foos to a useful value here, and the initialization earlier prevented the compiler from detecting that.
End If

Frobnicate(foos)

[AnthonyDGreen]

I hate that warning and that anti-pattern, @HeinziAT

[rskar-git]

Question: Was there any thought about maybe making ByRefOut and ByRefIn keywords?

[AnthonyDGreen]

Not specifically, no. Because of covariance and contravariance In and Out are already keywords. But there's a consumption story and a production story. Are you also interested in creating methods with Out parameters? Is there a scenario other than multiple return values?

[rskar-git]

To be clear, I meant one big keyword, e.g. ByRefOut, and not necessarily about re-purposing either In or Out.

I'm only asking because while it looks like one can use <Out()> (I guess), the resulting (annoying) warning message that @paul1956 (via @gafter) points out is happening probably because it's not something that one would think of in the VB world. There is after all only ByRef, and it makes no promise of assignment within the call. Instead, while there isn't any demand of assignment prior to the call, there is nonetheless the warning.

So paul1956's example:

Public Function Return6(<Out()> ByRef LastErrorMessage As String) As Integer
    LastErrorMessage = "None"
    Return 6
End Function

...could instead be:

Public Function Return6(ByRefOut LastErrorMessage As String) As Integer
    LastErrorMessage = "None"
    Return 6
End Function

...and now appropriate warnings/errors can be recognized more naturally.

It does happen, from time-to-time, that I need to use ByRef to instantiate for one or more values, in both Sub's and Function's. Heck, the various TryParse methods in the .NET FW are cases in point.

I think what's nice about Out parameters is partly what @HeinziAT said, it would be nice to reduce the clutter. But also (and perhaps HeinziAT would agree with this too) it would be nice if the compiler could take notice if a function might fail to do any assignment for any of its Out parameters with a warning; e.g. if Function Return6() might not assign a value to LastErrorMessage on every path.

While typing all this out, I'm now wondering this: Do you regard Out parameters as something passé? Will VB take a pass on (C#-ish) Out variables?

[HeinziAT]

A good question. I fully support making out parameters easier to consume, but I have mixed feelings about making them easier to produce. If we go that way, some dedicated keyword, as @rskar-git suggested, would surely be nice (might as well use Out instead of ByRefOut - I don't think there is a parsing ambiguity). But do we want to? Now that named tuples are a first-class citizen of the language, is there still a need for creating methods with out parameters?

[paul1956]

Don't need "(C#-ish) Out variables" if you are going to replace the "TryParse" methods and anything else that uses ByRef to return something plus break all the code that used them (dripping sarcasm}. VB already has \<Out> and it works to make intent clear to users and you can write an analyzer to verify that the called function always sets it. But you can't stop the compiler for putting out the bogus warning without the anti-pattern of initializing the variable passed to the function which makes the code confusing and error prone.

[AnthonyDGreen]

My newest thoughts on this is a pretty cheap solution. Out arguments. It's a very localized feature. We let you specify the keyword Out on any argument in a call list corresponding to a ByRef parameter. If Out is specified by the user definite assignment analysis will consider that use to be a write instead of a read/write. This will get rid of the warning and doesn't require implementing any notion of an Out parameter. It's just a modifier on argument passing and can work with any ByRef parameter.

[paul1956]

@AnthonyDGreen why not just implement your recommended logic with the already allowed \<Out> I am happy either way, just one requires no code changes in the client.

[AnthonyDGreen]

@paul1956,

Good point. I'm trading syntax for work in the symbols/metadata importer. The LDM will need to figure out which tradeoff is preferable and why. Thanks!

[AdamSpeight2008]

@AnthonyDGreen There is / was proposal (I think in the Roslyn repo) to add a Code Fix for this. By adding = Nothing at the declaration site.