[cfe-commits] [Patch] Suggest typo corrections for implicit function decls

[Richard Smith]

Hi Hans,

On Sat, November 26, 2011 19:29, Hans Wennborg wrote:
> I noticed that C programmers don't get to enjoy all of Clang's typo
> corrections: a mistyped function call becomes an implicit function
> declaration (with a warning). The attached patch adds typo correction to the
> already existing warnings.
>
> Please take a look and let me know if it's okay for me to land this.

Sorry for the delay. Some comments below.

diff --git a/lib/Sema/SemaDecl.cpp b/lib/Sema/SemaDecl.cpp
index cd2f071..4d45e32 100644
--- a/lib/Sema/SemaDecl.cpp
+++ b/lib/Sema/SemaDecl.cpp
@@ -7262,13 +7262,49 @@ NamedDecl
*Sema::ImplicitlyDefineFunction(SourceLocation Loc,
     return Pos->second;
   }

+  // See if we can find a typo correction.
+  TypoCorrection Corrected;
+  FunctionDecl* Func = NULL;

Please put the space to the right of the *, not to the left. Also, the null
pointer constant is written as 0 more frequently than as NULL in Sema.

+  std::string CorrectedStr;
+  std::string CorrectedQuotedStr;
+  if (S && (Corrected = CorrectTypo(DeclarationNameInfo(&II, Loc),
+                                    LookupOrdinaryName, S, 0))) {
+    // Since this is an implicit function declaration, we are only
+    // interested in a potential typo for a function name.
+    if ((Func = dyn_cast_or_null<FunctionDecl>(
+            Corrected.getCorrectionDecl()))) {
+      CorrectedStr = Corrected.getAsString(getLangOptions());
+      CorrectedQuotedStr = Corrected.getQuoted(getLangOptions());
+    }
+  }
+
   // Extension in C99.  Legal in C90, but warn about it.
-  if (II.getName().startswith("__builtin_"))
-    Diag(Loc, diag::warn_builtin_unknown) << &II;
-  else if (getLangOptions().C99)
-    Diag(Loc, diag::ext_implicit_function_decl) << &II;
-  else
-    Diag(Loc, diag::warn_implicit_function_decl) << &II;
+  // If we found a typo correction, then suggest that.
+  if (Func) {
+    unsigned diagnostic_kind;
+    if (II.getName().startswith("__builtin_"))
+      diagnostic_kind = diag::warn_builtin_unknown_suggest;
+    else if (getLangOptions().C99)
+      diagnostic_kind = diag::ext_implicit_function_decl_suggest;
+    else
+      diagnostic_kind = diag::warn_implicit_function_decl_suggest;
+
+    Diag(Loc, diagnostic_kind) << &II << CorrectedQuotedStr
+      << FixItHint::CreateReplacement(Loc, CorrectedStr);

Please move this fix-it to a separate note (at least for the implicit function
declaration case -- see below). When a fix-it is issued on a warning or an
error, we must recover as if the fixed code had been parsed (and for a
warning, this essentially means the fix-it must not change the semantics).

+
+    if (Func->getLocation().isValid())
+      Diag(Func->getLocation(), diag::note_previous_decl) << CorrectedQuotedStr;
+  } else {
+    unsigned diagnostic_kind;
+    if (II.getName().startswith("__builtin_"))
+      diagnostic_kind = diag::warn_builtin_unknown;
+    else if (getLangOptions().C99)
+      diagnostic_kind = diag::ext_implicit_function_decl;
+    else
+      diagnostic_kind = diag::warn_implicit_function_decl;
+
+    Diag(Loc, diagnostic_kind) << &II;
+  }

   // Set a Declarator for the implicit definition: int foo();
   const char *Dummy;
diff --git a/test/Misc/warning-flags.c b/test/Misc/warning-flags.c
index 5fc48cd..87a982f 100644
--- a/test/Misc/warning-flags.c
+++ b/test/Misc/warning-flags.c
@@ -17,7 +17,7 @@ This test serves two purposes:

 The list of warnings below should NEVER grow.  It should gradually shrink to 0.

-CHECK: Warnings without flags (274):
+CHECK: Warnings without flags (275):

New warnings without flags should not be added (not even if you're just
splitting a warning in two -- please add a flag; we all have taxes to pay).
However, it strikes me that a Warning, DefaultError message with no -W flag,
like this one, is a strange construct indeed, and it should probably simply be
changed into an Error. With that done, you can keep the fix-it on the error
for the __builtin_ case, if you also recover as if the user had typed the
correct builtin name.

 CHECK-NEXT:   ext_anon_param_requires_type_specifier
 CHECK-NEXT:   ext_anonymous_struct_union_qualified
 CHECK-NEXT:   ext_array_init_copy
@@ -129,6 +129,7 @@ CHECK-NEXT:   warn_bitfield_width_exceeds_type_size
 CHECK-NEXT:   warn_bool_switch_condition
 CHECK-NEXT:   warn_braces_around_scalar_init
 CHECK-NEXT:   warn_builtin_unknown
+CHECK-NEXT:   warn_builtin_unknown_suggest
 CHECK-NEXT:   warn_c_kext
 CHECK-NEXT:   warn_call_to_pure_virtual_member_function_from_ctor_dtor
 CHECK-NEXT:   warn_call_wrong_number_of_arguments
diff --git a/test/Sema/builtins.c b/test/Sema/builtins.c
index 64efefc..92a49bd 100644
--- a/test/Sema/builtins.c
+++ b/test/Sema/builtins.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -triple=i686-apple-darwin9
+// RUN: %clang_cc1 %s -fsyntax-only -verify -pedantic -fno-spell-checking
-triple=i686-apple-darwin9

Rather than turning off spell-checking, it'd be better to update the test to
expect the new output (and add more cases here if we've lost some coverage).

 // This test needs to set the target because it uses __builtin_ia32_vec_ext_v4si

 int test1(float a, int b) {
diff --git a/test/Sema/implicit-decl.c b/test/Sema/implicit-decl.c
index f455977..f4b9016 100644
--- a/test/Sema/implicit-decl.c
+++ b/test/Sema/implicit-decl.c
@@ -3,6 +3,8 @@
 typedef int int32_t;
 typedef unsigned char Boolean;

+extern int printf(__const char *__restrict __format, ...); //
expected-note{{'printf' declared here}}
+
 void func() {
    int32_t *vector[16];
    const char compDesc[16 + 1];
@@ -10,6 +12,10 @@ void func() {
    if (_CFCalendarDecomposeAbsoluteTimeV(compDesc, vector, compCount)) { //
expected-note {{previous implicit declaration is here}} \
          expected-warning {{implicit declaration of function
'_CFCalendarDecomposeAbsoluteTimeV' is invalid in C99}}
    }
+
+   printg("Hello, World!\n"); // expected-warning{{implicit declaration of
function
'printg' is invalid in C99; did you mean 'printf'?}}
+
+  __builtin_is_les(1, 3); // expected-error{{use of unknown builtin
'__builtin_is_les'; did you mean '__builtin_is_less'?}}
 }
 Boolean _CFCalendarDecomposeAbsoluteTimeV(const char *componentDesc, int32_t
**vector, int32_t count) { // expected-error{{conflicting types for
'_CFCalendarDecomposeAbsoluteTimeV'}}
  return 0;

Could you add a test for the C89 case, please?

Thanks,
Richard