Rubberduck: Inspection: Member Range implicitly references ActiveSheet - false positive with fully qualified arguments

[kolci]

The Range(ws1.Cells(1,1), ws1.Cells(5,5)) is a Range that is explicit. No matter what the ActiveSheet is, this statement will only return Rangefrom sheet ws1.
There is no need to write the statement like this: ws1.Range(ws1.Cells(1,1), ws1.Cells(5,5)).
Is it possible to add this check to the inspection: If the arguments of the Rangeare fully qualified -> ignore.

[retailcoder]

ActiveSheet.Range(Sheet1.Cells(1,1), Sheet1.Cells(5,5)) blows up if ActiveSheet isn't Sheet1, which indeed makes the inspection's quick-fix introduce a bug in this situation.

In other words, we haven't completely decyphered the full implications of [_Global].Range calls, and you are correct that qualifying that Range call would be redundant (that would even be inspection-worthy in its own right IMO).

However, one problem is that it depends where that code is written. If we're looking at a standard procedural module, then indeed, we're looking at [_Global].Range and all is good under blue skies. If the same identical code is copied into a worksheet module that isn't Sheet1, then we're looking at ThatSheet.Range, and with arguments referring to Sheet1, we can't just ignore that bug and let it slip if we can help it.

This needs to be taken into account when #3569 gets worked on and the inspection starts looking at the type of module it's running in. Until the inspection takes that into account, I don't think it's a good idea to blindly ignore these [would-be redundant, indeed] missing qualifiers. A redundant qualifier isn't wrong - but a missing one can be.

But there's more to it: it's not the inspection's job to determine that an unqualified Range call in a worksheet module is a member call on that worksheet - that's what the resolver does. Right now and until we leverage the TypeLib API implementation to tell the resolver "hey guess what, that Sheet1 component is a Worksheet module!", the resolver simply isn't able to see an unqualified Worksheet.Range call in a worksheet module as such, and so fixing the resolver will "automagically" fix the inspection.

So what I would recommend here, is to work on the resolver first, to get the inspection to no longer see every single unqualified Range call as a member call on [_Global] - this would still fire an inspection result for the case you're presenting though. So next step would be to fine-tune the inspection and address that specific case... possibly together with a new inspection that flags redundant qualifiers.

Thoughts?

Let's keep in mind that implicit ActiveSheet member calls through unqualified Range usage is responsible for a lot of duplicate questions on Stack Overflow; this inspection aims to spare Rubberduck users from ever experiencing this problem.

[comintern]

My instinct is to decline this. The inspection result is correct (#3569 aside), and the following code is an error 1004 when run from the code-behind in any Worksheet module other than Sheet1:

Sub Errors()
    Dim foo As Range
    Set foo = Range(Sheet1.Cells(1, 1), Sheet1.Cells(1, 2))
End Sub

I'd err on the side of explicit references everywhere, as it makes the intent of the code clear.

As an aside, IMHO you should really never write a line of code like that in the first place, because the 2 arguments have to match the reference that Range resolves to, and they have to match each other. That should almost always be written as:

With Sheet1
    Set foo = .Range(.Cells(1, 1), .Cells(1, 2))
End With

That ^^^ should be the recommendation if we're going to spend the effort unraveling the member that the Range arguments are being accessed on.

[bclothier]

Coming from a POV of an Access developer, I'm surprised that nobody has mentioned using Me in the cases where one intends to write code that should operate on the sheet's code-behind.

In Access, it is legal to do either:

MyControl.Value = "foo"

or:

Me.MyControl.Value = "foo"

However, I think it's safe to say that majority of Access developers would prefer the latter form over the former because it's now explicit that this is a member access to a property of the current class, and it should operate only on that member. When you consider the fact that it's legal to override already declared identifiers in VBA, omitting the Me leaves far too much to unwanted interpretations.

For those reasons, I'd much rather write code like this:

Me.Range("A1").Value = Sheet1.Range("B2").Value

which removes any doubts about what should be in scope. Seeing only a Range on a sheet's code-behind makes me feel a bit uneasy, just as when I see a MyControl on a form's code-behind.

[comintern]

@bclothier I'd agree that Me is also preferable - that was the reference to #3569.