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

476 ↗

(On Diff #162779)

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

1885–1886 ↗

(On Diff #162779)

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 ↗

(On Diff #163132)

I'd pass false directly here...

1882 ↗

(On Diff #163132)

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

1887–1888 ↗

(On Diff #163132)

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

1890–1891 ↗

(On Diff #163132)

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

test/Preprocessor/include-likely-typo.c

1–2 ↗

(On Diff #163132)

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

428–429 ↗

(On Diff #165162)

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

1901 ↗

(On Diff #165162)

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

1901 ↗

(On Diff #165162)

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

1891 ↗

(On Diff #165311)

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

1907 ↗

(On Diff #165311)

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.

cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td

def err_pp_pragma_hdrstop_not_seen : Error<
   DefaultFatal;
 def warn_pp_macro_def_mismatch_with_pch : Warning<
   "definition of macro %0 does not match definition in precompiled header">,
   InGroup<ClangClPch>;
 def warn_pp_hdrstop_filename_ignored : Warning<
   "#pragma hdrstop filename not supported, "
   "/Fp can be used to specify precompiled header filename">,
   InGroup<ClangClPch>;
-def err_pp_file_not_found_not_fatal : Error<
+def err_pp_file_not_found_angled_include_not_fatal : Error<
   "'%0' file not found with <angled> include; use \"quotes\" instead">;
+def err_pp_file_not_found_typo_not_fatal
+    : Error<"'%0' file not found, did you mean '%1'?">;
 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<

cfe/trunk/lib/Lex/PPDirectives.cpp

if (!SuppressIncludeNotFoundError) {
         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) <<
+          Diag(FilenameTok, diag::err_pp_file_not_found_angled_include_not_fatal) <<
             Filename <<
             FixItHint::CreateReplacement(Range, "\"" + Filename.str() + "\"");
         }
       }
 
+      // Check for likely typos due to leading or trailing non-isAlphanumeric
+      // characters
+      if (!File) {
+        StringRef OriginalFilename = Filename;
+        while (!isAlphanumeric(Filename.front())) {
+          Filename = Filename.drop_front();
+        }
+        while (!isAlphanumeric(Filename.back())) {
+          Filename = Filename.drop_back();
+        }
+
+        File = LookupFile(
+            FilenameLoc,
+            LangOpts.MSVCCompat ? NormalizedPath.c_str() : Filename, isAngled,
+            LookupFrom, LookupFromFile, CurDir,
+            Callbacks ? &SearchPath : nullptr,
+            Callbacks ? &RelativePath : nullptr, &SuggestedModule, &IsMapped);
+        if (File) {
+          SourceRange Range(FilenameTok.getLocation(), CharEnd);
+          auto Hint = isAngled ? FixItHint::CreateReplacement(
+                                     Range, "<" + Filename.str() + ">")
+                               : FixItHint::CreateReplacement(
+                                     Range, "\"" + Filename.str() + "\"");
+          Diag(FilenameTok, diag::err_pp_file_not_found_typo_not_fatal)
+              << OriginalFilename << Filename << Hint;
+        }
+      }
+
       // 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 (usingPCHWithThroughHeader() && SkippingUntilPCHThroughHeader) {

cfe/trunk/test/Preprocessor/empty_file_to_include.h

+
+#ifndef EMPTY_FILE_TO_INCLUDE_H
+#define EMPTY_FILE_TO_INCLUDE_H
+
+// empty file
+
+#endif

cfe/trunk/test/Preprocessor/include-likely-typo.c

+// RUN: %clang_cc1 %s -verify
+
+#include "<empty_file_to_include.h>" // expected-error {{'<empty_file_to_include.h>' file not found, did you mean 'empty_file_to_include.h'?}}