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

Implicit Application reference inspection? #4158

Open retailcoder opened 6 years ago

retailcoder commented 6 years ago

We currently report this:

Cells(1, 1) = 42

As an implicit ActiveSheet reference.

This:

Set ws = Worksheets("Summary")

Is an implicit ActiveWorkbook reference.

While correct, there's another thing that's implicitly referenced in both of the above: Excel.Application

I think we need to merge them into an ImplicitApplicationReferenceInspection, with a new InspectionResults.resx resource key for the members of Global/_Application, and reword the other two a bit, and the inspection can return a unified result with a fitting description for each case.

bclothier commented 6 years ago

Why not make the inspection more generic and look for those that has the TYPEFLAG_FAPPOBJECT flag / appobject MIDL attribute set? That way it will work for any type libraries that contains such objects. The inspection would only need to exempt the 2nd reference of the VBA references (e.g. the host type library) because the first 2 references are usually required & immovable, at least that's the case for Office VBA projects.

I'm not sure about the Set ws = Worksheets("Summary") being a implicit ActiveWorkbook -- WorkSheets is a member of the Global class. Furthermore in the related case where there is an implicit reference to the class members within the class module, so it seems more likely that the implicit access would refer to the Global class in Excel's case.

retailcoder commented 6 years ago

The inspection is specifically flagging (unqualified?) member calls that resolve to Global: it's kind of based on the implementation of these member calls, which do point to the active sheet/workbook.

I do like the idea of making it host-agnostic, but I'm not sure it's feasible.

bclothier commented 6 years ago

The Global object in Excel has the appobject attribute, so its members can be marked as implicitly accessible without any need to special-case. Note Word likewise treats its Global the same way. Access and Outlook does not have it; that belongs to their respective Application object. Other VBA host project may have some entirely different object that has the flag set.

Can you demonstrate that Set ws = Worksheets(ws) in fact resolves to Excel.ActiveWorksheets, not to Excel.Global.Worksheets? I'm inclined to think it will do the latter but I cannot conclusively demonstrate that, beyond the fact that's exactly the effect of the appobject attribute is supposed to have.

PS: BTW, Rubberduck seems to think that the Worksheets resolves to Excel.Global.Worksheets in a standard module, ThisWorkBook and Sheet1 code-behinds.

retailcoder commented 6 years ago

I does resolve to Excel.Global.MemberCall. The inspection is just too naive (it warns about the implementation details, the visible behavior): if a global-scope object implements the _Application interface, then its members can be assumed to implicitly reference an Excel.Application object; we don't need to special-case the Global identifier either then.. and we just need to figure out a not-too overwhelming yet informative description/info string.

ThunderFrame commented 6 years ago

My understanding has always been that there is only one occurrence of the Application property that can be called without qualification, and that occurrence is the Application member that is elevated to <globals>, and so when you execute any of these statements, they're all calling the <globals> member as Excel.Global.Application (where the Global CoClass implements the _Global interface). Moreover, there isn't any implicit call to Application.Application.

Set app = Application                'Implicit call to Excel and Global
Set app = Excel.Application          'Implicit call to Global
Set app = Excel.[Global].Application 'The square brackets are necessary for Intellisense
Set app = Excel.Global.Application   'No Intellisense, but does compile

It then follows that calls to any property or method member of <globals> works in the same way. For example, Range is a call to Excel.Global.Range and there isn't any implicit Application involved, other than within the type library's implementation of the Excel.Global.Range property.

But as we know, calls to Excel.Range from a non-Excel host, will result in a new, unreferenced instance of Excel (and error 1004, as there isn't any active workbook in the created instance, from which to return a range), so in an Excel host, to avoid implicit references to the ActiveSheet or ActiveWorkbook, we should be qualifying calls to Range with a contextual reference to the sheet and the workbook, and in a non-Excel host, we should be qualifying calls to Range with a contextual reference to the sheet, workbook and application. That hierarchy of prerequisites would seem to be host specific, unless the resolver can deduce the relationships from the type library.

I may not be interpreting all of the IDL attributes correctly, there appears to be some special/undocumented handling of globals, and it doesn't help that that the VBE's Object Browser duplicates the Global object members in search results.