[PATCH] D41326: [clang-tidy] Added diagnostics about incorrect usage of NOLINT comment

[Anton via Phabricator via cfe-commits]

xgsa added inline comments.


================
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:839-840
+        case NolintCommentType::Nolint:
+          Message = "there is no diagnostics on this line, "
+                    "the NOLINT comment is redundant";
+          break;
----------------
aaron.ballman wrote:
> xgsa wrote:
> > aaron.ballman wrote:
> > > xgsa wrote:
> > > > aaron.ballman wrote:
> > > > > xgsa wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > I don't think the user is going to care about the distinction between no diagnostics being triggered and the expected diagnostic not being triggered. Also, it's dangerous to claim the lint comment is redundant because it's possible the user has NOLINT(foo, bar) and while foo is not triggered, bar still is. The NOLINT comment itself isn't redundant, it's that the check specified doesn't occur.
> > > > > > > 
> > > > > > > I would consolidate those scenarios into a single diagnostic: "expected diagnostic '%0' not generated" and "expected diagnostic '%0' not generated for the following line".
> > > > > > > 
> > > > > > > One concern I have with this functionality is: how should users silence a lint diagnostic that's target sensitive? e.g., a diagnostic that triggers based on the underlying type of size_t or the signedness of plain char. In that case, the diagnostic may trigger for some targets but not others, but on the targets where the diagnostic is not triggered, they now get a diagnostic they cannot silence. There should be a way to silence the "bad NOLINT" diagnostics.
> > > > > > > I don't think the user is going to care about the distinction between no diagnostics being triggered and the expected diagnostic not being triggered. Also, it's dangerous to claim the lint comment is redundant because it's possible the user has NOLINT(foo, bar) and while foo is not triggered, bar still is. The NOLINT comment itself isn't redundant, it's that the check specified doesn't occur.
> > > > > > > 
> > > > > > > I would consolidate those scenarios into a single diagnostic: "expected diagnostic '%0' not generated" and "expected diagnostic '%0' not generated for the following line".
> > > > > > 
> > > > > > This branch of `if (NolintEntry.first.CheckName == NolintCommentsCollector::AnyCheck)` reports only about `NOLINT`/`NOLINTNEXTLINE` comments without check list, so I suppose it's fair to claim that this comment is redundant (we have already checked that no single check reported diagnostics on the line). The `else`-branch reports the diagnostics for the definite check in a list in case of `NOLINT(foo, bar)` (actually, if neither `foo` nor `bar` checks reported diagnostics for the line, there will be a few diagnostics from `nolint-usage` - not sure if it's good, but it seems acceptable). That is why, I suppose, it is necessary to distinct these cases.
> > > > > > 
> > > > > > > One concern I have with this functionality is: how should users silence a lint diagnostic that's target sensitive? e.g., a diagnostic that triggers based on the underlying type of size_t or the signedness of plain char. In that case, the diagnostic may trigger for some targets but not others, but on the targets where the diagnostic is not triggered, they now get a diagnostic they cannot silence. There should be a way to silence the "bad NOLINT" diagnostics.
> > > > > > 
> > > > > > There is such mechanism: it is possible to specify `// NOLINT(nolint-usage)` or `//NOLINT(check1, check2, nolint-usage) to silence the `nolint-usage`-mechanism. Please, see tests for details and more examples. 
> > > > > Can you provide an example where this distinction will make a difference to the user and help clarify a confusing situation? I cannot think of one, and it would be nice to simplify this code.
> > > > > 
> > > > > Thank you for the explanation about nolint-usage. This is not terminology I've seen before -- is this your invention, or is it a documented feature for NOLINT comments?
> > > > >> Can you provide an example where this distinction will make a difference to the user and help clarify a confusing situation? I cannot think of one, and it would be nice to simplify this code.
> > > > 
> > > > Example for the diagnostics emitted in the `if`-branch:
> > > > ```
> > > > class A2 { explicit A2(int i); }; // NOLINT
> > > > => warning: there is no diagnostics on this line, the NOLINT comment is redundant
> > > > ```
> > > > Thus, the whole NOLINT comment should be removed.
> > > > 
> > > > Example for the diagnostics emitted in the `else`-branch:
> > > > ```
> > > > // Case: NO_LINT for the specific check on line with an error on another check.
> > > > class A4 { A4(int i); }; // NOLINT(misc-unused-parameters, google-explicit-constructor)
> > > > => warning: there is no diagnostics for the 'misc-unused-parameters' check on this line, the NOLINT comment is redundant
> > > > ```
> > > > In this case, only misc-unused-parameters check should be removed from the list, but not the whole NOLINT.
> > > > 
> > > > Note also that if in the last example there is only misc-unused-parameters in the NOLINT checks list, the diagnostics will be the same. However, I suppose it's acceptable, because even if a user removed only a check name from the check list without removing NOLINT itself, clang-tidy will suggest removing NOLINT itself on the next run.
> > > > 
> > > > 
> > > > >> Thank you for the explanation about nolint-usage. This is not terminology I've seen before -- is this your invention, or is it a documented feature for NOLINT comments?
> > > > 
> > > > It's my invention, I suppose, from the user point of view it's logical to consider this mechanism as a check.
> > > Thank you for the examples, but I don't think they would add any clarification for the user, either. That said, I think we could collapse all four diagnostics into: "there are no diagnostics %select{|for '%2'}0 on %select{this|the next}1 line" where %0 distinguishes between NOLINT with and without an explicit check name, %1 distinguishes between NOLINT and NOLINTNEXTLINE, and %2 is the name of the check in question (if any).
> > > 
> > > >> Thank you for the explanation about nolint-usage. This is not terminology I've seen before -- is this your invention, or is it a documented feature for NOLINT comments?
> > > > It's my invention, I suppose, from the user point of view it's logical to consider this mechanism as a check.
> > > 
> > > I think we should devise a better way of expressing this unless there's industry practice suggesting a specific term or syntax. nolint-usage isn't very discoverable. Whatever we wind up with, it should work naturally for constructs like your first example. In the first example, diagnosing that NOLINT is redundant only makes sense in some circumstances, such as when the construct is in a source file that is not compiled with google-explicit-constructor enabled. However, if the code is in a shared header file where some (but not all) source code is checked with google-explicit-constructor enabled, it serves a purpose and removing it would be incorrect. The same holds for NOLINT(check).
> > > 
> > > Ultimately, I think that diagnosing NOLINT on a line that issues no diagnostics should not be enabled by default. It's highly likely that the NOLINT directive is due to reasons unknowable to the implementation, and suggesting the user remove the comment seems unhelpful. My example above is one plausible case where NOLINT may serve a purpose. Another example is code checked by clang-tidy as well as other tools; the NOLINT may be silencing the other tools. I think it should take extra work by the user to enable this functionality in a catch-all manner -- either via different syntax or enabled only through an explicit flag.
> > > 
> > > I think that diagnosing NOLINT(check-name) when that diagnostic is not triggered is useful by default because the user is explicitly expecting a diagnostic by name. However, I think the user needs an almost-trivial way to silence the "not triggered" diagnostic on a case-by-case basis. Target specific diagnostics as well as shared code are both examples of situations where the NOLINT comment may be accurate but for situations unknown to clang-tidy. Telling the user to remove the NOLINT comment would be the wrong thing to do.
> > If to look at cpplint, which I suppose, NOLINT syntax was taken from, it also reports diagnostics for incorrect usage of NOLINT as messages in category 'readability/nolint' [1].
> > 
> > In spite of the fact, that `nolint-usage` is technically not a check, it mimics the behavior of a simple check: it is only enabled if `nolint-usage` is enabled via `-check` option (explicitly or via *) or configuration; it reports diagnostics for this category and allows this diagnostics to be suppressed via NOLINT. The only missing thing for now is documentation page for the `nolint-usage` check and I am going to work on this after this patch is approved. Thus, I don't think that this approach is not discoverable and it seems more natural than adding a separate command line option and a separate way of suppressing NOLINT diagnostics.
> > 
> > I agree that this diagnostics will only be useful if NOLINT comments are carefully maintained exactly for clang-tidy. I don't think that if they are also added for another tool, the check names or even diagnostics lines will match. Thus, for these cases the check just shouldn't be enabled in configuration. The same thing is for headers. I suppose, that typically the list of checks is the same for the whole project, thus diagnostics for headers won't be an issue, but if it is not the case, the `nolint-usage` check could just be disabled for some modules. Anyway, I don't see the way to determine if a header contains the diagnostics been included from the other source files.
> > 
> > If it is necessary, some configuration options could be added for this check (e.g. an option to check only `NOLINT` comments in source files, but no headers; or to check only `NOLINT` comments with/without explicit list of checks specification).
> > 
> > [1] - https://github.com/google/styleguide/blob/9663cabfeeea8f1307b1acde59471f74953b8fa9/cpplint/cpplint.py#L577
> > In spite of the fact, that nolint-usage is technically not a check
> 
> That's part of what concerns me about this in terms of discoverability and usability -- it's not a real check but it sort of behaves like one. Does this get listed with -list-checks? If the user passes `-nolint-*` on the command line does it still get disabled?
> 
> Also, all of this is predicated by the fact that these nolint warnings are going to trigger in such a way that sometimes needs to be silenced. What I think isn't very intuitive is that the recommended way to silence a nolint-comment-related diagnostic is to use another nolint comment to disable reporting bugs with nolint comments. However, documentation can help with that, so perhaps it's not bad.
> Does this get listed with -list-checks?

Not yet, I missed that, but according to my understanding, the "nolint-usage" should be in the list. I'll fix it.

> If the user passes -nolint-* on the command line does it still get disabled?

It should work (there is the `isCheckEnabled` call in `ClangTidyContext::setPreprocessor`, so if no comments are collected, no checks will be done and no diagnostics reported).

> What I think isn't very intuitive is that the recommended way to silence a nolint-comment-related diagnostic is to use another nolint comment to disable reporting bugs with nolint comments.

Could you please suggest a more intuitive way as an example?

> However, documentation can help with that, so perhaps it's not bad.

So is documentation the only concern about this patch or don't you like the overall idea?


================
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:117
+const StringRef ClangDiagnosticsPrefix = "clang-diagnostic-";
+const StringRef NolintCheckName = "readability-nolint-usage";
+enum class NolintCommentType : uint8_t {
----------------
aaron.ballman wrote:
> Why "readability"? That module is generally for code readability issues like redundant code or naming conventions. I think this should probably be in "misc" or perhaps "bugprone".
I consider this check as a detector of redundant comments (i.e. redundant code). Moreover, it seems, these comments influence only on code readability and don't bring bugs, so "bugprone" doesn't look good for me. One more minor argument for "readability" is that it is in such category in cpplint, so it will be more natural for its users. Summing up, I am for leaving the check in "readability", but I don't mind also putting it in "misc".


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41326