Rubberduck: Class 'ThisWorkbook' hides property get accessor 'ThisWorkbook'

[retailcoder]

I think we're going to have to ignore document module names to work around that one.

[retailcoder]

There's more to it: it seems any identifier defined in any referenced library is flagged as "shadowing" said defined identifier - and that's a false positive in every case.

Standard module:

Dim visible As Boolean

Variable 'visible' hides property get accessor 'Visible'

Of what? There's nothing in-scope by that name, the variable isn't shadowing any declaration, Visible isn't a member of any global-namespace object. Was the inspection meant to fire results for any identifier that has the same name as something else, somewhere? That's not what "shadowing" stands for in my books.

For example, if you're in a Worksheet module's code-behind, then a Visible variable is shadowing the worksheet's Visible property, which is now required to be qualified with Me in order to be accessed (except that's a compile error too - "member already exists in an object module from this this object module derives").

Hence I get a hunch that either the inspection has a bug, or its scope wasn't clearly defined and the implementation goes too far with flagging innocuous identifiers.

[rkapka]

I will take a look at this later today or tomorrow. I understand what shadowing means, so this is a bug. If Visible is a property inside a class, then it cannot be shadowed (unless you declare a variable with the same name inside that class, just like you mention in your example). There is a case for that in code and I'm surprised it doesn't work.

[rkapka]

The issue with Visible is the same as with ThisWorkbook and Assert in test modules. All original declarations that are being "shadowed" live inside ComComponent component type. I don't remember why, but I didn't take this type into consideration when developing the inspection (probably I just skipped it, because I didn't know what to do with it). The implication is that every declaration living inside such component is treated like a declaration living inside a module in referenced project.

Is it possible to refer to any variables living inside COM components? If not, then I can just treat all of them as non-shadowable.

[retailcoder]

@rkapka type libraries typically don't expose state/fields. but many expose enums and constants, e.g. the VBA standard library has a vbBlack constant in its ColorConstants module, which would be legally shadowed by a vbBlack identifier in user code.

The key is being able to tell what's living in global scope: if a class has a GlobalNamespace flag, its members don't need to be qualified to be referenced. Global enums and constants don't need to be qualified either, and all of them can legally be shadowed.

What we don't want to flag as potential issues, is instance members, even those of a class with a PredeclaredId flag, e.g. our fake DebugClass.Assert procedure declaration, the _Global.ThisWorkbook property, or Application.Visible.

[rkapka]

The key is being able to tell what's living in global scope: if a class has a GlobalNamespace flag, its members don't need to be qualified to be referenced. Global enums and constants don't need to be qualified either, and all of them can legally be shadowed.

@retailcoder Maybe I'm oversimplifying what you're saying, but isn't it enough to check whether the declaration's parent declaration is a ClassModuleDeclaration having IsGlobalClassModule set to true? If this requirement is not met, then the declaration would be deemed as non-shadowable.

[retailcoder]

@rkapka definitely look for that flag. ...seems a fair assumption to make.

[rkapka]

@retailcoder Adding this check fixed everything except ThisWorkbook. The reason is that ThisWorkbook is a property get living inside a COM component class called Global, which has the IsGlobalClassModule flag set. Actually, when typing "ThisWorkbook" inside the editor, I see two results in intellisense - one for the class inside my project and the other one for a property. The property is exposed also after changing the workbook name. I guess that's a way of accessing the current workbook, regardless of its name.

All in all, I think this means that an inspection result should be displayed in this case (even though the property probably just accesses the workbook class indirectly, resulting in a reference to the same object).

[retailcoder]

A result should not be issued for the ThiSWorkbook document module - it's there by default and while it does hide the global ThiSWorkbook property, issuing a result for it in every single Excel-VBA project makes no sense; I think we're now correctly resolving the type to Workbook, so let's explicitly exclude any document-type class module of type Workbook from inspection results, and let's verify in other Office hosts if there's similar special-casing that needs to occur.

[retailcoder]

Actually, let's just ignore document-type class modules.

[rkapka]

@retailcoder I am eventually going to conform to your ideas, but ignoring all documents means that if the user has a Foo document and a Foo procedure, he won't be able to access the document. But maybe that's an edge case... I doubt many people change the default names of documents and they certainly do not name stuff ThisWorkbook.

I'd also like to know which scenario you have in mind. The one where the document is the original declaration, where the document is the shadowing declaration, or both?

[retailcoder]

@rkapka the scenario is that in a brand new empty VBA project in Excel, with nothing but the default document modules, we're issuing a warning about the shadowing of _Global.ThisWorkbook by ThisWorkbook; I find this terribly confusing for users - especially since we do not want them to rename that document module: I totally get that it is technically shadowing it, but it's a false positive, in the sense that the Excel object model is designed that way.