Logic error when converting ByRef parameters

[GrahamTheCoder]

Originally reported on a wordpress comment.

Input code

Public Class MyTestClass

	Private Property Prop As Integer
	Private Sub TakesRef(ByRef vrbTst As Integer)
            vrbTst = vrbTst + 1
	End Sub

	Private Sub UsesRef(someInt As Integer)
		TakesRef(Prop)
		someInt += 1
	End Sub
End Class

Erroneous output

public class MyTestClass
{
    private int Prop { get; set; }
    private void TakesRef(ref int vrbTst)
    {
        vrbTst = vrbTst + 1;
    }

    private void UsesRef(int someInt)
    {
        var argvrbTst = Prop;
        TakesRef(ref argvrbTst);
        someInt += 1;
    }
}

Expected output

public class MyTestClass
{
    private int Prop { get; set; }
    private void TakesRef(ref int vrbTst)
    {
        vrbTst = vrbTst + 1;
    }

    private void UsesRef(int someInt)
    {
        var argvrbTst = Prop;
        TakesRef(ref argvrbTst);
        Prop = argvrbTst; // Need to set value of property afterwards to maintain logic
        someInt += 1;
    }
}

Details

Should also:

• Create self-verifying tests to check cases where an implicit type conversion happens
• Check what happens when semantic info is incomplete (that it "errs" in the sensible direction)

See

CodeConverter/ICSharpCode.CodeConverter/CSharp/NodesVisitor.cs

Lines 1460 to 1469 in c70e6aa

	AdditionalLocals.AdditionalLocal local = null;
	if (refKind != RefKind.None && NeedsVariableForArgument(node)) {
	local = _additionalLocals.AddAdditionalLocal($"arg{argName}", expression);
	}
	var nameColon = node.IsNamed ? SyntaxFactory.NameColon((IdentifierNameSyntax)node.NameColonEquals.Name.Accept(TriviaConvertingVisitor)) : null;
	if (local == null) {
	return SyntaxFactory.Argument(nameColon, token, expression);
	} else {
	return SyntaxFactory.Argument(nameColon, token, SyntaxFactory.IdentifierName(local.ID).WithAdditionalAnnotations(AdditionalLocals.Annotation));
	}

If this solution is implemented, we should also bear in mind #310

[tverweij]

The expected output is still incorrect.
In this case the byRef method does not have a result - and makes it correct, but when the method has a result, the sequence of events is not correct

Changed the VB code to have a result:

Public Class MyTestClass
        Private SomeField As String
	Private Property Prop As Integer
	Private Function TakesRef(ByRef vrbTst As Integer) As String
            vrbTst = vrbTst + 1
            Return "Foo"
	End Function

	Private Sub UsesRef(someInt As Integer)
		SomeField = TakesRef(Prop)
                someInt += 1;
	End Sub
End Class

The sequence that happens in UsesRef is:

1. Prop is copied to a local
2. TakesRef is called with this local
3. The local is copied back to Prop
   4 SomeField is assigned

In the proposed solution step 3 and 4 are reversed, and that can (and will) cause changes in behavior when the code is called asynchronously.

The only way to get the correct behavior is to use one extra local to store the returnvalue:

    private void UsesRef(int someInt)
    {
        var argvrbTst = Prop;
        var resltvrTst = TakesRef(ref argvrbTst);
        Prop = argvrbTst; // Need to set value of property afterwards to maintain logic
        SomeField = resltvrTst; //now copy the result to preserve the proper sequence of events
        someInt += 1;
    }

[GrahamTheCoder]

Sorry I somehow missed this comment. You're absolutely right about the difference in logic for the solution I proposed. Now you've pointed that out, it brings to mind some further issues.

Imagine that TakesRef is implemented like this:

	Private Function TakesRef(ByRef vrbTst As Integer) As String
            vrbTst = vrbTst + 1
            Throw New Exception
	End Function

We'd also need a try...finally block at each call site to assign the result back like it would have been if it was a real ref

Now imagine TakesRef is implemented like this:

	Private Function TakesRef(ByRef vrbTst As Integer) As String
            vrbTst = vrbTst + 1
            vrbTst = vrbTest + Prop
            Throw New Exception
	End Function

Now we're really stuck. We can't possibly get the correct value for Prop within the method if we're only setting it after the method call.

Therefore I think the correct solution here is to output a property with a backing field (with the same accessibility) and pass that field by ref where needed. It'd be interesting to know if this works cross-project in VB, and if so, what the compiled IL looks like.

Off topic note on the asynchronous access you mentioned in your example: Be aware that in general you can't depend on seeing effects in program order from a different thread. If you need that guarantee, you should use a lock statement around the statement that must not be re-ordered. See Eric Lippert's post or part 3.10 of the C# spec

[tverweij]

If you look at the IL when compiling the Vb code, you see that VB is compiling exactly as I mentioned it. Your example won’t change the var as it is copied back after the call. From: GrahamTheCoder [mailto:[email protected]] Sent: Saturday, March 7, 2020 10:53 AM To: icsharpcode/CodeConverter Cc: Theo Verweij; Comment Subject: Re: [icsharpcode/CodeConverter] Logic error when converting ByRef parameters (#324) Sorry I somehow missed this comment. You're absolutely right about the difference in logic for the solution I proposed. Now you've pointed that out, it brings to mind some further issues. Imagine that TakesRef is implemented like this: Private Function TakesRef(ByRef vrbTst As Integer) As String vrbTst = vrbTst + 1 Throw New Exception End Function We'd also need a try...finally block at each call site to assign the result back like it would have been if it was a real ref Now imagine TakesRef is implemented like this: Private Function TakesRef(ByRef vrbTst As Integer) As String vrbTst = vrbTst + 1 vrbTst = vrbTest + Prop Throw New Exception End Function Now we're really stuck. We can't possibly get the correct value for Prop within the method if we're only setting it after the method call. Therefore I think the correct solution here is to output a property with a backing field (with the same accessibility) and pass that field by ref where needed. It'd be interesting to know if this works cross-project in VB, and if so, what the compiled IL looks like. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#324?email_source=notifications&email_token=AJZWR6HY34HHXACCB5CXGP3RGJNVNA5CNFSM4IA2OAD2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEOD3LWA#issuecomment-596096472> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJZWR6HYDOD42WU2PSPVNLTRGJNVNANCNFSM4IA2OADQ> . <https://github.com/notifications/beacon/AJZWR6GR2622XYNDMOLL6ADRGJNVNA5CNFSM4IA2OAD2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEOD3LWA.gif>

[GrahamTheCoder]

Consolidating #310 which would also be covered by the backing field solution

[GrahamTheCoder]

Ah thanks for checking, that's very interesting - I'm surprised the VB team went with that solution!
Obviously the implementation you suggested is the best to emulate it then.

[GrahamTheCoder]

Consolidating #531 into this, either a duplicate or a variant with missing semantic info