Don't offer 'convert if to conditional' if it crosses a preprocessor directive. #36124
Conversation
| } | ||
|
|
||
| public static bool SpansPreprocessorDirective(this IEnumerable<SyntaxToken> tokens) | ||
| { |
There was a problem hiding this comment.
this moved to AbstractSyntaxFactsService so it could be shared between C# and VB instead of having it be duplicated.
| } | ||
|
|
||
| public bool SpansPreprocessorDirective(IEnumerable<SyntaxNode> nodes) | ||
| => nodes.SpansPreprocessorDirective(); |
There was a problem hiding this comment.
this moved up to the base class.
| Dim tokens = list.SelectMany(Function(n) n.DescendantTokens()) | ||
|
|
||
| Return tokens.SpansPreprocessorDirective() | ||
| Return VisualBasicSyntaxFactsService.Instance.SpansPreprocessorDirective(list) |
There was a problem hiding this comment.
can now use common impl.
| Next token | ||
|
|
||
| Return False | ||
| Return VisualBasicSyntaxFactsService.Instance.SpansPreprocessorDirective(tokens) |
There was a problem hiding this comment.
can now use common impl
|
|
||
| Public Shadows Function SpansPreprocessorDirective(tokens As IEnumerable(Of SyntaxToken)) As Boolean | ||
| Return MyBase.SpansPreprocessorDirective(tokens) | ||
| End Function |
There was a problem hiding this comment.
need these because VB doesn't automatically use base-class methods as 'interface impls'. So we just need the method here to satisfy the compiler, but we delegate to base to actually have the logic.
|
Tagging @jinujoseph @mavasani @dotnet/roslyn-ide . Easy bugfix PR. Can someone PTAL. Thanks! |
| function M() as integer | ||
| dim check as boolean = true | ||
| #if true | ||
| [||]if check |
There was a problem hiding this comment.
Is this valid syntax for VB if statement?
There was a problem hiding this comment.
what part? not having parens? If so, yes this is valid VB :)
There was a problem hiding this comment.
I meant:
If check Then
Return 3
End If
There was a problem hiding this comment.
ah. the then is optional in VB. Automatic code cleanup adds it. but it's not necessary.
There was a problem hiding this comment.
But you would still need the End If?
There was a problem hiding this comment.
smacks-head. sorry. i get it now. will add hte other test as well :)
mavasani
left a comment
mavasani
left a comment
There was a problem hiding this comment.
LGTM, we may want to fix unintentional syntax errors in the added VB test.
|
I know comments like this spam up the thread, but thanks for the extremely quick reaction and fix. :) |
You're very welcome. Thank you for the great bug report. |
4 participants
Fixes #36117
The original code actually had a preprocessor-directive check. But the check was a little too narrow. This PR expands the check to handle the broader case. I also used this as an opportunity to unify a bunch of duplicate C#/VB code that was written prior to the great syntax unification.