Rubberduck: Selection for evil code

[Hosch250]

Take a look at this stupid code:

 _
 _
 Function _
 _
 Foo _
 _
 ( _
 _
 fizz _
 _
 ) As Boolean

 End _
 _
 Function

Our selection range is {L5C2 - L13C22}. The start is the Foo. The end would be if the End Function was on one line with all those extra characters in the count, or so... In fact, this code breaks almost all of our code inspections and refactorings...

[yeshua-watson-rheem]

Why not just be destructive and remove the line breaks on the keywords to make them one line again. If you're using the refactoring, I don't think you're really concerned that things are going to stay exactly the same considering most of the functions in the refactoring modify the code to some degree.

_
_
Function Foo ( _
_
fizz _
_
) As Boolean

End Function

[Vogel612]

@yeshua-watson-rheem the problem here is that code-inspections, refactorings and basically everything in the codebase rely on correct selection ranges to work.
In addition to that Rubberduck is intended to parse everything that the VBE parses. nothing more, nothing less. This also entails crazy things like the code above. That's just a very condensed example of multiple edge-cases that need to be handled correctly.

[ThunderFrame]

also, it's a fine line between _line-continuations for readability_, and _line-continuations for E-V-I-L_.

[yeshua-watson-rheem]

I always assumed the VBE didn't parse but instead does what most compilers do and strip out all the unnecessary fluff that us warm squishy things need for readability or E-V-I-L. But I get your point as the edge case I could see is

Function Foo ( _
par1 as int, _
par2 as float, _
par3 as bool) As Boolean

'More fluff

End Function

I've actually seen above in the wild before in numerous languages.

[comintern]

Getting a valid Selection for this code is actually _really_ easy. There are really 2 issues at play here though.

The first is that ParserRuleContextExtensions.GetSelection is making some really big assumptions about the ParserRuleContext that it's being passed. In the case of the code above, that it's on one line, but more generally that context.Stop is what the caller is interested in. For example, a FunctionStmtContext includes everything through EndOfStatementContext, so determining the selection for _only_ the procedure declaration line is left to the caller.

This leads to the second issue, which is purely tech debt. The fact that GetSelection is far too general means that adjustments and\or string parsings not tied to the parser context are being performed all over the code base. These should be consolidated into a set of common extensions to the extent possible.

[Hosch250]

This doesn't seem to be much of an issue anymore. The inspections select the right range, and the refactorings, which now use the rewriter as of #2928, all work.