Diagnose likely typos in include statements

[christylee]

Summary

When someone writes

#include "<some_file.h>"

or

#include " some_file.h "

the compiler returns "file not fond..." with fonts and quotes that may make it hard to see there are excess quotes or surprising bytes in the filename. Assuming that files are usually logically named and start and end with an alphanumeric character, we can check for the file's existence by stripping the non-alphanumeric leading or trailing characters. If the file is found, emit a non-fatal error with a fixithint.

[modocache]

This looks good to me, but maybe some people who've modified this part of the codebase before could review this as well? @aaron.ballman added a fix-it for angled/quoted strings in rL160406, and more recently @erikjv modified some of this code in rL285057. If anyone could suggest other reviewers that would be great as well!

[aaron.ballman]

include/clang/Basic/DiagnosticLexKinds.td

478

This seems like the wrong warning group for this diagnostic as it doesn't relate to pragmas. However, we may not want this exposed as a separate warning, either.

lib/Lex/PPDirectives.cpp

1888–1889

I don't think we want to produce two different diagnostics for the same line of code. What if, instead, we augment the error diagnostic so that it can produce additional information in this case?

def err_pp_file_not_found : Error<"'%0' file not found%select{|, possibly due to leading or trailing non-alphanumeric characters in the file name}1">, DefaultFatal;

(or something along those lines.)

test/Frontend/include-likely-typo.c

1–4 ↗

(On Diff #162779)

I think we want this test to live in test/Preprocessor instead of test/Frontend.

[christylee]

Merged warning with existing file_not_found_error.

[aaron.ballman]

lib/Lex/PPDirectives.cpp

1876

I'd pass false directly here...

1882

... and lower isFileNotFoundLikelyTypo to here and rename it IsFileNotFoundLikelyTypo to meet our usual naming conventions...

1887–1888

...then remove this call to Diag()...

1890–1891

As it stands, this is going to diagnose the same issue twice.

test/Preprocessor/include-likely-typo.c

1–2

I don't think this is the correct formulation for the test, which explains why it's passing for you. I think the test should be:

// RUN: %clang_cc1 %s -verify

#include "<hello.h>" // expected-error {{'<hello.h>' file not found, possibly due to leading or trailing non-alphanumeric characters in the file name}}

[rsmith]

Instead of guessing whether the corrected filename would be valid, why not strip off the leading and trailing non-alphanumeric characters, look up the resulting filename, and find out? If we did that, then not only could we be a lot more confident that we'd found the file that was intended, but we could also recover from the error by including the trimmed filename.

[aaron.ballman]

In D51333#1219938, @rsmith wrote:

Instead of guessing whether the corrected filename would be valid, why not strip off the leading and trailing non-alphanumeric characters, look up the resulting filename, and find out? If we did that, then not only could we be a lot more confident that we'd found the file that was intended, but we could also recover from the error by including the trimmed filename.

I was contemplating that approach, but I was also a bit worried about reaching out to the filesystem again. We already reach out once for angled vs quoted include path issues, and I don't think these two tests can be combined easily. However, this is on an error path and so performance isn't super important, so I think I've convinced myself this is a good approach to try.

[christylee]

In D51333#1219938, @rsmith wrote:

Instead of guessing whether the corrected filename would be valid, why not strip off the leading and trailing non-alphanumeric characters, look up the resulting filename, and find out? If we did that, then not only could we be a lot more confident that we'd found the file that was intended, but we could also recover from the error by including the trimmed filename.

Should we lookup the stripped filename and then error out on a warning along the lines of "'<hello,h>' file not found, did you mean 'hello,h'", or attempt to continue to compile using the file we found (i.e. "hello.h"). I'm learning towards the former because I'm worried the latter might be "too clever", in case the developers actually think <hello.h> and hello.h are two different files.

[rsmith]

In D51333#1220878, @christylee wrote:

In D51333#1219938, @rsmith wrote:

Instead of guessing whether the corrected filename would be valid, why not strip off the leading and trailing non-alphanumeric characters, look up the resulting filename, and find out? If we did that, then not only could we be a lot more confident that we'd found the file that was intended, but we could also recover from the error by including the trimmed filename.

Should we lookup the stripped filename and then error out on a warning along the lines of "'<hello,h>' file not found, did you mean 'hello,h'", or attempt to continue to compile using the file we found (i.e. "hello.h"). I'm learning towards the former because I'm worried the latter might be "too clever", in case the developers actually think <hello.h> and hello.h are two different files.

I think we should issue a (non-fatal) error along the lines of "<hello.h> not found; did you mean "hello.h"?" with a FixItHint, and carry on by including the file we believe they meant. The chance of hitting a false positive by doing this seems extremely remote to me, so I'm not worried about it being "too clever" and doing the wrong thing. (You'd not only need for the developer to intend to use a file whose name starts or ends with an odd character, but also for that file to not exist and a file whose name is the same but with said character(s) removed to actually exist.)

