Fix false positive in magic number checker

[0x8000-0000]

Summary

Fix false positive in magic number checker

https://bugs.llvm.org/show_bug.cgi?id=40640: cppcoreguidelines-avoid-magic-numbers should not warn about enum class

Test case code reduced by Pavel Kryukov.

[0x8000-0000]

clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp

127

127-130 are the new lines. The rest were moved about by clang-format. Please let me know if I should revert the format.

[Eugene.Zelenko]

Please mention fix in Release Notes.

[0x8000-0000]

Update release notes.

[0x8000-0000]

Fix typo.

[aaron.ballman]

clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp

129

So will this no longer treat *any* C-style cast as being a magic number? e.g., int i; i = (int)12;

[0x8000-0000]

clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp

129

Sadly, it would seem so.

if (ValueArray[(int)4] > ValueArray[1]) produces the following AST:

|   | | | `-ArraySubscriptExpr 0x11545f0 <col:7, col:24> 'int' lvalue
|   | | |   |-ImplicitCastExpr 0x11545d8 <col:7> 'int *' <ArrayToPointerDecay>
|   | | |   | `-DeclRefExpr 0x1154558 <col:7> 'int [5]' lvalue Var 0x1153cb0 'ValueArray' 'int [5]'                                                           
|   | | |   `-CStyleCastExpr 0x11545b0 <col:18, col:23> 'int' <NoOp>
|   | | |     `-IntegerLiteral 0x1154578 <col:23> 'int' 4

While the test case we're trying to avoid is:

