Rubberduck: "Missing 'Let' keyword" inspection

[martin-east]

What

Inspector that flags lack of explicit Let or Set in an assignment

Why
Although use of Let is obsolete (which is already detected by an inspector), I always use Let or Set to make it clear in the code, especially when default members are involved, and would love it if Rubberduck could tell me where I have forgotten to do so (much as it warns me of implicit ByRef arguments)

Example
This code should trigger the inspection:

I = 1

1. QuickFix Name

   Example code, after quickfix is applied:

   Let I = 1

---

Resources

• InspectionNames: the name of the inspection, as it appears in the inspection settings.
  MissingSetOrLetStatement

• InspectionInfo: the detailed rationale for the inspection, as it appears in the inspection results toolwindow's bottom panel.
  "An assignment is made without a Let or Set statement. This defaults to Let, but you may wish to ensure that all assignments are explicit"

• InspectionResults: the resource string for an inspection result; if the string needs placeholders, identify what goes in for them (e.g. Variable {0} is not used, {0}:the name of the variable).

none required, suggest option to choose Set or Let in the Quickfix

[Vogel612]

We already have an inspection for an assignment to an Object variable without the Set keyword. To avoid duplicate results, this should only flag assignments to value types as missing the Let keyword.

The code already written for recognizing missing Set keywords should possibly be reused for this inspection.
The two inspections should be separate, though. This is necessary for the inspection system to allow ignoring this inspection when the user doesn't want to explicitly qualify all assignments.

[martin-east]

Great, thanks!

[bclothier]

I'm just wondering -

Do you prefer Let because of constructs like the following:

i = ActiveWorksheet.Cells(1, 1)

or

s = myRecordset.Fields("someColumn")

?

I would prefer that in those cases, we spell out the actual member values:

i = ActiveWorksheet.Cells(1, 1).Value

or

s = myRecordset.Fields("someColumn").Value

Which ensures that this is always a Let statement, never a Set statement. Mat wrote an article about how default member can hurt more than help. Explicitly using Let won't help with this, and I don't know about you but if I had to read this block:

Set rs = New ADODB.Recordset
Set rs.ActiveConnection = con
Let rs.CursorLocation = adUseServer
Let rs.CursorType = adOpenStatic
Let rs.LockType = adLockReadOnly

I would have to read a bit slower to mentally distinguish between Sets and Lets. Compared to the block without Let:

Set rs = New ADODB.Recordset
Set rs.ActiveConnection = con
rs.CursorLocation = adUseServer
rs.CursorType = adOpenStatic
rs.LockType = adLockReadOnly

Because the Let are no longer aligned, they look more different so it's easier to read, without extra typing. If that's not why you're preferring the explicit Let, I'd love to know why.

[retailcoder]

We've written inspections we don't personally agree with before (e.g. missing Step keyword, redundant ByRef modifier, etc.), so I don't see an inspection that flags "missing" Let keywords completely out of place. It would be disabled by default though, so as to not fix an "obsolete Let keyword" result only to get a "missing Let keyword" result, but I don't have a problem with such an inspection being implemented.

That said the Set keyword inspection has a number of false positives at the moment; I would consider getting rid of false positives with the "missing Set keyword" inspection a (much) higher priority.

[martin-east]

I agree that default members are dangerous and confusing. The main reason I use them despite that is to make debugging easier: if they return a string, that string shows when you roll-over the variable in code, or have a watch on a variable. That way you can see the value of complex objects when debugging without having to look at their individual attributes, and you can control how they are displayed in the debugger. Additionally, I have built wrapper classes for VBA simple types (e.g. Integer, Cardinal, String) to enable proper OO manipulation (e.g. String.Length or Cardinal.Increment), built-in error checking and type safe nullability, and it's useful for the default member to return the wrapped VBA type, e.g. if you want to use the VBA operators like +, - etc.

If you do use default members, then Set and Let both work and do two different things: Set X = Y assigns Y to the Object X, Let X = Y assigns a non-object value Y to the default member of X (Y could even be an object with a non-object default member). I think Set X = Y and Let X = Y are more easily distinguishable than Set X = Y and X = Y, mainly because X = Y could also be an equality test, at least visually.

I was about to submit a bug report for the Set false positives, but since that is already in hand, I don't need to. If it helps, it fails for me for:

Set .AnArrayOfObject(ALong) = AnObjectWithANonObjectDefaultMember

I'm new to Rubberduck and am very impressed by the product and the programming philosophy that underpins it. I have many, many gripes with VBA as a language, but I'm forced to use it for my work, and Rubberduck makes working with VBA that much more satisfying.

[MDoerner]

Regarding the specific issue with the test for Set, I am looking into fixing that right now. It will require tweaking our reference resolver, but than it should be easy to deal with object arrays.

[retailcoder]

"Missing 'Set' keyword" false positives are fixed now - keeping this issue open as a feature request for a "missing 'Let' keyword" inspection, but please don't hold your breath for it (or, submit a pull request for it!)