[christylee]

Emit non-fatal error for typo if file exists.

[rsmith]

Thanks, some comments but the approach here looks great.

include/clang/Basic/DiagnosticLexKinds.td

420–421

Our usual convention is to phrase this as "'%0' file not found; did you mean '%1'?". I don't think we need to describe the exact process we used to figure out the intended filename here; the user should be able to compare the names before and after to determine what we changed.

lib/Lex/PPDirectives.cpp

1893

You're passing in false for isAngled here, and producing a double-quoted replacement below. Is that intentional? I would expect that we would preserve the form of the header-name (quoted or angled) and suggest a replacement with the same form.

[sammccall]

(No action needed, but curious on your thoughts)

One thing I'd like to do sometime is add code completion of filenames in #include directives.
This would mean listing the relevant directories from the include path. (VFS is slow for this today, but I have a patch out for it).

Maybe it would make sense to do that here and use edit-distance to pick the suggestion, like typo correction elsewhere?
Could be a way to extend this patch's behavior in the future.

[christylee]

lib/Lex/PPDirectives.cpp

1893

My mistake, thanks for catching it!

[christylee]

Addressed @rsmith 's comments.

[rsmith]

Looks good (with a couple of minor code style adjustments). Do you need someone to commit this for you?

In D51333#1233042, @sammccall wrote:

One thing I'd like to do sometime is add code completion of filenames in #include directives.
This would mean listing the relevant directories from the include path. (VFS is slow for this today, but I have a patch out for it).

Maybe it would make sense to do that here and use edit-distance to pick the suggestion, like typo correction elsewhere?
Could be a way to extend this patch's behavior in the future.

I agree; I think this would be a very interesting extension for this patch if someone feels motivated.

lib/Lex/PPDirectives.cpp

1883

Nit: please spell out the type here, because it's not obvious from the initializer. Please capitalize the variable name.

1899

Nit: please capitalize this variable name.

[xbolva00]

While here, I would love to have fixint hints for unknown functions.

E.g.
std::forward somewhere in code -> fixit: "try to add #include <utility>"

[christylee]

@rsmith , thanks for the review, I fixed the variable capitalization. If you could land it for me that'll be awesome!

In D51333#1233042, @sammccall wrote:

One thing I'd like to do sometime is add code completion of filenames in #include directives.
This would mean listing the relevant directories from the include path. (VFS is slow for this today, but I have a patch out for it).

Maybe it would make sense to do that here and use edit-distance to pick the suggestion, like typo correction elsewhere?
Could be a way to extend this patch's behavior in the future.

I also think it's a good idea. I'm happy to take a crack at it!

[lebedev.ri]

The tests seem to have disappeared form the diff.

[christylee]

Added tests.

include/clang/Basic/DiagnosticLexKinds.td

 def note_macro_here : Note<"macro %0 defined here">;
 
 def err_pp_opencl_variadic_macros :
   Error<"variadic macros not supported in OpenCL">;
 
 def err_pp_invalid_directive : Error<"invalid preprocessing directive">;
 def err_pp_directive_required : Error<
   "%0 must be used within a preprocessing directive">;
-def err_pp_file_not_found : Error<"'%0' file not found">, DefaultFatal;
+def err_pp_file_not_found : Error<
+  "'%0' file not found%select{|, possibly due to leading or trailing "
+  "non-alphanumeric characters in the file name}1">, DefaultFatal;
 def err_pp_through_header_not_found : Error<
   "'%0' required for precompiled header not found">, DefaultFatal;
 def err_pp_through_header_not_seen : Error<
   "#include of '%0' not seen while attempting to "
   "%select{create|use}1 precompiled header">, DefaultFatal;
 def warn_pp_macro_def_mismatch_with_pch : Warning<
   "definition of macro %0 does not match definition in precompiled header">,
   InGroup<ClangClPch>;
 def err_pp_file_not_found_not_fatal : Error<
   "'%0' file not found with <angled> include; use \"quotes\" instead">;
 def err_pp_error_opening_file : Error<
   "error opening file '%0': %1">, DefaultFatal;
 def err_pp_empty_filename : Error<"empty filename">;
 def err_pp_include_too_deep : Error<"#include nested too deeply">;
 def err_pp_expects_filename : Error<"expected \"FILENAME\" or <FILENAME>">;
 def err_pp_macro_not_identifier : Error<"macro name must be an identifier">;
 def err_pp_missing_macro_name : Error<"macro name missing">;
 def err_pp_missing_rparen_in_macro_def : Error<
   "missing ')' in macro parameter list">;
 def err_pp_invalid_tok_in_arg_list : Error<
   "invalid token in macro parameter list">;
