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.91k stars 299 forks source link

Trigger-happy VariableNotAssignedInspection warning #6192

Open iVilius opened 8 months ago

iVilius commented 8 months ago

Rubberduck version information The info below can be copy-paste-completed from the first lines of Rubberduck's log or the About box:

Rubberduck version 2.5.9.6316
Operating System: Microsoft Windows NT 10.0.22631.0, x64
Host Product: Microsoft Office x64
Host Version: 16.0.17029.20108
Host Executable: EXCEL.EXE

Description "VariableNotAssignedInspection" is firing in a manner that is not expected. There are 2 faulty behaviours:

Example 1:

CustomClassVariable.Sub subarg1 CustomClassVariable will in this case get flagged by the mentioned Warning (VariableNotAssignedInspection) even though it is being used to execute a method by utilizing one of its functions/subs. Example 2: Dim retVal as String * 8192 Mid$(retVal, 3) = SomeVal retVal gets flagged by the same warning again even though it is being used for fast string concatenation. These bugs should be fairly easy to fix. **To Reproduce** See examples above. **Expected behavior** Expected not to see the mentioned warning. **Screenshots** If applicable, add screenshots to help explain your problem. **Logfile** Rubberduck generates extensive logging in TRACE-Level. If no log was created at `%APPDATA%\Rubberduck\Logs`, check your settings. Include this log for bug reports about the behavior of Rubberduck. **Additional context** Add any other context about the problem here.
retailcoder commented 8 months ago

These bugs should be fairly easy to fix.

Pull requests are always welcome! 😊

These are actually two rather tricky edge cases: ByRef assignment (where an unassigned variable is passed ByRef to a procedure that assigns it) is not being tracked at all, mostly for performance reasons; we err on the side of caution and warn anyway, because there's no way to tell whether passing the parameter ByRef was intended or just an omission, given the language default.

If I recall correctly, the Mid/Mid$ statement is syntactically a special case that must be parsed as a keyword rather than a function call, so the arguments don't (and shouldn't either) resolve to Mid function parameters; I'd have to double-check but IIRC we are treating this argument as we would a ByRef assignment. This one could be a case of misunderstanding exactly how the statement operates, since it's a rather rare occurrence; I agree there should be a relatively easy way for the inspection to ignore them, if indeed it's guaranteed that the given argument is assigned after the statement is executed.

ByRef assignments are not likely to be addressed in v2.x, however the plan for v3 diagnostics is to massively expand the customization and allow each individual inspection to have its own SettingsGroup, which would make it possible to configure the inspection to either pursue ByRef assignments, or to ignore them specifically, without needing to sprinkle @Ignore annotations. Many inspections have such default behaviors that would ideally be configurable, but the 2.x settings would be painful to implement.

I think the case of the Mid statement could be fixed in 2.x, but I think it's probably best to let ByRef assignments slip for now, until diagnostics can be individually configured in v3.

retailcoder commented 8 months ago

Actually I misread the first case, it's about a member call and it's the object variable being flagged, not the argument (correct?). In that case then the outcome depends on where the object variable is assigned. If it's global and assigned in another scope that just so happens to be invoked at some point before, then there is indeed no statically guaranteed way that the containing procedure will always be invoked in the correct global state and the warning is warranted.

If it's set relatively near its usage, there's no reason for it to be flagged. The provided code example does not show how the object variable is set though.