Question 135065 in Visual Studio Code Question posted in
View the official documentation.

Visual Studio Code – How can I detect when a ReadOnly property has accidentally been passed by reference?

I’m working on a project which is written in VB.NET. The project has several Structures which used to have writable fields. I replaced all of those fields with read-only properties, and wrote functions for creating a copy of a structure that has one of its properties changed.

I was assuming that every part of the code that attempted to write to one of these properties would become an error, and then I could simply fix all the errors by making the code call the new functions. To my dismay, it turns out that if a ReadOnly property is accidentally passed into a ByRef parameter of a function, the compiler accepts this with no warning, and the value that’s assigned is silently discarded!

Here’s an example:

Structure Point
    Public ReadOnly Property X As Integer
    Public ReadOnly Property Y As Integer
End Structure

Module Module1
    Sub IncreaseByOne(ByRef x As Integer)
        x = x + 1
    End Sub

    Sub Main()
        Dim point As New Point
        IncreaseByOne(point.X)
        Console.WriteLine($"point.X is {point.X}")
    End Sub
End Module

I was hoping that the line IncreaseByOne(point.X) would throw an error, or at least a warning, since point.X is read-only and it doesn’t make sense to pass it by reference. Instead, the code compiles with no warnings, and the value assigned to x inside of IncreaseByOne is silently discarded, and the program prints point.X is 0.

How can I detect all of the places in my code where a read-only property is passed into a function that takes it by reference? The only way I can think of is to go through every read-only property that I have, find all places where that property is used as a parameter, and look to see if that parameter is ByRef. That’ll be very time-consuming, but if there’s no other solution, then that’s what I’ll do.

I’m using Visual Studio 2019. I’m open to installing new software in order to do this.

Tags:

3

Answers


  1. Chosen as BEST ANSWER
    0

    As Craig suggested in this answer, I went ahead and wrote a custom analyzer to detect when this occurs. Now, I can simply do Analyze / Run Code Analysis / On Solution, and every place that the described problem occurs gets marked with a warning such as "The property 'point.X' is read-only and should not be passed by reference."

    The entire analyzer is available on GitHub. I've copied the important part below:

    public override void Initialize(AnalysisContext context)
{
    context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
    context.EnableConcurrentExecution();

    context.RegisterSyntaxNodeAction(AnalyzeSyntax, SyntaxKind.SimpleArgument);
}

private static void AnalyzeSyntax(SyntaxNodeAnalysisContext context)
{
    SimpleArgumentSyntax node = (SimpleArgumentSyntax)context.Node;
    SemanticModel semanticModel = context.SemanticModel;

    if (!IsByRef(node, semanticModel))
        return;

    (bool isReadOnly, string symbolType) = IsReadOnly(node, semanticModel);

    if (isReadOnly)
    {
        Diagnostic diagnostic = Diagnostic.Create(
            Rule,
            node.Expression.GetLocation(),
            symbolType,
            node.Expression.GetText());
        context.ReportDiagnostic(diagnostic);
    }
}

/// <summary>
/// Determine if the given argument is passed by reference.
/// </summary>
private static bool IsByRef(SimpleArgumentSyntax node, SemanticModel semanticModel)
{
    ArgumentListSyntax argumentList = (ArgumentListSyntax)node.Parent;

    if (argumentList.Parent is InvocationExpressionSyntax invocation)
    {
        SymbolInfo functionInfo = semanticModel.GetSymbolInfo(invocation.Expression);
        if (functionInfo.Symbol is IMethodSymbol method)
        {
            IParameterSymbol thisParameter = null;

            if (node.IsNamed)
            {
                thisParameter = method.Parameters.FirstOrDefault(parameter =>
                    parameter.Name == node.NameColonEquals.Name.ToString());
            }
            else
            {
                int thisArgumentIndex = argumentList.Arguments.IndexOf(node);
                if (thisArgumentIndex < method.Parameters.Length)
                    thisParameter = method.Parameters[thisArgumentIndex];
            }

            // If we couldn't find the parameter for some reason, the
            // best we can do is just accept it.
            if (thisParameter == null)
                return false;

            RefKind refKind = thisParameter.RefKind;
            if (refKind != RefKind.None && refKind != RefKind.In)
                return true;
        }
    }

    return false;
}

/// <summary>
/// Determine if the given argument is a read-only field or property.
/// </summary>
private static (bool isReadOnly, string symbolType) IsReadOnly(SimpleArgumentSyntax node, SemanticModel semanticModel)
{
    string symbolType = "field or property";
    bool isReadOnly = false;

    if (node.Expression is MemberAccessExpressionSyntax memberAccess)
    {
        SymbolInfo memberInfo = semanticModel.GetSymbolInfo(memberAccess.Name);

        if (memberInfo.Symbol is IPropertySymbol propertySymbol && propertySymbol.IsReadOnly)
        {
            symbolType = "property";
            isReadOnly = true;
        }

        if (memberInfo.Symbol is IFieldSymbol fieldSymbol && fieldSymbol.IsReadOnly)
        {
            symbolType = "field";
            isReadOnly = true;
        }
    }

    return (isReadOnly, symbolType);
}

  2. 0

    There isn’t a way to catch this with the compiler. Even Option Strict On will allow passing a read-only property to a ByRef argument. This is defined to pass by copy-in/copy-out, and it’s surprising to me that the copy-out part will compile even when the Property Set is inaccessible.

    If you want to have an automated lint-type check for this, I would imagine that a custom analyzer could find it. I haven’t worked with analyzers, so I don’t have any specific suggestions for how to write one or set it up.

    Otherwise, you’re left to a manual check. As was noted in a comment, you can use the "Find All References" command from Visual Studio to help with it, but this will still require a manual review of every read-only property.

    Login or Signup to reply.
  3. 0

    That’s really interesting. The VB.NET Compiler really tries to make a property look like a variable. Even if I explicitly declare the property as

    Structure Point
    Dim _x As Integer

    ReadOnly Property X() As Integer
        Get
            Return _x
        End Get
    End Property
End Structure

    The code compiles and executes as before. If the property setter is added, it even works correctly!

    Structure Point
    Dim _x As Integer

    Property X() As Integer
        Get
            Return _x
        End Get
        Set(value As Integer)
            _x = value
        End Set
    End Property
End Structure

    With the above change, the program correctly prints 1.

    Looking at the generated IL, we can see why:

        IL_0009: ldloca.s     point
    IL_000b: call         instance int32 VisualBasicConsoleTest.Point::get_X()
    IL_0010: stloc.1      // Store returned value in local variable
    IL_0011: ldloca.s     // load address of that local variable (and pass to function call)
    IL_0013: call         void VisualBasicConsoleTest.Program::IncreaseByOne(int32&)
    IL_0018: nop
    IL_0019: ldloca.s     point
    IL_001b: ldloc.1      // Load contents of local variable again
    IL_001c: call         instance void VisualBasicConsoleTest.Point::set_X(int32) // and call setter

    Even though we expect an error because a property is not a value (and a byref requires a value), the compiler fakes what we might have intended: He actually generates a call to the getter, stores the value on the stack, passes a reference to the stack(!) to the called function and then calls the setter with that value.

    This works in this simple scenario, but I agree with the commenters above, this might be very confusing when looking at it in detail. If the property is actually a computed property, the outcome is just arbitrary (try implementing the getter as Return _x + 1…)

    C# would throw an error here, because a property is not a value and hence cannot be used as an out or ref parameter.

    Login or Signup to reply.
Please signup or login to give your own answer.

Back To Top
Search