Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support more complex ByRef arguments #307

Merged
merged 6 commits into from May 5, 2019

Conversation

Projects
None yet
2 participants
@mrmonday
Copy link
Contributor

commented May 1, 2019

References #264

Add support for more complicated arguments to methods with ByRef parameters.

Takes a different approach to #204.

Problem

VB supports passing anything as a ByRef parameter, where C# requires something it can take the address of.

Solution

  • When generating arguments, if the parameter is ByRef, create an identifier with a random name, annotate it (with AdditionalLocal.Annotation), and add an AdditionalLocal

  • When visiting an expression statement, generate a sensible unique variable name, insert additional local declarations, and update the name of the argument.

  • When calling a method with ByRef parameters in a field initializer, generate a method without ByRef to call instead

  • We only handle the easiest of cases for field initializers - a lot of the functionality needs to overlap with #281. We throw errors for some (not all) of the more complex cases. The rest should fail to compile in the generated C# anyway.

  • Add some additional type conversion support, for explicit upcasting. VB supports ByRef for mismatching types - we need to insert an explicit cast (and therefore, an additional local variable).

  • At least one test covering the code changed

  • All tests pass

@mrmonday mrmonday changed the title WIP: Reference parameters Support more complex ByRef arguments May 2, 2019

@mrmonday

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

Just tried this on a large codebase, looks like I missed a couple of easy cases - I'll take a look.

@mrmonday mrmonday changed the title Support more complex ByRef arguments WIP: Support more complex ByRef arguments May 2, 2019

@mrmonday

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

Changing this to WIP again - there are far more places that need to be updated to handle this properly. I think it's best to introduce another visitor for this, will play around with it and see how it goes.

@GrahamTheCoder

This comment has been minimized.

Copy link
Member

commented May 2, 2019

Just in case it's relevant, one idea I was considering, is to return more information from existing visitors.

Another vague plan is to maintain a mapping of nodes for use so that in the existing second pass, the vb node and semantic model would be available alongside the first pass C# node and semantic model.

@mrmonday mrmonday changed the title WIP: Support more complex ByRef arguments Support more complex ByRef arguments May 3, 2019

@mrmonday

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

I'm pretty happy with this now.

There's one additional case which this doesn't cover, but I'd rather open a separate issue for it (like handling fields properly).

Something like:

If False AndAlso MethodWithByRef(MethodWithSideEffects()) Then
    DoSomething()
End If

Will become:

var arg = MethodWithSideEffects();
if (false && MethodWithByRef(ref arg))
{
    DoSomething();
}

Causing the side-effects to happen even when they shouldn't. Dealing with that is a bit involved, and this PR is already quite large.

@@ -68,6 +68,9 @@ private TypeConversionKind AnalyzeConversion(Microsoft.CodeAnalysis.VisualBasic.

if (csType is null || csConvertedType is null)
{
if (alwaysExplicit && vbType != vbConvertedType) {

This comment has been minimized.

Copy link
@GrahamTheCoder

GrahamTheCoder May 5, 2019

Member

We only return implicit if "alwaysExplicit" is set...sounds slightly backwards to me. I expect it's a naming thing?

This comment has been minimized.

Copy link
@mrmonday

mrmonday May 5, 2019

Author Contributor

I meant it as "always insert an explicit type conversion". I agree it's a bit confusing.

@GrahamTheCoder

This comment has been minimized.

Copy link
Member

commented May 5, 2019

Looks good. Yes the issue you mention would definitely be good to fix. especially for the case where the first condition is a null check and the second condition uses that variable which is quite a common pattern. I agree that can be a separate PR though.

@GrahamTheCoder GrahamTheCoder merged commit ab97e22 into icsharpcode:master May 5, 2019

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@mrmonday mrmonday deleted the mrmonday:264-ref-parameters branch May 5, 2019

@mrmonday

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

Opened #310 for the conditional case, and added a comment to #281 for the field case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.