Change VB Code Generators to use entire block when reusing method syntax#55450
Change VB Code Generators to use entire block when reusing method syntax#55450Soreloser2 merged 5 commits intodotnet:mainfrom
Conversation
… tests in both languages that address this behavior.
src/Workspaces/VisualBasic/Portable/CodeGeneration/MethodGenerator.vb
Outdated
Show resolved
Hide resolved
…ator.vb Remove comment Co-authored-by: Joey Robichaud <joseph.robichaud@microsoft.com>
|
@CyrusNajmabadi @333fred Could either of you give me some input on this change, whenever you have some time? |
| Dim reusableSyntax = GetReuseableSyntaxNodeForSymbol(Of DeclarationStatementSyntax)([event], options) | ||
| If reusableSyntax IsNot Nothing Then | ||
| Return reusableSyntax | ||
| Return DirectCast(reusableSyntax.GetBlockFromBegin(), DeclarationStatementSyntax) |
There was a problem hiding this comment.
It might be a better idea to change Seems not possible since it can return GetBlockFromBegin return type to DeclarationStatementSyntax instead of doing the cast.StatementSyntax.
Wondering if we guarantee this cast is safe? Could we have different overloads of GetBlockFromBegin, one which accepts DeclarationStatementSyntax and returns DeclarationStatementSyntax, and one that takes StatementSyntax and returns StatementSyntax?
Note: I'm not sure if it's possible.
There was a problem hiding this comment.
In the GetBlockFromBegin method (linked here), we either return the current node, which is guaranteed to be a DeclarationStatementSyntax, or we return the parent, which is also a class derived from DeclarationStatementSyntax in all cases except when the parent is a VariableDeclaratorSyntax. We shouldn't ever have a VariableDeclaratorSyntax as our parent, because this only changes on the generation of methods, properties, and events, so I think this cast is safe.
I think we could change the other casts to also be DeclarationStatementSyntax, since it is derived from StatementSyntax. It should work the same, as every StatementSyntax returned from GetBlockFromBegin is also a DeclarationStatementSyntax.
We could alternatively write some overload/equivalent method of GetBlockFromBegin that removes the separate case for the parent being a VariableDeclaratorSyntax. Then, we could both require a DeclarationStatementSyntax as the input and also guarantee the return of a DeclarationStatementSyntax.
There was a problem hiding this comment.
I'll defer to @CyrusNajmabadi on what to do (if any action is required)
There was a problem hiding this comment.
yes. i think this should become strongly typed to try to return a DeclarationStatementSyntax. Try making that change and see if anything breaks with a cast-exception. if so, let us know and we can decide what to do at that point. if not, then let's definitely go that route. thanks!
…BlockReuseSyntax
…s to DeclarationStatementSyntaxes
Code Generators have an option to reuse syntax when generating code for a symbol, in which it will copy the declaring syntax reference and use the corresponding node to generate the symbol in the new location. This is used for efficient code movement, such as in Pull Member(s) Up and Extract Class, because you don't need to regenerate lines of code that are written somewhere else.
Currently, when you call
DeclaringSyntaxReferenceson a VB method-like symbol (Function, Sub, Property, Custom Event), you will get back the method statement syntax. This contains the first line of the method block, which is often what is desired for a declaration. However, when we want to reuse syntax, the method statement syntax thatDeclaringSyntaxReferencesgives us only contains the single method line, when we really want to use the entire method block, with all its inner statements and its closing statement. So, we can call theGetBlockFromBeginon the method statement, which will give us the entire method block.This change will bring the reuse syntax option behavior for VB in line with C#, as C#'s
DeclaringSyntaxReferencesreturn value (or one of them, when declared in multiple locations) actually points to a syntax node that contains the entire body, so no modification is necessary.A few tests are added to mark this behavior in VB, and are copied for C# (even though behavior hasn't changed).