rubberduck-vba/rubberduck

Every programmer needs a rubberduck. COM add-in for the VBA & VB6 IDE (VBE).

rubberduck-vba

Github-Pages rendering at rubberduck-vba.github.io

rubberduck's Issues

Toggle DefaultInstanceId attribute

Provide a way to toggle the value of a class module's default instance ID, effectively toggling between "normal" and "static" class behavior.

Warn user when toggling the attribute to False, because such a change is a breaking change.

Somewhat related to #48 and #49, to import/export code files - although this specific feature can work off temporary files, import/export functionality should be reused here.

Explicit type conversion is redundant

[Suggestion]

Warn when an identifier of a given type is being explicitly converted to that same type. Conversion can be removed.

Dim a As Integer, b As Integer
a = CInt(b)

Split declaration and assignment

[Suggestion]

Rubberduck should recommend splitting declaration and assignment.

Dim foo = New Bar

Can be written as:

Dim foo
foo = New Bar

TestExplorer window is visible by default

Steps to reproduce:

• Open up Excel, Alt+F11 to bring up VBA; TestExplorer dockable window is shown in the last state and position it was last time you closed the IDE. Great.
• Close the immediate pane if it's visible.
• Close the TestExplorer window.
• Close the IDE and exit Excel.
• Reopen Excel.
• Bring up the VBA editor: the immediate pane is gone, but the TestExplorer persists and is still visible.
• Maybe user settings aren't correctly saved. Resize the TestExplorer window, then close it.
• Close and reopen Excel.
• Bring up the VBA editor: the TestExplorer window is correctly sized the way you had it, but it's still visible although user settings surely say it should be hidden.

Single-letter identifier should have a meaningful name

[Suggestion]

Consider renaming single-letter identifiers. Rubberduck could inspect context to exclude i, j and k when used in a For loop, or to suggest a better name (tricky?)

GitHub integration

