Rubberduck: Inspection for arithmetic + operations on enum members

[IvenBach]

I kneejerk use MsgBox "This will show the third button selected", vbYesNoCancel + vbDefaultButton3 + vbInformation without thinking about it. Was corrected to use MsgBox "This will show the third button selected", vbYesNoCancel Or vbDefaultButton3 Or vbInformation.

Let's bring this to other users attention.

[Inarion]

Why is the latter preferable to the former? (Much to learn I still have.)

[retailcoder]

@Inarion blame @bclothier for this one :wink:

I'd propose this wording (under "Readability & Maintainability", and "suggestion" or "hint" level):

Bitwise operation uses arithmetic operator
While mathematically equivalent, the arithmetic addition + and bitwise Or operators don't convey the same semantics: in the case of bitwise operations on enum members, using a bitwise operator better conveys the actual operation being executed.

Better ideas welcome :smiley:

This inspection will fire a false positive for any enum that has a meaningful numerical value in the arithmetic sense (can't think of a legit example of one though); let's allow an @Ignore annotation at the enum declaration level (for user code anyway), so that a legit + can be ignored once and for all at the declaration site.

[Hosch250]

FWIW, you should still cast the value to a long or whatever if you want to do real addition with it, no?

[retailcoder]

@Hosch250 Correct... and this means the inspection needs to evaluate the type of the operands around + operators, which is a bit more involved than the OP assumed.

[retailcoder]

Actually, that's not the job of the inspection at all - we can consider this inspection blocked until a "compilation pass" is implemented in the parser/resolver engine, to annotate expressions with their type.

ref. #3184

[comintern]

I'd go 10 steps further with this personally. IDL doesn't have a [Flags] attribute for enumerations like .NET does, but it should be relatively easy to heuristically determine whether or not a set of enum values represent bit masks at a high confidence level. I'd propose adding a custom decoration to those enumerations and basing inspections on _that_ also. I.e.:

Mathematical operation on an Enum that doesn't represent flags
Mathematically operators are being applied to members of an enumeration that cannot heuristically represent bit flags. This is either a coding error or obfuscates the intention of the code.

If the result of the operation is a valid enumeration member, the quick fix would be to replace the expression with the enumeration member it would result in.

[ThunderFrame]

@comintern there's already an issue for '@Flags on project-level Enums, so this would tie in nicely:

see: #3375

The problems come with enums like VbMsgBoxStyle - where there are flags and extra members that are already the result of bitwise operations like vbDefaultButton4 , or they're repeated, like vbDefaultButton1 and vbApplicationModal

typedef [uuid(ed822011-6d7f-11cf-b949-00aa004455ea), version(0.0)]
enum {
    const int vbOKOnly = 0,
    const int vbOKCancel = 1,
    const int vbAbortRetryIgnore = 2,
    const int vbYesNoCancel = 3,
    const int vbYesNo = 4,
    const int vbRetryCancel = 5,
    const int vbCritical = 16,
    const int vbQuestion = 32,
    const int vbExclamation = 48,
    const int vbInformation = 64,
    const int vbDefaultButton1 = 0,
    const int vbDefaultButton2 = 256,
    const int vbDefaultButton3 = 512,
    const int vbDefaultButton4 = 768,
    const int vbApplicationModal = 0,
    const int vbSystemModal = 4096,
    const int vbMsgBoxHelpButton = 16384,
    const int vbMsgBoxRight = 524288,
    const int vbMsgBoxRtlReading = 1048576,
    const int vbMsgBoxSetForeground = 65536
} VbMsgBoxStyle;

[bclothier]

There is another potential issue - there may be enums that contains an union. For example, assume we have this enum from some other library that renders in VBE as:

Public Enum IAmReallyFlagsPinkyPromise
  ModeOne = 1
  ModeTwo = 2
  BothMode = 3
End Enum

The BothMode is basically a ModeOne Or ModeTwo. Even if it was originally written as BothMode = ModeOne Or ModeTwo, there's no guarantee that it won't retain that information once exported into a type library. It would be difficult to determine that the "extra" members in the enum are simply meant to represent shortcuts for other members Or'd together.

[comintern]

@ThunderFrame I'm not sure that these would really be a problem. VbMsgBoxStyle can be determined to be set of flags, so in my head, simply using Or to join them shouldn't trigger the inspection noted above. Perhaps a second inspection for semantically stupid flag use (it would need a better name than that)? For example there isn't really a reason to do something like vbOKOnly Or vbApplicationModal. Or maybe (once we're evaluating expressions) generalize that to any bitwise expression that can be simplified - i.e. foo = 1 Or 3.

@bclothier In the case of IAmReallyFlagsPinkyPromise, that raises an interesting philosophical question about the nature of flags. : -D If all possible flag combinations are represented by enumeration members, a heuristic would determine that they aren't flags. But in that case, you don't ever need to use them as flags because you should always have a single flag option. I think ModeOne Or ModeTwo triggering the inspection would be fine, because you probably should be using BothMode in your code.