clang-tidy: new check readability-misplaced-array-index

[danielmarjamaki]

Summary

this is a new check for clang-tidy that diagnoses when it see unusual array index syntax.

there is nothing wrong about such syntax so it's not a misc check. It's just a readability check.

[Eugene.Zelenko]

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

[danielmarjamaki]

updated releasedocs
deeper lookup if macro is used
only warn when the replacement can be done safely. the expressions "x[y+z]" and y+z[x]" are not the same.

[danielmarjamaki]

fixed typo in releasenotes

[alexfh]

The check seems reasonable, I'm surprised there's no warning in Clang that catches index[array] syntax ;)

A few comments re: implementation.

clang-tidy/readability/MisplacedArrayIndexCheck.cpp

44

Looks like this repeats getText from clang/Tooling/FixIt.h.

74

Looks like you could use createReplacement and/or getText from clang/Tooling/FixIt.h.

[danielmarjamaki]

clang-tidy/readability/MisplacedArrayIndexCheck.cpp

44

Yes indeed..

[danielmarjamaki]

Fixed review comments.

[danielmarjamaki]

Cleanup my previous commit. Ran clang-format.

[alexfh]

Thanks, that's better. Still a couple of comments.

clang-tidy/readability/MisplacedArrayIndexCheck.cpp

51

I'd say "confusing" instead of "unusual". This would also help avoiding the repetition ("unusual ..., usually ..."):

"confusing array index syntax; usually, the index is inside the brackets"

55

What exactly is the recursive hasMacroID function trying to prevent? Do you have a test that breaks if you just check that the start location of the expression is not a macro?

In most cases, it's enough to check that the whole range is on the same macro expansion level using Lexer::makeFileCharRange in order to prevent most problems with macros, while allowing fixes in safe cases, e.g. replacements in parameters of EXPECT_* macros from gtest.

[aaron.ballman]

Is there a reason we don't want this check to be a part of the clang frontend, rather than as a clang-tidy check?

clang-tidy/readability/MisplacedArrayIndexCheck.cpp

28–29

Can this use hasType(pointerType()) instead of hasType(pointsTo(qualType())) ?

49

Should not use auto here because the type is not spelled out in the initialization.

51

Instead of "array index syntax", perhaps "array subscript expression"?

53

What does "contraptions" mean in this context?

[jroelofs]

I think the replacement is wrong for something like:

int *arr; int offs1, offs2;
offs1[arr + offs2] = 42;

which I think would give:

int *arr; int offs1, offs2;
arr + offs2[offs1] = 42;

If the precedence of the "indexing" argument is less than that of the subscript operator, then you need to insert parens:

int *arr; int offs1, offs2;
(arr + offs2)[offs1] = 42;

[alexfh]

In D21134#508524, @jroelofs wrote:

I think the replacement is wrong for something like:

int *arr; int offs1, offs2;
offs1[arr + offs2] = 42;

which I think would give:

int *arr; int offs1, offs2;
arr + offs2[offs1] = 42;

If the precedence of the "indexing" argument is less than that of the subscript operator, then you need to insert parens:

int *arr; int offs1, offs2;
(arr + offs2)[offs1] = 42;

Indeed. It might be relevant to future (current ones seem to be fine) uses of tooling::fixit::createReplacement as well, so should probably be taken care of in its implementation. Maybe an additional hint from the calling code is needed (bool InsertParensIfNeeded = false).

[alexfh]

clang-tidy/readability/MisplacedArrayIndexCheck.cpp

49

While in general I agree with this recommendation, auto Diag = diag(...); has almost become an idiom in this code. I'd not worry about obscurity of auto in this context, especially, if the variable is renamed to Diag, which will make its meaning and possible ways of using it clearer.

[aaron.ballman]

clang-tidy/readability/MisplacedArrayIndexCheck.cpp

49

I'm okay with that approach.

[danielmarjamaki]

In D21134#508511, @aaron.ballman wrote:

Is there a reason we don't want this check to be a part of the clang frontend, rather than as a clang-tidy check?

I discussed this with frontend and clang-tidy people in the llvm meeting last year and we came to the conclusion it was better in clang-tidy.

I don't remember the exact reasons.

But I think that this code is just "weird". It could be a bug but I write the warning mostly for readability reasons.

clang-tidy/readability/MisplacedArrayIndexCheck.cpp

44

this is fixed. As you suggested I used createReplacement() in the fixit. It's much nicer!

[danielmarjamaki]

In D21134#508524, @jroelofs wrote:

I think the replacement is wrong for something like:

int *arr; int offs1, offs2;
offs1[arr + offs2] = 42;

which I think would give:

