Fix 1158 ref extension property#1227
Merged
GrahamTheCoder merged 5 commits intoicsharpcode:masterfrom Mar 11, 2026
Merged
Conversation
… ref * Extract `HoistByRefDeclaration` in `ArgumentConverter` to accept `ExpressionSyntax` instead of `SimpleArgumentSyntax` so it can be reused for the `this` parameter of extension methods. * Update `NameExpressionNodeVisitor` to apply ref-conversion hoisting rules to the `this` expression of extension methods using `ref` parameters. * Add a unit test verifying that `Number.NegEx()` where `NegEx` takes `ByRef num as Integer` correctly generates a temporary variable assignment before and after the call. Co-authored-by: GrahamTheCoder <2490482+GrahamTheCoder@users.noreply.github.com>
… ref * Add `HoistByRefDeclaration` overload in `ArgumentConverter` accepting `ExpressionSyntax` instead of `SimpleArgumentSyntax` to support generic expression hoisting. * Update `NameExpressionNodeVisitor` to correctly detect when the receiver expression of an extension method needs to be passed by reference (e.g. properties, fields), and reuse the variable hoisting logic to correctly generate temporary variables for property modification before and after the method call. * Add comprehensive test `TestExtensionMethodRefPropertyAsync` to ensure the generated C# code successfully extracts the property to a temporary variable for method calls. Co-authored-by: GrahamTheCoder <2490482+GrahamTheCoder@users.noreply.github.com>
… ref * Generalize `HoistByRefDeclaration` in `ArgumentConverter` to accept `ExpressionSyntax`. * Update `NameExpressionNodeVisitor` to correctly process extension method receiver ref-conversions, hoisting the receiver expression (e.g., properties) into a temporary variable when required by `ByRef`. * Introduce `TestExtensionMethodRefPropertyAsync` to explicitly verify property ref assignments to extension methods generate a valid C# state. Co-authored-by: GrahamTheCoder <2490482+GrahamTheCoder@users.noreply.github.com>
GrahamTheCoder
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1158.
When a VB.NET extension method with a
ByRefparameter is called on a property, the C# converter previously failed to hoist the property into a temporary variable, resulting in the invalid C# codethis.Number.NegEx();(CS0206: A property or indexer may not be passed as an out or ref parameter).This PR generalizes
ArgumentConverter.HoistByRefDeclarationso it can be applied to thethisargument expression, and updatesNameExpressionNodeVisitorto correctly hoist properties passed to extension methods. It also includes the requested unit test.Jules task 5310240367378084052 started by @GrahamTheCoder
Gemini review
This pull request addresses a critical bug in the VB.NET to C# code converter where
ByRefextension methods invoked on properties would generate invalid C# code. The changes ensure that such properties are correctly hoisted into temporary variables during conversion, resolving the compilation error and improving the robustness of the converter.Highlights
HoistByRefDeclarationmethod inArgumentConverter.cshas been generalized to accept a broaderVBSyntax.ExpressionSyntaxtype, allowing for more flexible handling of expressions that need to be hoisted into temporary variables.NameExpressionNodeVisitor.csnow includes specific logic to detect when a VB.NET extension method with aByRefparameter is called on a property. It correctly hoists the property into a temporary variable to avoid C# compilation errors (CS0206).ExtensionMethodRefPropertyTests.cs, has been added to validate the fix, ensuring that VB.NET code involving properties andByRefextension methods converts correctly to C#.🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.
Changelog
HoistByRefDeclarationmethod signature to acceptVBSyntax.ExpressionSyntaxfor broader applicability.GetTypeInfoandShouldPreferExplicitTypeto use the generalizednodeparameter.ByRefparameter hoisting.RefKindandRefConversiontype.HoistByRefDeclarationmethod for proper property hoisting in extension method calls.ByRefparameters when called on properties.Activity