This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Get ClangTidyContext out of the business of storing diagnostics. NFC
ClosedPublic

Authored by sammccall on Oct 31 2018, 3:15 PM.

Details

Summary

Currently ClangTidyContext::diag() sends the diagnostics to a
DiagnosticsEngine, which probably delegates to a ClangTidyDiagnosticsConsumer,
which is supposed to go back and populate ClangTidyContext::Errors.

After this patch, the diagnostics are stored in the ClangTidyDiagnosticsConsumer
itself and can be retrieved from there.

Why?

  • the round-trip from context -> engine -> consumer -> context is confusing and makes it harder to establish layering between these things.
  • context does too many things, and makes it hard to use clang-tidy as a library
  • everyone who actually wants the diagnostics has access to the ClangTidyDiagnosticsConsumer

The most natural implementation (ClangTidyDiagnosticsConsumer::take()
finalizes diagnostics) causes a test failure: clang-tidy-run-with-database.cpp
asserts that clang-tidy exits successfully when trying to process a file
that doesn't exist.
In clang-tidy today, this happens because finish() is never called, so the
diagnostic is never flushed. This looks like a bug to me.
For now, this patch carefully preserves that behavior, but I'll ping the
authors to see whether it's deliberate and worth preserving.

Event Timeline

sammccall created this revision.Oct 31 2018, 3:15 PM

@hokein @alexfh There is a test that asserts clang-tidy exits with 0 when processing a nonexistent file (test added in D17335 which you wrote/reviewed).
This seems like a bug to me, do I need to preserve this behavior? (See patch description for how this happens).

The command is clang-tidy --checks="-*,modernize-use-nullptr" -p /some/path /some/path/not-exist.

Behavior at HEAD is:

Error while processing /some/path/not-exist.
Suppressed 1 warnings (1 with check filters).
<exit with status 0>

More natural and possibly correct behavior (flushing all diagnostics):

Error while processing /some/path/not-exist.
error: unable to handle compilation, expected exactly one compiler job in '' [clang-diagnostic-error]
Suppressed 1 warnings (1 with check filters).
Found compiler error(s).
<exit with status 1>

LG in general.

The most natural implementation (ClangTidyDiagnosticsConsumer::take()
finalizes diagnostics) causes a test failure: clang-tidy-run-with-database.cpp
asserts that clang-tidy exits successfully when trying to process a file
that doesn't exist.
In clang-tidy today, this happens because finish() is never called, so the
diagnostic is never flushed. This looks like a bug to me.
For now, this patch carefully preserves that behavior, but I'll ping the
authors to see whether it's deliberate and worth preserving.

This looks like a bug indeed. Looking at https://reviews.llvm.org/D17335 where the test was added, I can't see any evidence of the opposite. Let's try fixing this (and the test) unless Haojian knows reasons to leave the current behavior.

unittests/clang-tidy/ClangTidyTest.h
128–129

Please specify the type explicitly, since it's not obvious from the context. http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

error: unable to handle compilation, expected exactly one compiler job in '' [clang-diagnostic-error]
Suppressed 1 warnings (1 with check filters).
Found compiler error(s).

That's preferred to the current behavior, but from a user perspective a more descriptive message would be better (like "can't find file /some/file" or "can't open file /some/file"). I don't know whether it's better to do in clang proper or in clang-tidy, but it can be handled separately from this patch.

sammccall updated this revision to Diff 172039.Oct 31 2018, 3:59 PM

Remove backwards-compat behavior around ClangdDiagnosticConsumer::finish(), it's presumably a bug
Update test to verify we fail but don't crash in that case.
auto -> real type

sammccall marked an inline comment as done.Oct 31 2018, 4:02 PM

Thanks Alex. @hokein, let me know if any concerns.

error: unable to handle compilation, expected exactly one compiler job in '' [clang-diagnostic-error]
Suppressed 1 warnings (1 with check filters).
Found compiler error(s).

That's preferred to the current behavior, but from a user perspective a more descriptive message would be better (like "can't find file /some/file" or "can't open file /some/file"). I don't know whether it's better to do in clang proper or in clang-tidy, but it can be handled separately from this patch.

Agreed, this error message template is really weird by itself, and the empty string makes it worse.
Will dig into it, agree it doesn't seem directly related.

alexfh accepted this revision.Oct 31 2018, 4:22 PM

LG with a nit.

clang-tidy/ClangTidyDiagnosticConsumer.h
252

Not used now.

This revision is now accepted and ready to land.Oct 31 2018, 4:22 PM
sammccall updated this revision to Diff 172047.Oct 31 2018, 4:31 PM

Removed unused var

sammccall marked an inline comment as done.Oct 31 2018, 4:36 PM

BTW, the missing "X doesn't exist" is because Tooling.cpp turns off driver diagnostics for nonexistent files, because clang/driver wasn't VFS-aware... in 2011.
I'll send patches to fix this if there isn't too much fallout (Driver still needs to be fixed to use its VFS in this case, and then Tooling needs to enable the check).

This means you'll get:

error: no such file or directory: '/path/to/file.cpp'
error: no input files

Clang-tidy will decorate these with [clang-diagnostic-error], but that's a separate bug I guess.

hokein accepted this revision.Nov 2 2018, 1:57 AM

There is a test that asserts clang-tidy exits with 0 when processing a nonexistent file (test added in D17335 which you wrote/reviewed).

This seems like a bug to me, do I need to preserve this behavior? (See patch description for how this happens).

I think this is a bug, we should return non-zero code when processing a non-existing file. The new behavior seems better to me, though we need to polish the error message.

clang-tidy/ClangTidy.h
247–248

nit: maybe name it Errors, and we match the comment again (it is out-of-date at HEAD :().

sammccall marked an inline comment as done.Nov 2 2018, 2:42 AM

I think this is a bug, we should return non-zero code when processing a non-existing file. The new behavior seems better to me, though we need to polish the error message.

Thanks. See D53958 for (most of) the needed error message improvements.

sammccall updated this revision to Diff 172326.Nov 2 2018, 2:46 AM

Address comment, rebase

This revision was automatically updated to reflect the committed changes.
This comment was removed by mehdi_amini.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 24 2023, 4:16 PM
Herald added a subscriber: mgehre. · View Herald Transcript
This comment was removed by mehdi_amini.