Warn when using <Out> without ByRef

[chucker]

Since VB 9, using ByVal is no longer mandatory, making method declaration syntax quite a bit cleaner. For ByRef, a keyword is still required, and unlike with C#, VB doesn't have an out-specific keyword. Instead, you need to use both ByRef and apply the OutAttribute, for example:

Private Function TryGetValue(obj As Object, <Out> ByRef result as Integer) As Boolean

End Function

This makes it easy to introduce a bug* that doesn't become apparent until runtime:

Private Function TryGetValue(obj As Object, <Out> result as Integer) As Boolean

End Function

This code looks fine at first glance, But the <Out> attribute doesn't actually do anything meaningful (best as I can tell), because result is actually implicitly ByVal now, not ByRef.

Thus, I propose that the compiler annotate this as a warning:

OutAttribute used on ByVal method parameter. Consider using ByRef.

*) Well, this has embarrassingly happened to me, anyway.

[paul1956]

I think there is a proposal to fix the compiler to not return an error when an uninitialized variable is passed to an parameter. This would be a good place to add this warning.

[reduckted]

#67 is the issue that @paul1956 is referring to.

Why don't we ask to add OutRef and InRef to VB.NET Instead?

Besides, I think this issue doesn't need a warning. The code editor can just add the Byref after <Out>, or the compiler can add it when generating IL. If <out> is there, there are no other assumptions put this param is ByRef, so take action directlty. I think this issue should go to Roslyn.

[chucker]

(I did see #67, but figured the two were distinct enough to warrant a separate discussion.)

[KathleenDollard]

This is a good idea. We have concerns about it as a breaking change, but we are thinking about solutions to that.

[reduckted]

@KathleenDollard We have concerns about it as a breaking change, but we are thinking about solutions to that.

Could you elaborate on the concerns you have? I'm interested to find out what this could possibly break. Is there actually a legitimate reason to have a parameter marked as <Out()>, but not have it marked as ByRef?

[KathleenDollard]

We consider new warnings to be breaking changes because they are breaking if you have "Treat warnings as errors" turned on. While this is a mistake and not doing what it says, production apps could be running quite happily with this mistake.

[AdamSpeight2008]

@KathleenDollard It would be a breaking change if new warning waves were introduce to existing released language version. The new warning would appear in updating the project / solution.
So long as the new warnings are documented in the changes log along with new features, I think they should be ok in some cases.

[jrmoreno1]

I haven't ever used the out attribute, could this not be done by a Roslyn analyzer?

[paul1956]

The only thing <out> was supposed to do was suppress the warning about use of "result" before it is initialized. An analyzer that caught this and warned about the old result initializer not being necessary I think would nicely address this until a future version when it should be at least a warning. Generally with the attribute present I would expect the code to be broken if ByRef was missing and the value was no longer initialized.

[sharwell]

It does seem like this could be covered with an analyzer. In C#, I believe there are some P/Invoke cases where [Out] is relevant even when the argument is passed by value (instructs the marshaller to copy data back from unmanaged code to managed code, e.g. as part of a StringBuilder or array). If those cases are relevant in VB, we should make sure the analyzer does not report false positives on them.