Extend -Wnonportable-include-path with a warning about trailing whitespace or dots #96960

tobias-mayer · 2024-06-27T20:14:31Z

Resolves #96064

Extends -Wnonportable-include-path with a warning about trailing whitespace or dots in include paths.

github-actions · 2024-06-27T20:14:48Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-06-27T20:15:18Z

@llvm/pr-subscribers-clang

Author: Tobias Mayer (tobias-mayer)

Changes

Resolves #96064

Extends -Wnonportable-include-path with a warning about trailing whitespace or dots in include paths.

Full diff: https://github.com/llvm/llvm-project/pull/96960.diff

4 Files Affected:

(modified) clang/include/clang/Basic/DiagnosticGroups.td (+3)
(modified) clang/include/clang/Basic/DiagnosticLexKinds.td (+5-1)
(modified) clang/lib/Lex/PPDirectives.cpp (+17)
(added) clang/test/Preprocessor/nonportable-include-trailing-whitespace.c (+11)

diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 9b37d4bd3205b..364a2387d3d50 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1535,3 +1535,6 @@ def BitIntExtension : DiagGroup<"bit-int-extension">;
 
 // Warnings about misuse of ExtractAPI options.
 def ExtractAPIMisuse : DiagGroup<"extractapi-misuse">;
+
+// Warnings about non-portable include paths
+def NonportablePath : DiagGroup<"nonportable-include-path">;
diff --git a/clang/include/clang/Basic/DiagnosticLexKinds.td b/clang/include/clang/Basic/DiagnosticLexKinds.td
index 12d7b8c0205ee..cf6e448d8fc36 100644
--- a/clang/include/clang/Basic/DiagnosticLexKinds.td
+++ b/clang/include/clang/Basic/DiagnosticLexKinds.td
@@ -361,7 +361,11 @@ class NonportablePath  : Warning<
   "non-portable path to file '%0'; specified path differs in case from file"
   " name on disk">;
 def pp_nonportable_path : NonportablePath,
-  InGroup<DiagGroup<"nonportable-include-path">>;
+  InGroup<NonportablePath>;
+def pp_nonportable_path_trailing_whitespace : Warning<
+  "non-portable path to file '%0'; specified path contains trailing"
+  "whitespace or dots">,
+  InGroup<NonportablePath>;
 def pp_nonportable_system_path : NonportablePath, DefaultIgnore,
   InGroup<DiagGroup<"nonportable-system-include-path">>;
 