int *arr; int offs1, offs2;
arr + offs2[offs1] = 42;

If the precedence of the "indexing" argument is less than that of the subscript operator, then you need to insert parens:

int *arr; int offs1, offs2;
(arr + offs2)[offs1] = 42;

I don't think so. The matcher says:

hasRHS(ignoringParenImpCasts(
          anyOf(stringLiteral(), declRefExpr(hasType(pointsTo(qualType()))),
                memberExpr(hasType(pointsTo(qualType()))

I think it's hard to know what the replaced code should be. In some cases it might be better with:

arr[offs1 + offs2] = 42;

[danielmarjamaki]

take care of review comments

[danielmarjamaki]

as far as I see the only unsolved review comment now is about macros. if it can be handled better.

clang-tidy/readability/MisplacedArrayIndexCheck.cpp

29–30

Yes thanks!

54

you can do lots of weird things with macros. so I wanted to just diagnose and then have no fixits that would be wrong etc. I removed the comment since I think the code is pretty clear.

56

What exactly is the recursive hasMacroID function trying to prevent? Do you have a test that breaks if you just check that the start location of the expression is not a macro?

I am worried about macros anywhere. I did not want to apply fixes for this code: 0[ABC] but maybe using the Lexer would make that safe.

In most cases, it's enough to check that the whole range is on the same macro expansion level using Lexer::makeFileCharRange in order to prevent most problems with macros, while allowing fixes in safe cases, e.g. replacements in parameters of EXPECT_* macros from gtest.

I was very unsuccessful with that. I must do something wrong...

CharSourceRange LRange = Lexer::makeFileCharRange(
    CharSourceRange::getCharRange(
        ArraySubscriptE->getLHS()->getSourceRange()),
    Result.Context->getSourceManager(), Result.Context->getLangOpts());

CharSourceRange RRange = Lexer::makeFileCharRange(
    CharSourceRange::getCharRange(
        ArraySubscriptE->getLHS()->getSourceRange()),
    Result.Context->getSourceManager(), Result.Context->getLangOpts());

StringRef LText = Lexer::getSourceText(LRange, *Result.SourceManager,
                                      Result.Context->getLangOpts());

StringRef RText = Lexer::getSourceText(RRange, *Result.SourceManager,
                                      Result.Context->getLangOpts());

Diag << FixItHint::CreateReplacement(ArraySubscriptE->getLHS()->getSourceRange(),
                                     RText);
Diag << FixItHint::CreateReplacement(ArraySubscriptE->getRHS()->getSourceRange(),
                                  LText);

No fixits worked with that code, with or without macros.

Do you see what I did wrong?

[aaron.ballman]

test/clang-tidy/readability-misplaced-array-index.cpp

32

Why is this considered to be "normal syntax" and not worth getting a diagnostic?

[danielmarjamaki]

test/clang-tidy/readability-misplaced-array-index.cpp

32

hmm.. I agree that it would be good to have a diagnostic for this code also.

I currently only diagnose when subscript is stringLiteral, declRefExpr or memberExpr. These are the cases when the code can be easily fixed by just replacing the expressions.

I will look into writing a diagnostic for this also.

[jroelofs]

In D21134#509522, @danielmarjamaki wrote:

In D21134#508524, @jroelofs wrote:

I think the replacement is wrong for something like:

int *arr; int offs1, offs2;
offs1[arr + offs2] = 42;

which I think would give:

int *arr; int offs1, offs2;
arr + offs2[offs1] = 42;

If the precedence of the "indexing" argument is less than that of the subscript operator, then you need to insert parens:

int *arr; int offs1, offs2;
(arr + offs2)[offs1] = 42;

I don't think so. The matcher says:

hasRHS(ignoringParenImpCasts(
          anyOf(stringLiteral(), declRefExpr(hasType(pointsTo(qualType()))),
                memberExpr(hasType(pointsTo(qualType()))

I think it's hard to know what the replaced code should be. In some cases it might be better with:

arr[offs1 + offs2] = 42;

Oh, yeah, you're right.

[danielmarjamaki]

More generic diagnostic. Diagnose all integerType[pointerType] expressions.

[danielmarjamaki]

ran clang-format

[alexfh]

clang-tidy/readability/MisplacedArrayIndexCheck.cpp

57

Both ranges seem to be using ArraySubscriptE->getLHS()->getSourceRange(). I guess, the second one should refer to ArraySubscriptE->getRHS()->getSourceRange() instead?

docs/clang-tidy/checks/readability-misplaced-array-index.rst

13

Please format examples according to LLVM style (specifically, leave braces at the end of the previous line.

test/clang-tidy/readability-misplaced-array-index.cpp

17

Please truncate all CHECK-MESSAGES except for the first one after "confusing array subscript expression".

26

Please add a CHECK-FIXES to verify the original code is left intact. Same for other places where no fixits are expected.

[danielmarjamaki]

fixed review comments

[danielmarjamaki]

clang-tidy/readability/MisplacedArrayIndexCheck.cpp

58

I removed hasMacroId() and use fixit::getText(). The replacements look good now.

docs/clang-tidy/checks/readability-misplaced-array-index.rst

14

ok thanks same mistake I've done before. Should I start using uppercase variable names from now on?

[alexfh]

A few nits.

clang-tidy/readability/MisplacedArrayIndexCheck.h

20

Please enclose inline code snippets in backquotes.

docs/clang-tidy/checks/readability-misplaced-array-index.rst

11

This should be .. code-block:: c++.

14

Interesting question. Probably, better to do so.

[danielmarjamaki]

fix review comments

[alexfh]

Looks good.

Please ensure it works reasonably on LLVM code before submitting.

Sorry for the delay. Feel free to ping earlier.

clang-tidy/readability/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyReadabilityModule
   AvoidConstParamsInDecls.cpp
   BracesAroundStatementsCheck.cpp
   ContainerSizeEmptyCheck.cpp
   DeletedDefaultCheck.cpp
   ElseAfterReturnCheck.cpp
   FunctionSizeCheck.cpp
   IdentifierNamingCheck.cpp
   ImplicitBoolCastCheck.cpp
   InconsistentDeclarationParameterNameCheck.cpp
+  MisplacedArrayIndexCheck.cpp
   NamedParameterCheck.cpp
   NamespaceCommentCheck.cpp
   ReadabilityTidyModule.cpp
   RedundantControlFlowCheck.cpp
   RedundantStringCStrCheck.cpp
   RedundantSmartptrGetCheck.cpp
   RedundantStringInitCheck.cpp
   SimplifyBooleanExprCheck.cpp

clang-tidy/readability/MisplacedArrayIndexCheck.h

+//===--- MisplacedArrayIndexCheck.h - clang-tidy-----------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_MISPLACED_ARRAY_INDEX_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_MISPLACED_ARRAY_INDEX_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Warn about unusual array index syntax (index[array] instead of
+/// array[index]).
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-misplaced-array-index.html
+class MisplacedArrayIndexCheck : public ClangTidyCheck {
+public:
+  MisplacedArrayIndexCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_MISPLACED_ARRAY_INDEX_H

clang-tidy/readability/MisplacedArrayIndexCheck.cpp

+//===--- MisplacedArrayIndexCheck.cpp - clang-tidy-------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "MisplacedArrayIndexCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Tooling/FixIt.h"
+
+using namespace clang::ast_matchers;
+using clang::tooling::fixit::internal::getText;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+void MisplacedArrayIndexCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      arraySubscriptExpr(
+          hasLHS(hasType(isInteger())),
+          hasRHS(ignoringParenImpCasts(
+              anyOf(stringLiteral(), declRefExpr(hasType(pointsTo(qualType()))),
+                    memberExpr(hasType(pointsTo(qualType())))))))
+          .bind("expr"),
+      this);
+}
+
+static bool hasMacroID(const Expr *E) {
+  if (E->getExprLoc().isMacroID())
+    return true;
+  for (const Stmt *Child : E->children()) {
+    const Expr *ChildExpr = dyn_cast<Expr>(Child);
+    if (ChildExpr && hasMacroID(ChildExpr))
+      return true;
+  }
+  return false;
+}
+
+void MisplacedArrayIndexCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *ArraySubscriptE =
+      Result.Nodes.getNodeAs<ArraySubscriptExpr>("expr");
+
+  auto D =
+      diag(ArraySubscriptE->getLocStart(),
+           "unusual array index syntax, usually the index is inside the []");
+
+  // Don't even try to resolve macro or include contraptions. Not worth emitting
+  // a fixit for.
+  if (hasMacroID(ArraySubscriptE) || !Result.SourceManager->isWrittenInSameFile(ArraySubscriptE->getLocStart(), ArraySubscriptE->getLocEnd()))
+    return;
+
+  const Expr *LHS = ArraySubscriptE->getLHS();
+  const Expr *RHS = ArraySubscriptE->getRHS();
+
+  D << tooling::fixit::createReplacement(*LHS, *RHS, *Result.Context)
+    << tooling::fixit::createReplacement(*RHS, *LHS, *Result.Context);
+
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

clang-tidy/readability/ReadabilityTidyModule.cpp

 #include "BracesAroundStatementsCheck.h"
 #include "ContainerSizeEmptyCheck.h"
 #include "DeletedDefaultCheck.h"
 #include "ElseAfterReturnCheck.h"
 #include "FunctionSizeCheck.h"
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolCastCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
+#include "MisplacedArrayIndexCheck.h"
 #include "NamedParameterCheck.h"
 #include "RedundantControlFlowCheck.h"
 #include "RedundantSmartptrGetCheck.h"
 #include "RedundantStringCStrCheck.h"
 #include "RedundantStringInitCheck.h"
 #include "SimplifyBooleanExprCheck.h"
 #include "StaticDefinitionInAnonymousNamespaceCheck.h"
 #include "UniqueptrDeleteReleaseCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<FunctionSizeCheck>(
         "readability-function-size");
     CheckFactories.registerCheck<IdentifierNamingCheck>(
         "readability-identifier-naming");
     CheckFactories.registerCheck<ImplicitBoolCastCheck>(
         "readability-implicit-bool-cast");
     CheckFactories.registerCheck<InconsistentDeclarationParameterNameCheck>(
         "readability-inconsistent-declaration-parameter-name");
+    CheckFactories.registerCheck<MisplacedArrayIndexCheck>(
+        "readability-misplaced-array-index");
     CheckFactories.registerCheck<StaticDefinitionInAnonymousNamespaceCheck>(
         "readability-static-definition-in-anonymous-namespace");
     CheckFactories.registerCheck<readability::NamedParameterCheck>(
         "readability-named-parameter");
     CheckFactories.registerCheck<RedundantControlFlowCheck>(
         "readability-redundant-control-flow");
     CheckFactories.registerCheck<RedundantSmartptrGetCheck>(
         "readability-redundant-smartptr-get");

docs/ReleaseNotes.rst

- New `cppcoreguidelines-special-member-functions
   Flags classes where some, but not all, special member functions are user-defined.
 
 - New `performance-inefficient-string-concatenation
   <http://clang.llvm.org/extra/clang-tidy/checks/performance-inefficient-string-concatenation.html>`_ check
 
   This check warns about the performance overhead arising from concatenating
   strings using the ``operator+``, instead of ``operator+=``.
 
+- New `readability-misplaced-array-index
+  <http://clang.llvm.org/extra/clang-tidy/checks/readability-misplaced-array-index.html>`_ check
+
+  Warns when there is array index before the [] instead of inside it.
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

docs/clang-tidy/checks/list.rst

.. toctree::
    readability-braces-around-statements
    readability-container-size-empty
    readability-deleted-default
    readability-else-after-return
    readability-function-size
    readability-identifier-naming
    readability-implicit-bool-cast
    readability-inconsistent-declaration-parameter-name
+   readability-misplaced-array-index
    readability-named-parameter
    readability-redundant-control-flow
    readability-redundant-smartptr-get
    readability-redundant-string-cstr
    readability-redundant-string-init
    readability-simplify-boolean-expr
    readability-static-definition-in-anonymous-namespace
    readability-uniqueptr-delete-release

docs/clang-tidy/checks/readability-misplaced-array-index.rst

+.. title:: clang-tidy - readability-misplaced-array-index
+
+readability-misplaced-array-index
+=================================
+
+This check warns for unusual array index syntax.
+
+The following code has unusual array index syntax:
+
+.. code:: c++
+
+  void f(int *x, int y)
+  {
+    y[x] = 0;
+  }
+
+becomes
+
+.. code:: c++
+
+  void f(int *x, int y)
+  {
+    x[y] = 0;
+  }
+
+The check warns about such unusual syntax for readability reasons:
+ * There are programmers that are not familiar with this unusual syntax.
+ * It is possible that variables are mixed up.
+

test/clang-tidy/readability-misplaced-array-index.cpp

+// RUN: %check_clang_tidy %s readability-misplaced-array-index %t
+
+#define ABC  "abc"
+
+struct XY { int *x; int *y; };
+
+void dostuff(int);
+
+void unusualSyntax(int *x, struct XY *xy)
+{
+  10[x] = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unusual array index syntax, usually the index is inside the []
+  // CHECK-FIXES: x[10] = 0;
+
+  10[xy->x] = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unusual array index syntax, usually the index is inside the []
+  // CHECK-FIXES: xy->x[10] = 0;
+
+  dostuff(1["abc"]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: unusual array index syntax, usually the index is inside the []
+  // CHECK-FIXES:  dostuff("abc"[1]);
+
+  dostuff(1[ABC]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: unusual array index syntax, usually the index is inside the []
+  // No fixit, we don't want to replace with "abc"[1]
+}
+
+void normalSyntax(int *x)
+{
+  x[10] = 0;
+  dostuff(0[0 + ABC]);
+}