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 302 forks source link

Code Inspections - Local variable is not declared + Forms #4560

Closed Gener4tor closed 5 years ago

Gener4tor commented 5 years ago

When I use a Control in a Form like this:

request.Name = PE_NAMF

the code inspection tells me:

"Local variable 'PE_NAMF' is not declared"

and that my code will not compile. When I click "fix" rubberduck will create the following line:

"Dim PE_NAMF As Variant"

which is wrong as "PE_NAMF" is the name of an edit field in the form. Im not sure what exacly does this, but I suppose it introduced a bug in my program.

It seems like rubberduck does not know the Names of the controls in the form. As a quick fix form me I can simly write:

request.Name = Me.PE_NAMF

and everything is ok. But the wrong inspection-message is misleading and the "fix" is dangerous.

P.S.: I use: Version 2.3.0.4221 OS: Microsoft Windows NT 10.0.16299.0, x64 Host Product: Microsoft Office 2016 x86 Host Version: 16.0.4711.1000 Host Executable: MSACCESS.EXE

FuSoftware commented 5 years ago

That's because without the Me., RD can hardly now if it's a mistyped variable name. RD has little to no access to what is outside the VBA.

Using the Me. 'namespace' makes it clear you're looking for a property of the current form, which it can find. Not using Me makes your intent unclear. Imagine if RD had to check if every single variable is present inside Me's scope, which means inside every Object's scope, since Me is just an Object

The only fix afaik would be to check Me's content on every VBA File (Project/Module), and test if the variable's name is inside it. Waste of parsing time, if you can fix it using Me

Gener4tor commented 5 years ago

But Rubberduck should not give me wrong information and should absolutly not put a bug in my code when I click "fix".

When rubberduck is not able to parse all the valid variables in a namespace, it should not tell me that this variable is undeclared.

FuSoftware commented 5 years ago

It did correctly parse the variable names. There is no PE_NAMF variable, but a Me.PE_NAMF variable exists. Also RD is still under heavy development, and as the License states, it's provided "AS IS" with no guarantees, so no need to be angry. The Inspection is still very inaccurate and bugs are found everyday, that's why you don't blindly Fix its warnings/errors.

Gener4tor commented 5 years ago

Im not angry. Sry, if I sounded like this.

I just wanted to help and tell you about this bug in RD.

MDoerner commented 5 years ago

The basic issue here is not the inspection or the quickfix, but the resolution of the control. That is a long standing and tough issue since the API around forms is cumbersome.

I guess, Me.PE_NAMF is still not resolved correct, but with the specifier Me, it is not treated as potential variable.

bclothier commented 5 years ago

As @MDoerner said - this is a known issue already reported in various issues: #389, #1171, #1647, #2613, #3924, and possibly more. This is all already tracked in the TypeLib API project. We already have a in-process PR #4535 that is WIP at the moment. This is foundational because we need to integrate the new TypeLib API into our parsing subsystem in a seamless manner. Once that is achieved, those issues will become much easier to resolve and will be resolved.

The primary reason why this has been a hard problem to resolve and requires the TypeLib API is because we cannot detect the document's class type from source code alone in an accurate and reliable manner. In Access, all forms and reports must start with Form_* and Report_* but other document types such as Excel's Worksheet or Workbook do not have such restrictions and in fact could be renamed to be Form_Foo if so desired. UserForm is also a problem for same reason. TypeLib API is what provide us that missing metadata; the current barrier is that we need to update our parsing subsystem to make use of it in an efficient manner. Hence, why I referred to the WIP PR.

Since we already have so many issues around this very important functionality, I'm going to close this issue as an duplicate and suggest that we track this via the TypeLib API project.

retailcoder commented 5 years ago

When rubberduck is not able to parse all the valid variables in a namespace, it should not tell me that this variable is undeclared.

Document module controls are not variables, they're objects akin to hidden module-level public fields, and they are not accessible through the VBE Extensibility API. I'm open to ideas for dismissing an inspection result when it is known that document-module controls aren't resolved, that doesn't produce false negatives for undeclared variables in the code-behind. Seems to me that knowing the picture is incomplete implies knowing what the complete picture should be - and if we knew that then there wouldn't be missing declarations in the first place.

Early 2.0 builds (and 1.x) didn't create "undeclared" variables, which made it impossible to warn about undeclared variables, which is a very common problem with VBA/VB6 code since Option Explicit is, well, optional. As a static code analysis tool for VBA/VB6, being able to identify variables that aren't declared is a fundamental building block, and false positives due to Rubberduck's current inability to locate controls in document modules were deemed (correctly, IMO) easy enough to spot and thus the advantage of being able to report undeclared variables in user code outweights the annoyance of wrongly reporting document module controls as undeclared variables.

The only way I can see document module controls not reported as such, is to revert the resolver logic that introduces an undeclared variable for any identifier it's unable to resolve in user code - short of being able to somehow identify document module controls, of course. This is simply not an acceptable solution.

The only real solution is to know what controls exist on a document module, and as @bclothier aluded to, this is something that has only recently become an available, with the contributions from @WaynePhillipsEA allowing Rubberduck to break the wall of the under-featured VBIDE API (which obviously was never intended or foreseen to support an IDE extension as deep and thorough as Rubberduck), and finally see the VBA/VB6 project contents through, essentially, the eyes of the VBE itself.

Until the TypeLib API is leveraged to that effect, requiring variable declaration (via VBE options) and ensuring Rubberduck's "Option Explicit" inspection is enabled, should be enough to dismiss "Variable not declared" inspection results - the inspection can safely be disabled if variable declaration is enforced by the IDE - but Rubberduck has no way of knowing this, and thus ships with the inspection enabled by default, again because the default VBE configuration allows for undeclared variables to exist.

Even after the typelib API is leveraged and Rubberduck correctly identifies document module controls, please keep in mind that no static code analysis tool is ever flawless. ReSharper isn't free OSS, is built by a company that makes IDEs and decompilers, sells for a good hundred bucks yearly, still has false positives as of version 10.x, and openly tells its users to always use their judgement before applying a quickfix. Holding Rubberduck to a higher standard doesn't strike me as fair at all.