diff --git a/clang/lib/Lex/PPDirectives.cpp b/clang/lib/Lex/PPDirectives.cpp
index a53540b12dee6..919e3edc18032 100644
--- a/clang/lib/Lex/PPDirectives.cpp
+++ b/clang/lib/Lex/PPDirectives.cpp
@@ -2098,6 +2098,23 @@ OptionalFileEntryRef Preprocessor::LookupHeaderIncludeOrImport(
     const FileEntry *LookupFromFile, StringRef &LookupFilename,
     SmallVectorImpl<char> &RelativePath, SmallVectorImpl<char> &SearchPath,
     ModuleMap::KnownHeader &SuggestedModule, bool isAngled) {
+
+  // Check for trailing whitespace or dots in the include path.
+  // This must be done before looking up the file, as Windows will still
+  // find the file even if there are trailing dots or whitespace.
+  size_t TrailingPos = Filename.find_last_not_of(" .");
+  if (TrailingPos != StringRef::npos && TrailingPos < Filename.size() - 1) {
+    StringRef TrimmedFilename = Filename.rtrim(" .");
+
+    auto Hint = isAngled
+                    ? FixItHint::CreateReplacement(
+                          FilenameRange, "<" + TrimmedFilename.str() + ">")
+                    : FixItHint::CreateReplacement(
+                          FilenameRange, "\"" + TrimmedFilename.str() + "\"");
+    Diag(FilenameTok, diag::pp_nonportable_path_trailing_whitespace)
+        << Filename << TrimmedFilename << Hint;
+  }
+
   auto DiagnoseHeaderInclusion = [&](FileEntryRef FE) {
     if (LangOpts.AsmPreprocessor)
       return;
diff --git a/clang/test/Preprocessor/nonportable-include-trailing-whitespace.c b/clang/test/Preprocessor/nonportable-include-trailing-whitespace.c
new file mode 100644
index 0000000000000..6def70a59d2e1
--- /dev/null
+++ b/clang/test/Preprocessor/nonportable-include-trailing-whitespace.c
@@ -0,0 +1,11 @@
+// REQUIRES: case-insensitive-filesystem
+// RUN: %clang_cc1 -Wall %s
+
+#include "empty_file_to_include.h " // expected-warning {{non-portable path}}
+#include "empty_file_to_include.h." // expected-warning {{non-portable path}}
+#include "empty_file_to_include.h       " // expected-warning {{non-portable path}}
+#include "empty_file_to_include.h......." // expected-warning {{non-portable path}}
+#include "empty_file_to_include.h . . . " // expected-warning {{non-portable path}}
+#include "empty_file_to_include.h.. .. " // expected-warning {{non-portable path}}
+
+#include "empty_file_to_include.h" // No warning
\ No newline at end of file

AaronBallman

Thank you for working on this! Please be sure to add a release note to clang/docs/ReleaseNotes.rst so users know about the improved diagnostics.

AaronBallman · 2024-06-28T11:31:50Z

clang/include/clang/Basic/DiagnosticLexKinds.td

@@ -361,7 +361,11 @@ class NonportablePath  : Warning<
  "non-portable path to file '%0'; specified path differs in case from file"
  " name on disk">;
 def pp_nonportable_path : NonportablePath,
-  InGroup<DiagGroup<"nonportable-include-path">>;
+  InGroup<NonportablePath>;


I would be nice to give the diagnostic group a different identifier (the name we expose to users is fine!) so that we don't have this weird naming conflict between the diagnostic group and the class NonportablePath definition a few lines up from here.

AaronBallman · 2024-06-28T11:32:38Z

clang/test/Preprocessor/nonportable-include-trailing-whitespace.c

+#include "empty_file_to_include.h . . . " // expected-warning {{non-portable path}}
+#include "empty_file_to_include.h.. .. " // expected-warning {{non-portable path}}
+
+#include "empty_file_to_include.h" // No warning


Please add a newline to the end of the file.

zmodem

Nice!

zmodem · 2024-06-28T12:53:43Z

clang/include/clang/Basic/DiagnosticGroups.td

@@ -1543,3 +1543,6 @@ def ExtractAPIMisuse : DiagGroup<"extractapi-misuse">;
 // Warnings about using the non-standard extension having an explicit specialization
 // with a storage class specifier.
 def ExplicitSpecializationStorageClass : DiagGroup<"explicit-specialization-storage-class">;
+
+// Warnings about non-portable include paths


nit: period at the end

zmodem · 2024-06-28T12:57:43Z

clang/include/clang/Basic/DiagnosticLexKinds.td

+  InGroup<NonportablePath>;
+def pp_nonportable_path_trailing_whitespace : Warning<
+  "non-portable path to file '%0'; specified path contains trailing"
+  "whitespace or dots">,


Unless it would complicate the code too much, could we use %select{whitespace|dots} here to make the diagnostic specific to whether it's a trailing space or dot?

Sounds good. How should we handle a case like #include "test.h.. .. " where we have both? Should we add a third option complaining about both or just pick whatever is the last character?

Good point. I think having both trailing dot and whitespace should be rare in practice, so just picking whichever comes last sounds fine to me.

zmodem · 2024-06-28T13:02:05Z

clang/lib/Lex/PPDirectives.cpp

+  if (TrailingPos != StringRef::npos && TrailingPos < Filename.size() - 1) {
+    StringRef TrimmedFilename = Filename.rtrim(" .");
+
+    auto Hint = isAngled


Since you're providing fixits, it would be nice to have tests for them. Look at clang/test/Parser/extra-semi.cpp for an example.

zmodem · 2024-06-28T13:03:21Z

clang/test/Preprocessor/nonportable-include-trailing-whitespace.c

+// REQUIRES: case-insensitive-filesystem
+// RUN: %clang_cc1 -Wall %s
+
+#include "empty_file_to_include.h " // expected-warning {{non-portable path}}


The expected-warning directives require running clang with the -verify flag.

zmodem · 2024-06-28T13:10:05Z

clang/test/Preprocessor/nonportable-include-trailing-whitespace.c

@@ -0,0 +1,11 @@
+// REQUIRES: case-insensitive-filesystem


Hmm, I think Mac's qualifies as a case-insensitive-filesystem, but I don't think it ignores trailing spaces/dots like Windows does(?).

I wasn't sure about this but I think I could remove that.
The behavior should be as follows:

Linux: You get the new warning and a file not found error.

Windows: You only get the new warning as the file is still found.

Mac: Warning + error just as on Linux. Case sensitivity shouldn't matter in both cases.

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jun 27, 2024

add warning about trailing whitespace or dots in include paths

4118e37

tobias-mayer force-pushed the non-portable-include-whitespace branch from ec07df7 to 4118e37 Compare June 27, 2024 20:32

AaronBallman requested review from rnk, zmodem and AaronBallman June 28, 2024 11:29

AaronBallman reviewed Jun 28, 2024

View reviewed changes

zmodem reviewed Jun 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend -Wnonportable-include-path with a warning about trailing whitespace or dots #96960

Extend -Wnonportable-include-path with a warning about trailing whitespace or dots #96960

tobias-mayer commented Jun 27, 2024

github-actions bot commented Jun 27, 2024

llvmbot commented Jun 27, 2024

AaronBallman left a comment

AaronBallman Jun 28, 2024

AaronBallman Jun 28, 2024

zmodem left a comment

zmodem Jun 28, 2024

zmodem Jun 28, 2024

tobias-mayer Jun 28, 2024

zmodem Jun 28, 2024

zmodem Jun 28, 2024

zmodem Jun 28, 2024

zmodem Jun 28, 2024

tobias-mayer Jun 28, 2024

Extend -Wnonportable-include-path with a warning about trailing whitespace or dots #96960

Are you sure you want to change the base?

Extend -Wnonportable-include-path with a warning about trailing whitespace or dots #96960

Conversation

tobias-mayer commented Jun 27, 2024

github-actions bot commented Jun 27, 2024

llvmbot commented Jun 27, 2024

AaronBallman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zmodem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment