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.92k stars 301 forks source link

New Annotations: '@ProcedureName, '@ModuleName #5476

Open Burky opened 4 years ago

Burky commented 4 years ago

Rubberduck Opportunities

VBA's error handling support is, at least, weak. What I'm missing (as a die-hard VFP developer:-) are built-in functions like Program() and CallStack() which return the currently executing routine, its containing module and the enclosing project. The easiest approach which, IMOH, has the least impact on performance and the smallest memory footprint, is to introduce one module-global, and for every routine, one local constant to provide that information. Well, that's no big deal, but introduces new maintenance challenges: subtle, and hard to find bugs that will write misleading information into our error logs, if we rename some of the above modules without syncing the corresponding constant assignments, for example

Therefore, it would be great if we could use annotations to decorate those constant assignments and let Rubberduck check if the assigned values are still in sync with the module/method names they represent.

VBA Runtime Information!

Class Names This.Project = TypeName(me) is a reliable way to get form & class names, but cannot be used to retrieve code module names.

Method Names There is no VBA-native way to find out the name of the function/sub/property the currently executing code is in without adding some hinting, right?

Project Names To get the right project name is easy. Application.VBE.ActiveVBProject.Name always returns the name of the project where this code is in. IMHO, it would have been a better choice to name ActiveVBProject: ThisVBProject (to be congruent with Excel.Application.ThisWorkbook). So, there is no real need to introduce another constant for VBProject names in code.

Examples

Let's assume that the class clsGenericProxy was renamed to clsGlobalProxy. Then, this code should trigger the '@ModuleName inspection:

'@ModuleName : Don't forget to change the class name here!
Private Const pcCLASS_NAME As String = "clsGenericProxy"

' It shouldn't matter if the LHS is a constant or a variable:
'@ModuleName : Don't forget to change the class name here!
Private pcCLASS_NAME As String = "clsGenericProxy"

' It shouldn't even matter what's the name/visibility of the constant or variable:
'@ModuleName : Don't forget to change the class name here!
Public cWhatTheHeck As String = "clsGenericProxy"

This code should trigger the '@ProcedureName inspection:

' I renamed the Sub but forgot to sync the RHS of the constant assignment
Public Sub DoSomethingElse()
    '@ProcedureName: Don't forget to change the Sub name below, bummer!
    Const pcPROCEDURE As String = "DoSomething" ' the old (guarded) Sub name 
    '...
End Sub

QuickFixes

There should only be one quick-fix in both cases: Sync RHS with Module Name and Sync RHS with Procedure Name Even an ignore once makes no sense to me! In both cases, RubberDuck found a very nasty to debug logic error. I even would prefer a global Auto-Fix setting to allow RD to fix those mistakes automatically w/o even query me!

Last but not least, the code name of the method/module is significant. The quick-fix always alters only the RHS string expression to match the method/module name!

  1. SyncModuleName Example code, after quick-fix is applied:
    '@ModuleName : Don't forget to change the class name here!
    Private Const pcCLASS_NAME As String = "clsGlobalProxy"
  2. SyncRoutineName Example code, after quick-fix is applied:
    Public Sub DoSomethingElse()
    '@ProcedureName: Don't forget to change the Sub name below, bummer!
    Const pcPROCEDURE As String = "DoSomethingElse" ' the new Sub name 
    '...
    End Sub

Resources

Each inspection needs a number of resource strings: Well, we'll see later...

retailcoder commented 4 years ago

That can be done, though I'm not too crazy about the Hungarian prefixing. Needs some setting to configure/reserve an identifier name for the generated locals and module variables, with a sensible default.

TBH code-gen has been thus far deemed too invasive, but I can see how Rubberduck could be leveraged to keep such metadata in sync. Short of using vbWatchDog and actually accessing the call stack programmatically, it's probably the best way to get something like a stack trace in VBA.

Personally I'm reluctant to resort to this kind of code, and have never seen a VBA dev go through logs to debug their code, so while stack traces are invaluable when you need to analyze an error after the fact, my experience has been that VBA devs tend to reproduce the bug and debug in-place: in such a situation a stack trace is nice, but not needed at all - using breakpoints, Resume statements, and the debugger tools available, I've never felt the need to go nuts about the "source" parameter of Err.Raise, in 20 years (once I made a Stack data structure specifically for that, and it was dumb). With code that adheres to best practices, and with unit tests, I've found that kind of labelling is even more irrelevant: I'm genuinely torn about whether implementing this feature is a bad "good idea".

IMHO, it would have been a better choice to name ActiveVBProject: ThisVBProject (to be congruent with Excel.Application.ThisWorkbook)

The VBIDE API has nothing to do with the Excel object model though, or with the object model of any of the 200+ VBA hosts out there: it's more about the VBA SDK and the VBE's own object model than the type library of any host.

Burky commented 4 years ago

@retailcoder

The VBIDE API has nothing to do with the Excel object model, though, or with the object model of any of the 200+ VBA hosts out there: it's more about the VBA SDK and the VBE's own object model than the type library of any host.

Good Point, You win! :-) You're absolutely right! Very often I forget, that VBA only is a foreign object inside the M$Office "organisms" ;-)

TBH code-gen has been thus far deemed too invasive

Sorry for my ignorance, but even Google couldn't tell me what that "TBH code-gen" is.

I'm genuinely torn about whether implementing this feature is a bad "good idea".

Yeah, again, You're right... seen from your perspective. Any single developer sitting in front of her computer will take the approach you've described so well above. And, without any doubt, today, there are really elaborate ways to UnitTest our components to firefight any (even unforeseen) malfunction before they will rollout.

