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

Don't bother reporting no Option Explicit on empty sheets #2621

Closed SystemsModelling closed 5 years ago

SystemsModelling commented 7 years ago

Option Explicit is not specified in 'Sheet1'

No code, empty sheet module

comintern commented 7 years ago

Confirmed, although oddly this only seems to hit on one module when I test it with multiple modules with no code in them. That might point to a caching issue rather than an issue with the inspection itself.

retailcoder commented 7 years ago

@comintern the inspection doesn't care if there's code in the module or not. We can fix that, but I'm not sure I'd call it a bug.

Another way to look at it, is that you could go Tools > Options > Require variable declaration:

image

The VBE will automatically add Option Explicit to every module, code or no code.

comintern commented 7 years ago

@retailcoder - The bug tag isn't really related to the inspection itself (although I'm not sure that the Option Explicit inspection should report if there's no code). I tagged it as a bug because when I was testing it at home this morning it wasn't giving consistent results when I deleted all of the code in modules that previously had code in them, then re-ran the inspections:

oddly this only seems to hit on one module when I test it with multiple modules with no code in them

That might possibly be related to #2603, but I haven't been able to replicate it on my work machine.

SystemsModelling commented 7 years ago

"The VBE will automatically add Option Explicit to every module, code or no code." AFAIK only if the code is edited, Virgin sheet modules stay empty. In fact, in other cases such one-line declarations affect other parsing tools I use.

comintern commented 7 years ago

Just replicated the inconsistent results when the code is changed, and it is a caching error. Removing the bug label in favor of handling that on the issue referenced above.

SystemsModelling commented 7 years ago

Still there.

Error: Option Explicit is not specified in 'Sheet2' - (testRubberduck.xlsm) VBAProject.Sheet2, line 1

retailcoder commented 7 years ago

Hehe... still open, too :wink:

SystemsModelling commented 5 years ago

Still there

retailcoder commented 5 years ago

@SystemsModelling right... the issue is still opened. This should be a low-hanging fruit that makes a good opportunity for a first pull request :wink:

retailcoder commented 5 years ago

AFAIK only if the code is edited, Virgin sheet modules stay empty.

That is not true. The IDE setting adds Option Explicit to any & every new module, including ThisWorkbook and Sheet1 in a brand new workbook / VBA project. The only thing it doesn't do, is add Option Explicit to existing modules.

In fact, in other cases such one-line declarations affect other parsing tools I use.

If Option Explicit is causing problems with a tool, that tool has a bug. Vanilla-VBE adds Option Explicit to empty modules with "require variable declarations" enabled.

I'm going to suggest disabling the inspection: the intent of this inspection is mainly to teach beginners (who may not know about "require variable declarations") about the module option, and why they should use it.

The inspection uses a listener that is traversing the parse tree of a module, and upon exiting the declarations section iterates the specified options and if none of them are an OptionExplicitStmt, the declarations section context is added to the _contexts list from which the inspection creates the results.

Fixing this issue would involve overriding ExitModuleBody and if the context has no children, then the declarations section context needs to be removed from the _context list.... which should probably be turned into a Dictionary<string, QualifiedContext<ParserRuleContext>> to make it easier to remove.

public class MissingOptionExplicitListener : VBAParserBaseListener, IInspectionListener
{
    private readonly List<QualifiedContext<ParserRuleContext>> _contexts = new List<QualifiedContext<ParserRuleContext>>();
    public IReadOnlyList<QualifiedContext<ParserRuleContext>> Contexts => _contexts;

    public QualifiedModuleName CurrentModuleName { get; set; }

    public void ClearContexts()
    {
        _contexts.Clear();
    }

    public override void ExitModuleDeclarations([NotNull] VBAParser.ModuleDeclarationsContext context)
    {
        var hasOptionExplicit = false;
        foreach (var element in context.moduleDeclarationsElement())
        {
            if (element.moduleOption() is VBAParser.OptionExplicitStmtContext)
            {
                hasOptionExplicit = true;
            }
        }

        if (!hasOptionExplicit)
        {
            _contexts.Add(new QualifiedContext<ParserRuleContext>(CurrentModuleName, (ParserRuleContext)context.Parent));
        }
    }
}
retailcoder commented 5 years ago

fixed in my next PR

SystemsModelling commented 5 years ago

I'd like this to be suppressed for EMPTY sheet modules, but enforced if there is any code present.

retailcoder commented 5 years ago

Agh, I had a fix for this, but ended up dropping the PR. I wonder if the commit can be cherry-picked into a new PR.