Rubberduck: "Remove unused declaration" quickfix should remove assignment references too

[retailcoder]

The "variable is not used" inspection result is (correctly) triggered when a variable is never referenced, regardless of whether it's assigned or not (a separate inspection checks that).

This means such code (correctly) triggers the inspection:

vb Sub DoSomething() Dim foo foo = 42 End Sub

The problem is that the "remove unused declaration" quickfix for it will remove the declaration, but leave the assignment, turning the code into this:

vb Sub DoSomething() foo = 42 End Sub

Which is obviously broken.

[retailcoder]

The inspection result needs a CanRemove flag that determines whether or not the quick-fix can be applied. If the variable has any references, we can't automatically remove the declaration. This lets the user acknowledge the fact that the variable isn't referred to, without introducing the possibility to accidentally remove side-effecting code.

[comintern]

Wouldn't it always be safe to remove the assignment if the RHS doesn't contain a reference type or procedure call? Constants and literals would always be safe to remove, as would assignments from anything other than a Variant or reference type.

[retailcoder]

@comintern you're right. I'm just being a bit lazy :smile:

[Vogel612]

@retailcoder FWIW we could even go so far as to only remove the LHS and the = for any assignments

[daFreeMan]

@Vogel612

Wouldn't that leave you with

Sub DoSomething()
    42
End Sub

Which may be the answer to life, the universe and everything, but isn't really all that useful?

[bclothier]

I would really not want to do that. This feel like actually changing the intent of the source code, and it might have unwanted side-effect. I'd prefer to blow the whole line away, or leave up to user to fix it.

[MDoerner]

As mentioned in the dupe above, the most sensible solution is probably to deactivate the quickfix in case there are assignments.

[comintern]

Shouldn't any code that triggers inspection when there's an assignment also trigger an AssignmentNotUsedInspection result? Given the new inspection, I'd propose not even reporting the result for the VariableNotUsedInspection. Technically it is "used', just in a way that would cause another inspection result.

[retailcoder]

Given the new inspection, I'd propose not even reporting the result for the VariableNotUsedInspection

I agree. No result, no quickfix, no problem! 💯