Now, let's assume you have created some wonderfully useful piece of software that sells like hotcakes all over the world. Of course, you have implemented the best error management system one can think of, combined with a cool email-based error reporting system. All of your many thousands of Unit tests passed green and you're good to go flying to Miami, counting your money in the sun. What do you think has happened, when you return from your holidays? Let me tell you: The inbox of your special error reporting email account holds over 300.000 Emails! Now, it's your turn! What I want to stress, painting this nonnatural colored picture, is: Naturally, no human being is interested in that additional information, I was talking about in my initial proposal! Of course, this will be a computer program that needs those info to categorize, merge, and prioritize the overflowing disaster-reports. Most of them may be harmless, but would you (personally) want to decide that for every single error-mail you receive? Believe me (or not:-), all of your Unit-Testing, Regression-Testing, Integration-Testing, Error-Testing, Intrusion-Testing, and Interface-Testing will not prevent you from Errors to occur - they will do! Because that's the nature of the WINOS-based ecosystem: There are too many different hard/software configurations out there in the wild. You cannot take them all into account. Enough on that.

A typical error manager (for shrink-wrapped software) should at least deliver:

Then a system can distribute the incoming errors either to the helpdesk, or the devs, or a sink :-) In such an automated error handling system, error emails containing wrong meta-data can cause a lot of confusion (at least), right? Well, my proposal would not really improve RD. It's only a tiny lego-block that only a few players will be missing. Thus, I'm absolutely okay with, you working on more important proposals first.

bclothier commented 4 years ago

But, @Burky this already exists, as Mat mentioned in his first post. Look at vbWatchDog. If you're selling a commercial software based on VBA, then paying $220 USD for its license (assuming single developer seat) should be a quite affordable proposition and would solve the problem much better than any code-gen we could come up with.

Code gen => code generation. A file that's code-gen simply means that there's some software that writes code and replaces the entire content of the file.

Burky commented 4 years ago

@bclothier Hi, and many thanks for replying. I do know vbWatchDog and I have a license, too. But now, I found this cool piece of software on the internet (https://vbacompiler.com/best-way-protect-vba-code/) but I'm afraid, those two will not work together. I'm still waiting for a response from the VbaCompiler team. In between, I'm reasoning about which of them I will drop: best copy-protection against best error-handling. Well, error handling can be implemented almost to the same level of vbWatchDog using plain vanilla VBA. I will wait until things get clearer and then I'll make my decision.

Code gen => code generation. A file that's code-gen simply means that there's some software that writes code and replaces the entire content of the file.

You are a decent guy! Well, my fault - should have asked: "What heck is the TBH in TBH code-gen" :-) No offense!

retailcoder commented 4 years ago

To clarify: I can absolutely see how this would be valuable in some specific scenarios.

I can also see how it could be detrimental to overall code quality by encouraging our users to implement cargo-cult style error handling, i.e. "a handler for every scope" the way MZ-Tools users do (it has a feature to insert "error handler boilerplate"), with every single procedure annotated - something we've been actively avoiding since forever. Not saying that's what the intent is behind the request, just thinking of how it could be misused & abused, and coldly balancing that against the benefits of the actual purpose.

I'd much rather develop tooling to analyze whether the error-handling code is allowing a handler to run in a non-error context, or whether handler subroutines are being jumped out and allow non-error code paths to execute in an error context: in other words, teach how to do things properly and understand how error handling works, rather than proclaim a blanket one-size-fits-all "silver bullet" solution: there is no programmatic access to the call stack in VBA, and using code / generating code to work around that, is always going to be clunky at best - if you're writing VBA code for a living, use vbWatchDog for your error handling, and problem solved, gracefully and more powerfully than anything else available. Yes, it's commercial, no it's not free, or open-source - but if VBA is your bread & butter, paying for the wizard-level work involved in this, should be a no-brainer. And if you don't need vbWatchDog, then you very likely don't need any kind of elaborate error-handling tooling, either. Hence the reluctance. Full disclaimer: the author of vbWatchDog has contributed several pieces of extremely low-level code and pointer magic that... allowed unit testing to work in every host, among other things. What vbWatchDog does simply cannot be done by anything else - short of Microsoft itself releasing something like it.

That said if Rubberduck gets a PR that implements the feature and passes code review, I'm not against merging it at all... But for me personally to implement that feature, is a stretch given the many other things that need attention, let alone how I think maybe perhaps users might possibly abuse it.

Free, open-source software is provided as-is, in the hope it will be useful. Feature requests are welcome, but as you can see there are well over 800 open issues here, the overwhelming majority of which are exactly that: feature ideas. The fastest way to get a feature you really really want implemented, is always going to be to fork the project, implement it yourself, and PR it in (even as a WIP/work-in-progress). We're happy to answer any questions, too!

retailcoder commented 4 years ago

TBH -> "to be honest"

Burky commented 4 years ago

@retailcoder Ah, never have seen that ABBRV. ;-) Vielen lieben Dank from Germany!

Burky commented 4 years ago

First of all, let me tell you, how much I appreciate the deal of time and effort you've invested in answering my initial proposal and all my follow-up annotations! Finally, I agree 100% to what you've said: My proposal only was, still is, and will be only one new "feature idea" amongst many others that are already there. The only two VBA Tools I use are MZ Tools and RubberDuck. But, honestly, I never used MZ-Tools' error handler, after I studied its implementation. For me, brainless cargo-cult programming (https://blog.ndepend.com/cargo-cult-programming/) never was an issue. I never wrote a single line of code just because, because just I always have been the laziest programmer under the sun. Okay, not a lazy programmer, but the laziest code writer: Better think about twice, then type only once, but never do copy!

(even as a WIP/work-in-progress)

AH! THX 4 the WIP