def warn_has_warning_invalid_option :
    ExtWarn<"__has_warning expected option name (e.g. \"-Wundef\")">,
    InGroup<MalformedWarningCheck>;
 
 def err_pp_identifier_arg_not_identifier : Error<
   "cannot convert %0 token to an identifier">;
 
 def warn_pragma_include_alias_mismatch_angle :
    ExtWarn<"angle-bracketed include <%0> cannot be aliased to double-quoted "
    "include \"%1\"">, InGroup<UnknownPragmas>;
 def warn_pragma_include_alias_mismatch_quote :
    ExtWarn<"double-quoted include \"%0\" cannot be aliased to angle-bracketed "
    "include <%1>">, InGroup<UnknownPragmas>;
 def warn_pragma_include_alias_expected :
    ExtWarn<"pragma include_alias expected '%0'">,
    InGroup<UnknownPragmas>;
 def warn_pragma_include_alias_expected_filename :
    ExtWarn<"pragma include_alias expected include filename">,

lib/Lex/PPDirectives.cpp

if (Callbacks) {
         }
       }
     }
 
     if (!SuppressIncludeNotFoundError) {
       // If the file could not be located and it was included via angle
       // brackets, we can attempt a lookup as though it were a quoted path to
       // provide the user with a possible fixit.
+      bool isFileNotFoundLikelyTypo = false;
       if (isAngled) {
         File = LookupFile(
             FilenameLoc,
             LangOpts.MSVCCompat ? NormalizedPath.c_str() : Filename, false,
             LookupFrom, LookupFromFile, CurDir,
             Callbacks ? &SearchPath : nullptr,
             Callbacks ? &RelativePath : nullptr, &SuggestedModule, &IsMapped);
         if (File) {
           SourceRange Range(FilenameTok.getLocation(), CharEnd);
-          Diag(FilenameTok, diag::err_pp_file_not_found_not_fatal) <<
-            Filename <<
-            FixItHint::CreateReplacement(Range, "\"" + Filename.str() + "\"");
+          Diag(FilenameTok, diag::err_pp_file_not_found_not_fatal)
+              << Filename
+              << FixItHint::CreateReplacement(Range,
+                                              "\"" + Filename.str() + "\"")
+              << isFileNotFoundLikelyTypo;
         }
       }
 
       // If the file is still not found, just go with the vanilla diagnostic
-      if (!File)
-        Diag(FilenameTok, diag::err_pp_file_not_found) << Filename
-                                                       << FilenameRange;
+      if (!File) {
+        // Assuming filename logically starts and end with alphnumeric
+        // character
+        if (!isAlphanumeric(Filename.front()) ||
+            !isAlphanumeric(Filename.back())) {
+          isFileNotFoundLikelyTypo = true;
+          Diag(FilenameTok, diag::err_pp_file_not_found)
+              << Filename << isFileNotFoundLikelyTypo;
+        }
+        Diag(FilenameTok, diag::err_pp_file_not_found)
+            << Filename << FilenameRange << isFileNotFoundLikelyTypo;
+      }
     }
   }
 
   if (usingPCHWithThroughHeader() && SkippingUntilPCHThroughHeader) {
     if (isPCHThroughHeader(File))
       SkippingUntilPCHThroughHeader = false;
     return;
   }
 
   // Should we enter the source file? Set to false if either the source file is
   // known to have no effect beyond its effect on module visibility -- that is,
   // if it's got an include guard that is already defined or is a modular header
   // we've imported or already built.
   bool ShouldEnter = true;
 

lib/Lex/Pragma.cpp

void Preprocessor::HandlePragmaDependency(Token &DependencyTok) {
 
   // Search include directories for this file.
   const DirectoryLookup *CurDir;
   const FileEntry *File =
       LookupFile(FilenameTok.getLocation(), Filename, isAngled, nullptr,
                  nullptr, CurDir, nullptr, nullptr, nullptr, nullptr);
   if (!File) {
     if (!SuppressIncludeNotFoundError)
-      Diag(FilenameTok, diag::err_pp_file_not_found) << Filename;
+      Diag(FilenameTok, diag::err_pp_file_not_found) << Filename << 0;
     return;
   }
 
   const FileEntry *CurFile = getCurrentFileLexer()->getFileEntry();
 
   // If this file is older than the file it depends on, emit a diagnostic.
   if (CurFile && CurFile->getModificationTime() < File->getModificationTime()) {
     // Lex tokens at the end of the message and include them in the message.

test/Preprocessor/include-likely-typo.c

+// RUN: not %clang_cc1 -verify -frewrite-includes
+#include "<hello.h>" @expected-error {{'<hello.h>' file not found, possibly due to leading or trailing non-alphanumeric characters in the file name}}