Fix properties not handled correctly for ref extension methods#26
Fix properties not handled correctly for ref extension methods#26GrahamTheCoder wants to merge 5 commits intomasterfrom
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>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in the VB.NET to C# code converter where Highlights
🧠 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
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an issue with converting VB.NET extension methods with ByRef parameters when called on properties. The changes generalize ArgumentConverter.HoistByRefDeclaration to work with expressions and update NameExpressionNodeVisitor to use this for hoisting the this argument when it's a property. The approach is sound and the included unit test correctly verifies the fix for the described scenario.
I've found one issue in the new logic in NameExpressionNodeVisitor where the logic for determining if hoisting is needed is an incomplete duplication of existing logic, which could lead to incorrect code generation in some cases. I've provided a suggestion to make it more robust.
| var symbolInfo = _semanticModel.GetSymbolInfoInDocument<ISymbol>(maes.Expression); | ||
| if (symbolInfo is IPropertySymbol { ReturnsByRef: false, ReturnsByRefReadonly: false } propertySymbol) { | ||
| refConversion = propertySymbol.IsReadOnly ? SemanticModelExtensions.RefConversion.PreAssigment : SemanticModelExtensions.RefConversion.PreAndPostAssignment; | ||
| } else if (symbolInfo is IFieldSymbol { IsConst: true } or ILocalSymbol { IsConst: true }) { | ||
| refConversion = SemanticModelExtensions.RefConversion.PreAssigment; | ||
| } else if (symbolInfo is IMethodSymbol { ReturnsByRef: false, ReturnsByRefReadonly: false }) { | ||
| refConversion = SemanticModelExtensions.RefConversion.PreAssigment; | ||
| } else { | ||
| var typeInfo = _semanticModel.GetTypeInfo(maes.Expression); | ||
| bool isTypeMismatch = typeInfo.Type == null || !typeInfo.Type.Equals(typeInfo.ConvertedType, SymbolEqualityComparer.IncludeNullability); | ||
| if (isTypeMismatch) refConversion = SemanticModelExtensions.RefConversion.PreAndPostAssignment; | ||
| } |
There was a problem hiding this comment.
This logic for determining if hoisting is required seems to be a partial duplication of the logic in SemanticModelExtensions.GetRefConversion (which is a local function within NeedsVariableForArgument).
The current implementation is incomplete and may lead to incorrect code generation. Specifically, for an expression that is not a property, const, method, identifier, or member access (e.g., an arithmetic expression like a + b), refConversion will incorrectly remain Inline, and no hoisting will be performed. This would result in invalid C# code since such expressions cannot be passed by ref.
To make this more robust and align with the existing logic for arguments, I suggest restructuring this block to more closely match GetRefConversion. This will correctly handle more complex expressions by defaulting to hoisting them.
While the ideal solution would be to extract and reuse the logic from SemanticModelExtensions, the following change within this file should fix the issue:
var symbolInfo = _semanticModel.GetSymbolInfoInDocument<ISymbol>(maes.Expression);
if (symbolInfo is IPropertySymbol { ReturnsByRef: false, ReturnsByRefReadonly: false } propertySymbol) {
refConversion = propertySymbol.IsReadOnly ? SemanticModelExtensions.RefConversion.PreAssigment : SemanticModelExtensions.RefConversion.PreAndPostAssignment;
} else if (symbolInfo is IFieldSymbol { IsConst: true } or ILocalSymbol { IsConst: true }) {
refConversion = SemanticModelExtensions.RefConversion.PreAssigment;
} else if (symbolInfo is IMethodSymbol { ReturnsByRef: false, ReturnsByRefReadonly: false }) {
refConversion = SemanticModelExtensions.RefConversion.PreAssigment;
} else if (maes.Expression is VBSyntax.IdentifierNameSyntax || maes.Expression is VBSyntax.MemberAccessExpressionSyntax || _semanticModel.GetOperation(maes.Expression).IsArrayElementAccess()) {
var typeInfo = _semanticModel.GetTypeInfo(maes.Expression);
bool isTypeMismatch = typeInfo.Type == null || !typeInfo.Type.Equals(typeInfo.ConvertedType, SymbolEqualityComparer.IncludeNullability);
if (isTypeMismatch) refConversion = SemanticModelExtensions.RefConversion.PreAndPostAssignment;
} else {
refConversion = SemanticModelExtensions.RefConversion.PreAssigment;
}… 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>
1 participant
Fixes icsharpcode#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.PR created automatically by Jules for task 5310240367378084052 started by @GrahamTheCoder