vblang icon indicating copy to clipboard operation
vblang copied to clipboard

Using readonly property as target of ByRef

Open gafter opened this issue 6 years ago • 3 comments

@jrmoreno1 commented on Fri Aug 26 2016

Microsoft Visual Studio Enterprise 2015 Version 14.0.25123.00 Update 2 Microsoft .NET Framework Version 4.6.01590

Visual Basic 2015 00322-90150-04967-AA372 Microsoft Visual Basic 2015

Steps to Reproduce: Compile

Dim nlDate As Date? = #01/01/2015# If (Date.TryParse("07/04/2016", nlDate.Value)) Then Console.WriteLine(nlDate) End If

Expected Behavior: Compiler error ('ReadOnly' Property cannot be the target of an assignment) Actual Behavior: Compiles, and when run the parse succeeds, but the nullable date is unchanged. Checked with VS 2013, and same behavior, so it's not a change due to Roslyn, but I do think it's an error.


@gafter commented on Fri Aug 26 2016

I believe the VB language spec specifically permits this, instead sending a ref to the temporary value that it has read the value into. @AnthonyDGreen Can you please confirm?


@markhurd commented on Fri Aug 26 2016

Because VB.NET doesn't acknowledge Out parameters as more than ByRef, this makes sense: as far as VB is concerned you might just be expecting to pass in the property as an argument.


@paul1956 commented on Fri Aug 26 2016

Please someone fix VB so it acknowledge "Out" parameters even if it is only for new code. Currently the "Out" attribute is completely ignored. Lack of this makes code less clear and for readonly variables being passed to Out parameters subtle bugs.


@jrmoreno1 commented on Sun Aug 28 2016

@markhurd : ref or out, it doesn't make a difference. Either way you are potentially making an assigment to a readonly value. If you only need the value, then the argument should be ByVal.


@jrmoreno1 commented on Sun Aug 28 2016

@gafter: VB.NET does allow using properties for ByRef arguments, but it shouldn't allow doing so for ReadOnly properties. Your comment prompted me to check readonly fields, and it has the same behavior. I also looked at the IL and there is no attempt to assign to the Property (or the readonly field).


@jrmoreno1 commented on Fri Sep 02 2016

I don't know if VB is scheduled to get readonly locals, but if so it's going to get even more confusing if readonly local variables can't be passed to as a ref argument but readonly field/properties can. And if readonly locals are allowed, that's going to cause a lot of confusion between vb and C#.

gafter avatar Sep 28 '17 00:09 gafter

This is by design. In VB6 ByRef was the default so passing any argument would have been ByRef, ReadOnly properties included. It might be worth considering a warning it you pass a readonly property or variable to a parameter marked as <Out>. That could dovetail with our resolution to #152.

AnthonyDGreen avatar Oct 02 '17 00:10 AnthonyDGreen

Page 130 of the Visual Basic Language Specification states: Copy-in copy-back. If the type of the variable being passed to a reference parameter is not compatible with the reference parameter's type, or if a non-variable (e.g. a property) is passed as an argument to a reference parameter, or if the invocation is late-bound, then a temporary variable is allocated and passed to the reference parameter. The value being passed in will be copied into this temporary variable before the method is invoked and will be copied back to the original variable (if there is one and if it's writable) when the method returns.

So it looks like this behavior is according to the specification.

ghost avatar Feb 16 '18 19:02 ghost

In case anyone else runs into the same problem in the future, I've written a code analyzer that detects when it happens. https://github.com/tswett/FindReadOnlyByRef/

This code analyzer issues a warning for every ReadOnly field or property that's passed into a parameter that takes it ByRef.

Hopefully, a rule similar to this one will be added to Roslyn soon. Making it a compiler error probably wouldn't be practical, since a lot of code accidentally relies on the current behavior, but it could definitely be a warning.

tswett avatar Dec 08 '22 20:12 tswett