A "Commit" feature will export all code files to a local repository (see #48), and a "Push" feature will send these files to a remote one, all through through command-line API calls.

A "Sync" feature will download source code from the remote repository, update the local ones, and import the modified files back into the VBA Project.

Nodes should retain position in code file

This will enable navigating code file from a tree representation, and will help analyse indentation.

Move UI elements into a UI namespace.

Revert condition to reduce nesting

[Suggestion]

When a procedure's body is wrapped with an If statement, Rubberduck should recommend reverting the condition to reduce nesting.

Public Sub Foo()
    If Bar Then
        'body
    End If
End Sub

Can be written as:

Public Sub Foo()
    If Not Bar Then
        Exit Sub
    End If
    'body
End Sub

Toggle Default Member Attribute

Related: #52 (same prerequesites about importing/exporting code files)

This feature allows marking a class member as its "default member"; the feature should ensure there's only ever a single member marked as such.

Warn user when changing a class' default member, as this is a breaking change.

Use Option Explicit

[Warning]

Code modules should always have Option Explicit somewhere in their declarations section.

ProcedureNode

There might be an issue with the ProcedureSyntax regex; only properties get picked up.

Join declaration and assignment

[Suggestion]

When a New reference is being assigned only once, recommend joining declaration and assignment:

Dim foo As Bar
Set foo = New Bar

Can be written as:

Dim foo As New Bar

Note: assignment only needs to be in the same scope - not necessarily in the instruction that immediately follows the declaration.

Related: #17 Split declaration and assignment

Code inspections should be configurable

There should be a pane in the options dialog to let user configure code inspections.

• Toggle inspection on/off
• Show as:
  • Suggestion
  • Warning

Succeeded/Failed/Inconclusive counter labels in TestExplorer toolbar aren't always accurate

When test methods are manually added to a test module, they are correctly discovered and their results are correctly reported, but the counter labels only seem to account for test methods that were added using the TestExplorer "Add" menu.

Iterator getter attribute

Allows user to mark a class' NewEnum member {Property Get) as being an iterator, which allows using the class in a For Each loop construct.

Warn user when toggling this off, as it is a breaking change.

Multiple conditionals operating on the same variable can be converted to Select Case block

[Suggestion]

Rubberduck should be able to determine whether a If..ElseIf block is checking the same variable against different values, and recommend converting the entire block into a Select Case block.

If foo = 123 Then
    '...
Else If foo = 456 Then
    '...
Else If foo = 789 Then
    '...
End If

Can be converted to:

Select Case foo
    Case 123:
        '...
    Case 456:
        '...
    Case 789:
        '...
End Select

Variant type may not be intentional

[Warning]

In multiple declarations, a common mistake is to only declare the type for the last identifier:

Dim foo, bar As String

This declares foo as a Variant. Recommend explicit typing:

Dim foo As Variant, bar As String

Separate multiple declarations into multiple instructions

[Suggestion]

This:

Dim foo As String, bar As Integer, baz As Long

Can be written as:

Dim foo As String
Dim bar As Integer
Dim baz As Long

Option Explicit should be specified first

[Suggestion]

When multiple options are specified, Option Explicit should be first.

Option Base 1
Option Explicit

Can be written as:

Option Explicit
Option Base 1

Indentation is not accounted for

Instruction.StartColumn should account for indentation and return the position of the first non-whitespace character in the instruction.

If block body can be on separate line

[Suggestion]

This:

If condition Then instruction

Can be written as:

If condition Then
    instruction
End If

RubberDuck Settings dialog

Add a form to allow users to configure the add-in. Form should use a tab control to allow for future expansion of settings. Begin with a UI for ToDo list settings.

Change internal accessibility to public, and use ComVisible(false) attributes to control COM visibility

Per #38

Parameter is passed ByRef implicitly

[Suggestion]

If parameter is assigned in the body of the procedure, Rubberduck should suggest passing ByRef explicitly; if parameter is not assigned, Rubberduck should suggest passing it ByVal.

Assert.AreEqual fails when comparing a constant and a function result

Module 1

Option Explicit

Public Function Add(a As Long, b As Long) As Long
    Add = a + b
End Function

TestModule1

'@TestModule
Option Explicit
Private Assert As New RetailCoderVBE.AssertClass

'@TestMethod
Public Sub OnePlusOneEqualsTwo()
    Dim actual As Long, expected As Long
    expected = 2
    actual = Add(1, 1)

    Assert.AreEqual expected, actual

    Assert.AreEqual expected, Module1.Add(1, 1)

    Assert.AreEqual expected, Add(1, 1)

    Assert.AreEqual 2, 2
End Sub

'@TestMethod
Public Sub Bug()

    Assert.AreEqual 2, Add(1, 1)

End Sub

Option Base 1 is being used.

[Warning]

Procedure description attribute

This feature allows the user to enter a short description for a module member, to appear in the object browser when it is selected.

related: #48 & #49 (same prerequesites, although like #52 it can work off temporary files)

Configure project location

In order to support source control integration, Rubberduck will need to export/import all code files to/from a given folder. Making this folder the workbook's location may not always be possible; first thing to do is to determine where the local repository is going to be located; we need a feature that will enable the user to specify that.

Make ToDo List comments configurable

Using app.Config custom section and new class to read the config file, allow users to add attributes to be picked up by the ToDo List. Priority level of attribute should also be configurable.

Should internal classes be made public with ComVisible(false) attribute?

Writing unit tests will be easier with public classes.

Import code files from local repository

related: #48 - Rubberduck will need to be able to synchronize changes by finding all code files in the local repository, removing all project components with the same name, and importing the code files back into the VBA project.

Condition can be a single instruction

[Suggestion]

When an If block contains only a single instruction, recommend making it a single instruction:

If condition Then
    instruction
End If

Can be written as:

If condition Then instruction

Related: #25 If block body can be on separate line

Property with public mutator (Let/Set) should expose a public accessor (Get)

[Warning]

Properties that expose no public getter but have public mutators, should be complemented with a getter.

Public Property Let Foo(ByVal value As String)
    this.Foo = value
End Property

When Property Let is the only Public member for Foo, Rubberduck should recommend adding this:

Public Property Get Foo() As String
    Foo = this.Foo
End Property

Use of undeclared identifier

[Warning]

Detect when code is using undeclared identifiers.

Public Sub Foo()
    bar = 123 'not declared anywhere
End Sub

Related: #15 Use Option Explicit.

DeclarationNode

Current DeclarationNode code should be the base type for declaration nodes, and be a child node under a new node type that supports multiple child nodes.

• Parser should generate a DeclarationNode with as many child nodes as there are declarations in the parsed instruction / logical code line.

Parser is skipping syntaxes with SyntaxType.IsChildNodeSyntax, so:

• ConstantSyntax, DimSyntax, GlobalFieldSyntax, PrivateFieldSyntax and PublicFieldSyntax should be derived from SyntaxBase, with SyntaxType.IsChildNodeSyntax.
• Need to implement a DeclarationSyntax class not derived from SyntaxBase but implementing ISyntax (because SyntaxBase.CreateNode only passes a single Instruction), and then include this DeclarationSyntax class in the parser's grammar.

Use meaningful names

Detect when an identifier is named after its type and followed by a number, e.g. Label1 or TextBox1, or when there are multiple identifiers with the same name, only with a different number suffix, like foo1, foo2, foo3.

More formally:

• Identifier is less than 3 characters long
• Identifier ends with a number
• Identifier contains no vowel (suggest better name from lookup "dictionary" of disemvoweled words?)

multiple opened projects named VBProject will break the fully-qualified method calls

Test methods are called with a fully-qualified syntax ProjectName.ModuleName.TestMethodName - however if there are two or more projects with the same name, VBA will not be able to resolve the method call and an exception will be thrown.
If the test method is added from the Test Explorer, an exception will be thrown when an existing TestMethod (ProjectName.ModuleName.TestMethodName) is added to the session.

ToDoItemsMenu should inherit from Menu

Use of obsolete Call instruction

[Warning]

Call is obsolete, only included in the language for backward compatibility; replace with inline procedure call.

Call DoSomething

Becomes

DoSomething

Use CodeBlockNode to determine indentation level?

The parsed code tree contains CodeBlockNode items, which should add an indentation level. Each node in the tree contains an Instruction which knows about its StartColumn, and each Instruction knows about a LogicalCodeLine, which knows about its StartLine.

Using the parsed code tree may seem like a good idea for the indenter, but there are complications:

• A LogicalCodeLine can refer to more than 1 line of code
• An Instruction may not be the only instruction on a code line; its StartColumn isn't necessarily its indentation level.

Indentation applies to code lines - not to logical code lines, not to instructions. Should the code tree be modified to accomodate this? Or should we find another way of determining code indentation?

Variable type not declared.

[Warning]

Variable declared without a type. Variable is implicitly typed as variant.

Variable declarations without a type will be implicitly typed as a variant. Recommend explicit declaration.

Dim someString

Related to #27

Unreacheable code detected

[Warning]

Rubberduck should detect unreacheable code, and warn about it.

DoSomething 123
Exit Sub

DoSomethingElse 456 'unreacheable code (not under a label, and/or no reacheable jump to that label)

DoSomething 123
Err.Raise SomeError

DoSomethingElse 456 'unreacheable code (unless On Error Resume Next is specified)

DoSomething 123
If False Then
    DoSomethingElse 456 'unreacheable code (condition expression must be statically evaluatable)
End If

If IDE is set to Require Variable Declaration, Option Explicit is inserted twice when adding new test module

Handle runtime errors

Detect On Error Resume Next instructions, and recommend adding an error-handling subroutine instead.

    On Error Resume Next
    foo(42) = "bar"

Can be written as:

    On Error GoTo ErrHandler
    foo(42) = "bar"

    Exit Sub
ErrHandler:
    If Err.Number = 9 Then 'Subscript out of range
        Err.Clear
        Resume Next
    End If

---

Rubberduck may not be able to detect exactly which runtime error to handle. Perhaps the quickfix snippet can look like this:

    On Error GoTo {ErrorHandlerLabel}

    {code}

    Exit {Sub|Function|Property}
{ErrorHandlerLabel}:
    If Err.Number > 0 Then 'todo: handle specific error
        Err.Clear
        Resume Next
    End If

Export all code files to local repository

Rubberduck needs a feature that will export all code files to a local repository.

Decouple TestExplorerWindow from TestEngine

The TestExplorerWindow and TestEngine are tightly bound together. For example:

    public TestMenu(VBE vbe, AddIn addInInstance):base(vbe, addInInstance)
    {
        TestExplorerWindow testExplorer = new TestExplorerWindow();
        toolWindow = CreateToolWindow("Test Explorer", testExplorer);
        _engine = new TestEngine(vbe, testExplorer, toolWindow);
    }

And

    public void Run(TestMethod test)
    {
        _explorer.ClearProgress();
        var tests = new Dictionary<TestMethod, TestResult> { { test, null } };

        _explorer.SetPlayList(tests);
        AssignResults(tests.Keys);

        _lastRun = tests.Keys;
        ShowExplorerWindow();
    }

Test Engine should raise events that the TestExplorerWindow listens for. When these events are raised, TestExplorer can make itself visible.

GoSub can be extracted to procedure

[Suggestion]

GoSub...Return statements should trigger a recommendation to refactor/extract procedure/function (depending on context):

    foo = GetFoo(123)
    If foo Then GoSub DoSomething
    Exit Sub

DoSomething:
    bar = 456
    Return

Should be refactored to:

    foo = GetFoo(123)
    If foo Then bar = DoSomething
    Exit Sub

Private Function DoSomething() As Integer
    DoSomething = 456
End Function

Options should be specified at the top of code module

[Suggestion]

When options are specified after declarations in a code module, Rubberduck should recommend moving them to the top.

Private foo As Bar
Option Base 1
Option Explicit

Can be written as:

Option Base 1
Option Explicit

Private foo As Bar

CodeBlock nodes are added as ...CodeBlock nodes

Implement an abstract node factory, so as to retain actual node types.