[Diagnostics] Check for misleading pointer declarations

[xbolva00]

Summary

void foo() {

char* a, b; // warn that b is not a pointer type as someone may expect

}

Fixes PR39150 (also I see a request for this checker on GCC Bugzilla)

[xbolva00]

• Avoid possible crash

[erik.pilkington]

Thanks for working on this!

include/clang/Basic/DiagnosticSemaKinds.td

318–320

I think this should really be off by default, this is a stylistic warning that would be really noisy for existing projects. Also, I think the text might be better spelled as "declaring a variable of type %0; did you mean to declare a pointer?". Also, -Wmisleading-declarations doesn't really describe what this warning diagnoses. This is C, so there is plenty of ways to write a misleading declaration ;). Maybe -Wmisleading-pointer-declarator?

lib/Sema/SemaStmt.cpp

81

The null check here is redundant, new (Context) should never return nullptr.

[xbolva00]

• Addressed review comments

[xbolva00]

Any futher comments?

[Rakete1111]

There's a false positive.

int **A, *B; // false positive: declaring a variable of type 'int *'; did you mean to declare a pointer?

And IMO this should also warn:

int **A, B; // no warning currently

[xbolva00]

Yes, I am gonna work on it after I finish other patch :)

[rsmith]

I think we need some documented guidelines on what is and is not an appropriate warning for Clang. I think it's reasonable to warn in cases where we are confident that the code means something other than what the programmer or a later reader is likely to expect; it's less obvious to me whether this warning fits that categorization: while a lot of people are confused by C/C++'s multi-declarator declaration rules, there are also many people who are not, and who use this kind of coding pattern (if you look at, say, the ffmpeg source code, you can find all sorts of things like this: char *p, buf[AV_HASH_MAX_SIZE * 2 + 64] = { 0 };, int *fds = NULL, fdsnum, fdsidx;, int *tmp, i, len;, int *p, vmax, v, n, i, j, k, code;, and so on).

Generally in the cases where we do accept such warnings on valid code that might nonetheless mean something different from what was intended, or be misread by people who have not internalized the relevant language rules, there is a simple syntactic workaround that suppresses the diagnostic, and we suggest that workaround in a note attached to the warning. (For example, for assignment in an if condition, we suggest adding another pair of parentheses, and likewise we suggest adding parentheses for an && expression as an operand of an || expression.) Is there a similar syntactic workaround we can suggest here? For example, would

int *ptr, (a), b, c;

... suppress the warning for a?

[rsmith]

In D52791#1254559, @rsmith wrote:

int *ptr, (a), b, c;

Another possible heuristic for suppressing the check: only warn if there is whitespace before the identifier. So:

int* a, b; // warning
int *a, b; // no warning

lib/Sema/SemaStmt.cpp

82–95

This is an inappropriate way of performing the check. Instead, you should look at the syntactic form of the declarator while parsing it.

[xbolva00]

void test(void) {

int *a;
int* b;

}

TranslationUnitDecl
`-FunctionDecl <line:4:1, line:7:1> line:4:6 test 'void ()'

`-CompoundStmt <col:17, line:7:1>
  |-DeclStmt <line:5:5, col:11>
  | `-VarDecl <col:5, col:10> col:10 a 'int *'
  `-DeclStmt <line:6:5, col:11>
    `-VarDecl <col:5, col:10> col:10 b 'int *'

Why not 'int*' in ast dump?

[xbolva00]

This shouldn't be implemented here, better choice is clang-tidy. Closing this revision.

include/clang/Basic/DiagnosticSemaKinds.td

def warn_parens_disambiguated_as_function_declaration : Warning<
   "parentheses were disambiguated as a function declaration">,
   InGroup<VexingParse>;
 def warn_parens_disambiguated_as_variable_declaration : Warning<
   "parentheses were disambiguated as redundant parentheses around declaration "
   "of variable named %0">, InGroup<VexingParse>;
 def warn_redundant_parens_around_declarator : Warning<
   "redundant parentheses surrounding declarator">,
   InGroup<DiagGroup<"redundant-parens">>, DefaultIgnore;
+def warn_misleading_var_type_decl : Warning<
+  "misleading variable declaration, supposed to be a pointer type instead ?">,
+  InGroup<DiagGroup<"misleading-declarations">>;
 def note_additional_parens_for_variable_declaration : Note<
   "add a pair of parentheses to declare a variable">;
 def note_raii_guard_add_name : Note<
   "add a variable name to declare a %0 initialized with %1">;
 def note_function_style_cast_add_parentheses : Note<
   "add enclosing parentheses to perform a function-style cast">;
 def note_remove_parens_for_variable_declaration : Note<
   "remove parentheses to silence this warning">;

lib/Sema/SemaStmt.cpp

 
 StmtResult Sema::ActOnDeclStmt(DeclGroupPtrTy dg, SourceLocation StartLoc,
                                SourceLocation EndLoc) {
   DeclGroupRef DG = dg.get();
 
   // If we have an invalid decl, just return an error.
   if (DG.isNull()) return StmtError();
 
-  return new (Context) DeclStmt(DG, StartLoc, EndLoc);
+  DeclStmt *DS = new (Context) DeclStmt(DG, StartLoc, EndLoc);
+  if (DS) {
+    VarDecl *FirstVarDecl = nullptr;
+    for (auto *DI : DS->decls()) {
+      VarDecl *VD = dyn_cast<VarDecl>(DI);
+      if (!FirstVarDecl) {
+        if (!VD || !VD->getType()->isPointerType())
+          break;
+        FirstVarDecl = VD;
+      } else {
+        if (VD->getType() == FirstVarDecl->getType()->getPointeeType())
+          Diag(VD->getLocation(), diag::warn_misleading_var_type_decl);
+      }
+    }
+  }
+
+  return DS;
 }
 
 void Sema::ActOnForEachDeclStmt(DeclGroupPtrTy dg) {
   DeclGroupRef DG = dg.get();
 
   // If we don't have a declaration, or we have an invalid declaration,
   // just return.
   if (DG.isNull() || !DG.isSingleDecl())

test/Sema/misleading-var-type-decl.c

+// RUN: %clang_cc1 %s -verify -Wmisleading-declarations -fsyntax-only
+
+void test(void) {
+    int *a, b; //  expected-warning {{misleading variable declaration, supposed to be a pointer type instead ?}}
+    int x, *y;
+    int *g, *h, *i, **j;
+
+    for (int *a, b;;) { } // expected-warning {{misleading variable declaration, supposed to be a pointer type instead ?}}
+    for (int x, *y;;) { }
+    for (int *g, *h, *i, **j;;) { }
+}