`-TemplateSpecializationType 0x1169ee0 'holder<(Letter)9U>' sugar holder
  |-TemplateArgument expr
  | `-ConstantExpr 0x1169dc0 <col:59> 'Letter' 9
  |   `-SubstNonTypeTemplateParmExpr 0x1169da0 <col:59> 'Letter'
  |     `-CStyleCastExpr 0x1169d78 <col:59> 'Letter' <IntegralCast>
  |       `-IntegerLiteral 0x1169d48 <col:59> 'unsigned int' 9
  `-RecordType 0x1169ec0 'holder<Letter::J>'
    `-ClassTemplateSpecialization 0x1169de0 'holder'

Now, the question is - do we care? How many people use a C cast with a bare integer?

I suppose I can add a check for the grandparent node to be SubstNonTypeTemplateParmExpr, in order to filter this out?

[0x8000-0000]

clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp

129

An interesting fact is that the C-style cast does not produce a warning now anyway (even before applying this change.) and I don't have any idea why.

Adding these lines does not create a test failure:

+  79   if (((int)4) > IntSquarer[10])                                                                                                                         
+  80     return 0;

[0x8000-0000]

Add a test file for expected failures

[0x8000-0000]

clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers-todo.cpp

9 ↗

(On Diff #234998)

This test fails even with clang-tidy-9.

$ clang-tidy-9 --checks="-*,readability-magic-numbers" clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers-todo.cpp

/home/florin/tools/llvm-project/clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers-todo.cpp:9:35: warning: 10 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
  if (((int)4) > ProcessSomething(10))
                                  ^

Nothing about the "4" here.

[0x8000-0000]

My take: this change fixes a user-reported bug, and does not cause any known regressions. I think we should integrate this.

[aaron.ballman]

In D71686#1794053, @0x8000-0000 wrote:

My take: this change fixes a user-reported bug, and does not cause any known regressions. I think we should integrate this.

I sort of wonder whether we want to document this as a blessed approach to silencing the warning. I'm not certain if it's too obtuse or not, but I notice the check has no documented ways to silence the diagnostic aside from using the correct kind of magic number or adding it to a list of excluded magic numbers.

[0x8000-0000]

In D71686#1794330, @aaron.ballman wrote:

In D71686#1794053, @0x8000-0000 wrote:

My take: this change fixes a user-reported bug, and does not cause any known regressions. I think we should integrate this.

I sort of wonder whether we want to document this as a blessed approach to silencing the warning. I'm not certain if it's too obtuse or not, but I notice the check has no documented ways to silence the diagnostic aside from using the correct kind of magic number or adding it to a list of excluded magic numbers.

You mean Hyrum's Law is not sufficient?

The check can be silenced with the regular NOLINT, or with defining and using a constant/enum. Using this "backdoor" way seems even more cumbersome and confusing than the NOLINT. At least with NOLINT it is clear what you're doing, and somebody else can grep for it and fix it if it is appropriate.

[aaron.ballman]

In D71686#1794360, @0x8000-0000 wrote:

In D71686#1794330, @aaron.ballman wrote:

In D71686#1794053, @0x8000-0000 wrote:

My take: this change fixes a user-reported bug, and does not cause any known regressions. I think we should integrate this.

I sort of wonder whether we want to document this as a blessed approach to silencing the warning. I'm not certain if it's too obtuse or not, but I notice the check has no documented ways to silence the diagnostic aside from using the correct kind of magic number or adding it to a list of excluded magic numbers.

You mean Hyrum's Law is not sufficient?

The check can be silenced with the regular NOLINT, or with defining and using a constant/enum. Using this "backdoor" way seems even more cumbersome and confusing than the NOLINT. At least with NOLINT it is clear what you're doing, and somebody else can grep for it and fix it if it is appropriate.

My concern is that NOLINT is insufficient. Consider: foo(12, 42, 18); where the 42 is not desired to be warned about due to domain-specific knowledge but the 12 and 18 are.

However, I am not convinced that you're wrong either -- casting is cumbersome. But it's also a somewhat well-used workaround to silence warnings (casting to void silencing unused value warnings being a common example).

For right now, we can leave it as a bug -- we can decide to bless the approach later.

[aaron.ballman]

LGTM aside from some minor nits.

clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers-todo.cpp

2 ↗

(On Diff #234998)

You might as well move this onto the previous line; it just looks strange here.

clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp

219

prove -> Prove

and add a full stop to the end of the comment.

[0x8000-0000]

In D71686#1794366, @aaron.ballman wrote:

In D71686#1794360, @0x8000-0000 wrote:

In D71686#1794330, @aaron.ballman wrote:

In D71686#1794053, @0x8000-0000 wrote:

My take: this change fixes a user-reported bug, and does not cause any known regressions. I think we should integrate this.

I sort of wonder whether we want to document this as a blessed approach to silencing the warning. I'm not certain if it's too obtuse or not, but I notice the check has no documented ways to silence the diagnostic aside from using the correct kind of magic number or adding it to a list of excluded magic numbers.

You mean Hyrum's Law is not sufficient?

The check can be silenced with the regular NOLINT, or with defining and using a constant/enum. Using this "backdoor" way seems even more cumbersome and confusing than the NOLINT. At least with NOLINT it is clear what you're doing, and somebody else can grep for it and fix it if it is appropriate.

My concern is that NOLINT is insufficient. Consider: foo(12, 42, 18); where the 42 is not desired to be warned about due to domain-specific knowledge but the 12 and 18 are.

However, I am not convinced that you're wrong either -- casting is cumbersome. But it's also a somewhat well-used workaround to silence warnings (casting to void silencing unused value warnings being a common example).

For right now, we can leave it as a bug -- we can decide to bless the approach later.

Fair enough; I'm aiming for good precision and recall by default.

[0x8000-0000]

Minor comment fixes; capitalization, full stop.

[0x8000-0000]

@aaron.ballman updated as suggested; please commit/integrate when you have a moment. Thank you!

[aaron.ballman]

In D71686#1794377, @0x8000-0000 wrote:

@aaron.ballman updated as suggested; please commit/integrate when you have a moment. Thank you!

Happy to do so, thank you for the fix! I've commit in c16b3ec597d277b5a7397db308f8ec730f3330a3

clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp

     return AsDecl->isImplicit();
   }
 
-  if (Node.get<EnumConstantDecl>() != nullptr)
+  if (Node.get<EnumConstantDecl>())
     return true;
 
   return llvm::any_of(Result.Context->getParents(Node),

 
 bool MagicNumbersCheck::isConstant(const MatchFinder::MatchResult &Result,
                                    const Expr &ExprResult) const {
-  return llvm::any_of(
-      Result.Context->getParents(ExprResult),
-      [&Result](const DynTypedNode &Parent) {
-        if (isUsedToInitializeAConstant(Result, Parent))
-          return true;
-
-        // Ignore this instance, because this match reports the location
-        // where the template is defined, not where it is instantiated.
-        if (Parent.get<SubstNonTypeTemplateParmExpr>())
-          return true;
-
-        // Don't warn on string user defined literals:
-        // std::string s = "Hello World"s;
-        if (const auto *UDL = Parent.get<UserDefinedLiteral>())
-          if (UDL->getLiteralOperatorKind() == UserDefinedLiteral::LOK_String)
-            return true;
-
-        return false;
-      });
+  return llvm::any_of(Result.Context->getParents(ExprResult),
+                      [&Result](const DynTypedNode &Parent) {
+                        if (isUsedToInitializeAConstant(Result, Parent))
+                          return true;
+
+                        // Ignore this instance, because this matches an
+                        // expanded class enumeration value.
+                        if (Parent.get<CStyleCastExpr>())
+                          return true;
+
+                        // Ignore this instance, because this match reports the
+                        // location where the template is defined, not where it
+                        // is instantiated.
+                        if (Parent.get<SubstNonTypeTemplateParmExpr>())
+                          return true;
+
+                        // Don't warn on string user defined literals:
+                        // std::string s = "Hello World"s;
+                        if (const auto *UDL = Parent.get<UserDefinedLiteral>())
+                          if (UDL->getLiteralOperatorKind() ==
+                              UserDefinedLiteral::LOK_String)
+                            return true;
+
+                        return false;
+                      });
 }
 
 bool MagicNumbersCheck::isIgnoredValue(const IntegerLiteral *Literal) const {

clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp

 
   return Total;
 }
+
+// prove that using enumerations values don't produce warnings (code by Pavel Kryukov)
+enum class Letter : unsigned {
+    A, B, C, D, E, F, G, H, I, J
+};
+
+template<Letter x> struct holder  { Letter letter = x;  };
+template<Letter x> struct wrapper { using h_type = holder<x>;  };
+
+template struct wrapper<Letter::A>;
+template struct wrapper<Letter::J>;