[clangd] Drop diags from non-written #include.

[hokein]

Summary

This would fix that we show weird diagnostics on random lines of the main file.

[ilya-biryukov]

LGTM, thanks!
We should also merge this into the release branch if it's not too late yet.

clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp

952 ↗

(On Diff #214587)

Could you add a comment describing which diagnostic we do not want to see here and why?
I assume it's main should return int, right?

[hokein]

Add a comment.

[hokein]

@hans is there still any chance to merge this patch into the release?

[hans]

In D66074#1624885, @hokein wrote:

@hans is there still any chance to merge this patch into the release?

I tried to merge this to release_90, but I got a test failure. Does this rely on more patches that need merging? Can you try applying it to the release branch in the monorepo and see what needs fixing?

$ ninja check-clangd
[7/8] Running the Clangd regression tests
llvm-lit: /work/llvm-9/llvm/utils/lit/lit/llvm/config.py:340: note: using clang: /work/llvm-9/build.release/bin/clang
FAIL: Clangd Unit Tests :: ./ClangdTests/IgnoreDiags.FromNonWrittenInclude (368 of 573)
******************** TEST 'Clangd Unit Tests :: ./ClangdTests/IgnoreDiags.FromNonWrittenInclude' FAILED ********************
Note: Google Test filter = IgnoreDiags.FromNonWrittenInclude
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from IgnoreDiags
[ RUN      ] IgnoreDiags.FromNonWrittenInclude
/work/llvm-9/llvm/tools/clang/tools/extra/clangd/unittests/DiagnosticsTests.cpp:945: Failure
Value of: TU.build().getDiagnostics()
Expected: is empty
  Actual: { [./a.h:0:0-0:4] 'main' must return 'int' }, which has 1 element
[  FAILED  ] IgnoreDiags.FromNonWrittenInclude (20 ms)
[----------] 1 test from IgnoreDiags (20 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (21 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] IgnoreDiags.FromNonWrittenInclude

 1 FAILED TEST
Preamble for file /clangd-test/TestTU.cpp cannot be reused. Attempting to rebuild it.
Built preamble of size 201076 for file /clangd-test/TestTU.cpp

********************
Testing Time: 3.92s
********************
Failing Tests (1):
    Clangd Unit Tests :: ./ClangdTests/IgnoreDiags.FromNonWrittenInclude

  Expected Passes    : 569
  Unsupported Tests  : 3
  Unexpected Failures: 1
FAILED: tools/clang/tools/extra/clangd/test/CMakeFiles/check-clangd

[hokein]

In D66074#1626856, @hans wrote:

In D66074#1624885, @hokein wrote:

@hans is there still any chance to merge this patch into the release?

I tried to merge this to release_90, but I got a test failure. Does this rely on more patches that need merging? Can you try applying it to the release branch in the monorepo and see what needs fixing?

$ ninja check-clangd
[7/8] Running the Clangd regression tests
llvm-lit: /work/llvm-9/llvm/utils/lit/lit/llvm/config.py:340: note: using clang: /work/llvm-9/build.release/bin/clang
FAIL: Clangd Unit Tests :: ./ClangdTests/IgnoreDiags.FromNonWrittenInclude (368 of 573)
******************** TEST 'Clangd Unit Tests :: ./ClangdTests/IgnoreDiags.FromNonWrittenInclude' FAILED ********************
Note: Google Test filter = IgnoreDiags.FromNonWrittenInclude
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from IgnoreDiags
[ RUN      ] IgnoreDiags.FromNonWrittenInclude
/work/llvm-9/llvm/tools/clang/tools/extra/clangd/unittests/DiagnosticsTests.cpp:945: Failure
Value of: TU.build().getDiagnostics()
Expected: is empty
  Actual: { [./a.h:0:0-0:4] 'main' must return 'int' }, which has 1 element
[  FAILED  ] IgnoreDiags.FromNonWrittenInclude (20 ms)
[----------] 1 test from IgnoreDiags (20 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (21 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] IgnoreDiags.FromNonWrittenInclude

 1 FAILED TEST
Preamble for file /clangd-test/TestTU.cpp cannot be reused. Attempting to rebuild it.
Built preamble of size 201076 for file /clangd-test/TestTU.cpp

********************
Testing Time: 3.92s
********************
Failing Tests (1):
    Clangd Unit Tests :: ./ClangdTests/IgnoreDiags.FromNonWrittenInclude

  Expected Passes    : 569
  Unsupported Tests  : 3
  Unexpected Failures: 1
FAILED: tools/clang/tools/extra/clangd/test/CMakeFiles/check-clangd

thanks for the information, I suspect that this patch may depend on rL367303 (which is not merged in the release), I will double check.

[hans]

thanks for the information, I suspect that this patch may depend on rL367303 (which is not merged in the release), I will double check.

Yes, that seems to be it. I've merged that, and then I could merge this one in r368683.

[hokein]

In D66074#1626932, @hans wrote:

thanks for the information, I suspect that this patch may depend on rL367303 (which is not merged in the release), I will double check.

Yes, that seems to be it. I've merged that, and then I could merge this one in r368683.

Great, thanks!

clang-tools-extra/trunk/clangd/Diagnostics.cpp

bool adjustDiagFromHeader(Diag &D, const clang::Diagnostic &Info,
 
   const SourceLocation &DiagLoc = Info.getLocation();
   const SourceManager &SM = Info.getSourceManager();
   SourceLocation IncludeInMainFile;
   auto GetIncludeLoc = [&SM](SourceLocation SLoc) {
     return SM.getIncludeLoc(SM.getFileID(SLoc));
   };
   for (auto IncludeLocation = GetIncludeLoc(DiagLoc); IncludeLocation.isValid();
-       IncludeLocation = GetIncludeLoc(IncludeLocation))
+       IncludeLocation = GetIncludeLoc(IncludeLocation)) {
+    if (clangd::isInsideMainFile(IncludeLocation, SM)) {
       IncludeInMainFile = IncludeLocation;
+      break;
+    }
+  }
   if (IncludeInMainFile.isInvalid())
     return false;
 
   // Update diag to point at include inside main file.
   D.File = SM.getFileEntryForID(SM.getMainFileID())->getName().str();
   D.Range.start = sourceLocToPosition(SM, IncludeInMainFile);
   D.Range.end = sourceLocToPosition(
       SM, Lexer::getLocForEndOfToken(IncludeInMainFile, 0, SM, LangOpts));

clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp

Annotations Header(R"cpp(
     int x = 5/0;
     int b = [[FOO]];)cpp");
   TestTU TU = TestTU::withCode(Main.code());
   TU.AdditionalFiles = {{"a.h", Header.code()}};
   TU.ExtraArgs = {"-DFOO=NOOO"};
   EXPECT_THAT(TU.build().getDiagnostics(), UnorderedElementsAre());
 }
 
+TEST(IgnoreDiags, FromNonWrittenInclude) {
+  TestTU TU = TestTU::withCode("");
+  TU.ExtraArgs.push_back("--include=a.h");
+  TU.AdditionalFiles = {{"a.h", "void main();"}};
+  // The diagnostic "main must return int" is from the header, we don't attempt
+  // to render it in the main file as there is no written location there.
+  EXPECT_THAT(TU.build().getDiagnostics(), UnorderedElementsAre());
+}
+
 } // namespace
 
 } // namespace clangd
 } // namespace clang