Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VarType equality check #5616

Open
retailcoder opened this issue Nov 3, 2020 · 4 comments
Open

VarType equality check #5616

retailcoder opened this issue Nov 3, 2020 · 4 comments
Labels
enhancement Feature requests, or enhancements to existing features. Ideas. Anything within the project's scope. feature-inspections up-for-grabs Use this label in conjunction with a difficulty level label, e.g. difficulty-02-ducky

Comments

@retailcoder
Copy link
Member

retailcoder commented Nov 3, 2020

What
An inspection that locates call sites for the VBA.Information.VarType function, and flags a direct = check as problematic when the operand is VbVarType.vbArray. Although, it probably doesn't hurt to flag any direct = comparison from the return value of VBA.Information.VarType, either.

Why
The enum value returned by the VarType function requires bitwise comparison to evaluate as intended: as written, the condition is systematically false given a Variant() (variant array) value, because what's returned is a compound value that boils down to vbArray+vbVariant.

Example
This code should trigger the inspection:

Public Sub DoSomething()
    Select Case True
        Case VarType(Value) = vbArray '<~ here
            '...
        Case Else
            Debug.Print TypeName(Value)
    End Select
End Sub

QuickFixes

  1. Convert to bitwise comparison

    Example code, after quickfix is applied:

    Public Sub DoSomething()
        Select Case True
            Case (VarType(Value) And VbVarType.vbArray) = VbVarType.vbArray
                '...
            Case Else
                Debug.Print TypeName(Value)
        End Select
    End Sub

Resources
Each inspection needs a number of resource strings - please provide a suggestion here:

  • InspectionNames: VarTypeComparisonInspection
  • InspectionInfo: TBD
  • InspectionResults: TBD
@retailcoder retailcoder added enhancement Feature requests, or enhancements to existing features. Ideas. Anything within the project's scope. feature-inspections up-for-grabs Use this label in conjunction with a difficulty level label, e.g. difficulty-02-ducky labels Nov 3, 2020
@retailcoder
Copy link
Member Author

retailcoder commented Nov 3, 2020

💡 there has to be a way to make that quick-fix readily applicable to any = operator involving Enum operands...

(without there necessarily being an inspection result)

@MDoerner
Copy link
Contributor

MDoerner commented Nov 3, 2020

I understand the use of warning against direct comparison to vbArray, but I do not like the idea to flag it for any other value.
Why should VarType(foo) = vbLong be suspicious, if I want to test whether the variable foo contains a scalar of value type Long? If And this with vbLong I might get an array, which is probably not what I want since I would have to access it a different way than a scalar.

@bclothier
Copy link
Contributor

The real problem is that the VbVarType enum is not just a bitmask. It has non-flag members with few flag members. Therefore, the case of VarType(someVariant) = vbLong would fail if my intention was to assert whether I had a Long value or an array of Long values before proceeding. Your point about requiring different access is well-taken but it won't change the fact that if I have a Variant parameter and I need to support both a scalar value and array value but still assert that it's only of a particular data type (e.g. I am not handed an array of strings for example). Such check would be likely expressed as:

If VarType(someVariant) = vbLong Then
  HandleScalarValue
ElseIf VarType(someVariant) = vbArray Then
  HandleArrayValues
Else
  Err.Raise 5, , "Only Long or an array of Longs are supported"
End If

This naive approach probably should be converted into something like this:

Dim valueType As VarType
Set valueType = VarType(someVariant)

If (valueType And vbLong = vbLong) = False Then
  Err.Raise 5, , "Only Long or an array of Longs are supported."
End If

If (valueType And vbArray = vbArray) Then
  HandleArrayValues
Else
  HandleScalarValue
End If

In that example, we needed to change the check for both vbLong and vbArray to make it work for both cases correctly. The problem becomes worse if we need to check whether a variant is either a vbLong or a vbDouble -- those cannot be treated like a flags so expression of VarType(someVariant) = (vbLong Or vbDouble) would be totally wrong. We'd need to do VarType(someVariant) And vbLong = vbLong and VarType(someVariant) And vbDouble = vbDouble to handle that scenario.

The point being, it's easy to get the usage of VarType wrong due to the fact that it contains some bitwise members with non-bitwise members. It would require additional code analysis to determine whether the VarType(someVariant) = vbLong is the only thing that's being done. I think it's rare that this will be the case.

@retailcoder
Copy link
Member Author

Also to be taken into account: non-equality checks involving a <> operator, which would be similarly "always true" (the fix could be the same just surrounded with a Not operator, or the bitwise logic can be flipped - either works)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests, or enhancements to existing features. Ideas. Anything within the project's scope. feature-inspections up-for-grabs Use this label in conjunction with a difficulty level label, e.g. difficulty-02-ducky
Projects
None yet
Development

No branches or pull requests

3 participants