rubberduck-vba / Rubberduck

Every programmer needs a rubberduck. COM add-in for the VBA & VB6 IDE (VBE).
https://rubberduckvba.com
GNU General Public License v3.0
1.91k stars 299 forks source link

Empty UDT causes Parse Error #3792

Closed Stevefb closed 6 years ago

Stevefb commented 6 years ago

Using ver: 2.1.2.2886 - in an otherwise empty project, add Class1 and an empty UDT:

'@Folder("Project")
Option Explicit

' Class1

Private Type someType

End Type

This results in a Parse Error

retailcoder commented 6 years ago

IIRC an empty UDT is also a compile error, isn't it?

Stevefb commented 6 years ago

Yes, just tested.

Stevefb commented 6 years ago

FYI: The reason I ended up with an empty type is because just after creating the Type, I wanted to check what I had in some other classes so I could get everything 'just so'. I'm guessing that a more seasoned programmer would be more confident and capable, but I needed to check stuff.

Navigating away from the class with the empty type I noticed something that needed fixing (easily distracted much) and so I fixed it. Having been caught out before by Rubberduck wiping changes, e.g. some (all?) refactor operations can wipe changes if the project is not refreshed first..., I've gotten into the habit of refreshing after making changes - but my empty Type was still there.

TL;DR - wasn't ready to Debug > Compile

WaynePhillipsEA commented 6 years ago

There is now a 'Compile code before parsing' option in RD settings. Turn it on and then restart your host (for some reason it seems ineffective until restarting the host at the moment).

Once you've done that, you should get a warning that the code doesn't compile before attempting the parse.

jhiston commented 3 years ago

Recognize this is a closed issue, but just to offer a use case where this creates a speedbump in a workflow.

Add a new class in which you will implement an interface.

Type in the "Implements I_Something" line

Start 'templating' by sticking in an empty "Private type tThis (blank line) End Type and private This as tThis" in preparation for storing any local variables to back the interface.

Go to the implements line and right click to bring up RubberDuck context menu and refactor and oh... implements interface is greyed out... ah remember, need to do a parse first..... and then get the parse error (and then hopefully remember that this is a simple empty UDT issue and not something broken where you were just working and decided needed the new class and go down that rabbit hole for a while....)

Easy workaround is sticking 'dummy as integer' to make the UDT not empty and then parse and then go back to the implements key word... and now the refactoring is enabled and works.

Not a huge deal, just a use-case where a parsing error doesn't seem the right flag/feedback for user - seems like it should be a compile error, but shouldn't affect RDs ability to parse the code.

One possible enhancement suggestion would be modifying the context string that gets reported in the parser errors - with experience 'mismatched input 'end type' expecting... makes sense, but I could see a naive user being better served with something hinting that its due to an empty UDT. Maybe "mismatched input 'end type'. Commonly due to empty type declaration. Was expecting.... "

Even better as an enhancement suggestion would be doing an implements interface refactoring building in the 'private type tThis....' with prepopulated backing entries for anything in the interface that has a get/set/let appropriate pairing... (think I saw a hint of that coming in the RD blog?)

Thanks for all the work on the tool.

(comments made based on using RD: Version 2.5.1.5557 OS: Microsoft Windows NT 10.0.19042.0, x64 Host Product: Microsoft Office x64 Host Version: 16.0.13801.20360 Host Executable: MSACCESS.EXE )

bclothier commented 3 years ago

FWIW, an empty UDT is a compile error and to parse, we'd have to be able to compile. The implement refactor is a exception due to the fact that in middle of implementing, the state may be temporarily uncompilable due to requirements RE: implementing all the members. Still, it's best to be able to compile the code before executing any refactoring, including implementing an interface.

retailcoder commented 3 years ago

@jhiston making parser errors that make sense without prior knowledge of how the parser works is very much non-trivial, and currently not something that's handled in Rubberduck's own code (it's ANTLR's default parser errors). I 100% agree the dream thing would be for the parser to produce meaningful error messages closer to compile errors, but for example "mismatched input 'x', expected [huge list of possible tokens]" is basically a generic parser error that isn't context-sensitive at all.

An empty UDT is a compile error and normally a compile error that involves legal syntax can still be parsed, but the grammar definition doesn't allow it:

udtDeclaration : (visibility whiteSpace)? TYPE whiteSpace untypedIdentifier endOfStatement udtMemberList END_TYPE;

We could parse an empty udtDeclaration statement by changing its definition as follows (making udtMemberList optional):

udtDeclaration : (visibility whiteSpace)? TYPE whiteSpace untypedIdentifier endOfStatement udtMemberList? END_TYPE;

I don't think parsing an empty UDT would cause any problems with the resolver, there'd just be no members to iterate through for that UDT declaration.

That said, v2.5.1.5557 would be the [main]/[master] build; you'll want to try the [next] build to get the implement interface refactoring enhancements that will be shipping with the next release :)

jhiston commented 3 years ago

Thanks both - I appreciate the followup and understand (mostly) on what makes it non-trivial under the hood.

@retailcoder re: [next] .... such a tease ;) ....

Vogel612 commented 3 years ago

For the record: ANTLR does have support for recovering from parser errors. There have been multiple (more or less abandoned) attempts at enabling that behaviour in Rubberduck to allow for partial functionality and better parser errors.

As it is, supporting such a behaviour would radically change the assumptions that the resolver can make about the parse trees it works on, though. So TBQH unless extensive work is done on both the parser and the resolver (basically nothing short of a rewrite, probably) I don't foresee anything like this happening in RD :cry: