Page MenuHomePhabricator

Add new clang-tidy check for string_view(nullptr)
ClosedPublic

Authored by CJ-Johnson on Nov 3 2021, 2:47 PM.

Details

Summary

Checks for various ways that the const CharT* constructor of std::basic_string_view can be passed a null argument and replaces them with the default constructor in most cases. For the comparison operators, braced initializer list does not compile so instead a call to .empty() or the empty string literal are used, where appropriate.

This prevents code from invoking behavior which is unconditionally undefined. The single-argument const CharT* constructor does not check for the null case before dereferencing its input. The standard is slated to add an explicitly-deleted overload to catch some of these cases: wg21.link/p2166

https://reviews.llvm.org/D114823 is a companion change to prevent duplicate warnings from the bugprone-string-constructor check.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

This seems to be an existing check. Have you compared it with bugprone-string-constructor?

Thanks for the suggestion! From what I can tell, bugprone-string-constructor check only has warnings and does not provide fixes in most cases. The goal of bugprone-stringview-nullptr is to robustly enumerate the many cases that it cares about and provide fixes. For that reason, I think making it a separate check is best.

This seems to be an existing check. Have you compared it with bugprone-string-constructor?

Thanks for the suggestion! From what I can tell, bugprone-string-constructor check only has warnings and does not provide fixes in most cases. The goal of bugprone-stringview-nullptr is to robustly enumerate the many cases that it cares about and provide fixes. For that reason, I think making it a separate check is best.

Yes, But i think that improving existing check is the best way. Because improving bugprone-string-construct in a new check may make developers confused and cause redundant overlap.
Let's see if @aaron.ballman or @whisperity has any comments?

This seems to be an existing check. Have you compared it with bugprone-string-constructor?

Thanks for the suggestion! From what I can tell, bugprone-string-constructor check only has warnings and does not provide fixes in most cases. The goal of bugprone-stringview-nullptr is to robustly enumerate the many cases that it cares about and provide fixes. For that reason, I think making it a separate check is best.

Yes, But i think that improving existing check is the best way. Because improving bugprone-string-construct in a new check may make developers confused and cause redundant overlap.
Let's see if @aaron.ballman or @whisperity has any comments?

Generally speaking, we prefer to improve the existing checks. I think bugprone-string-constructor would probably be a better place for the constructor-related functionality. But that still means we don't have a good place for things like the assignment and comparison functionality, and it seems more useful to keep all of the string_view-with-nullptr logic in one place. That said, we should be careful we're not too onerous when users enable all bugprone checks together.

CJ-Johnson updated this revision to Diff 384775.Nov 4 2021, 9:09 AM

Remove trailing whitespace

Generally speaking, we prefer to improve the existing checks. I think bugprone-string-constructor would probably be a better place for the constructor-related functionality. But that still means we don't have a good place for things like the assignment and comparison functionality, and it seems more useful to keep all of the string_view-with-nullptr logic in one place. That said, we should be careful we're not too onerous when users enable all bugprone checks together.

I agree with the statement "it seems more useful to keep all of the string_view-with-nullptr logic in one place". To me, these are very logically related. The fact that some of it is constructor based is not as useful from a user perspective. The relationship between nullptr and string_view is more important, in my view.

As for "we should be careful we're not too onerous when users enable all bugprone checks together.", these fixes are about preventing UB. While I did put this in the "bugprone" module, perhaps "prone" is the wrong way to think about it. It's unconditionally UB in all cases, not just a potential bug. The suggested fixes are important for preventing crashes in the short term, but more importantly for allowing the code to build in the future, since the constructor overload basic_string_view(nullptr_t) = delete; is being added.

As for "we should be careful we're not too onerous when users enable all bugprone checks together.", these fixes are about preventing UB. While I did put this in the "bugprone" module, perhaps "prone" is the wrong way to think about it. It's unconditionally UB in all cases, not just a potential bug. The suggested fixes are important for preventing crashes in the short term, but more importantly for allowing the code to build in the future, since the constructor overload basic_string_view(nullptr_t) = delete; is being added.

The concern I have is that a user passes -checks=-*, bugprone-* and gets two diagnostics on the same line for the same issue. There is precedence, but usually clang-tidy only gets in this situation when the user enables checks from different modules (I don't know of any case where it happens within the same module). Your existing test cases have significant coverage already: https://godbolt.org/z/49nGYEGzq so this isn't a theoretical concern -- anyone checking against all bugprone (which is pretty typical) will get duplicate diagnostics. So we're in a bit of a novel situation with this patch, and I'm not certain what the correct answer is. I see a few paths:

  • accept that there are two diagnostics for the same code construct
  • remove the string_view nullptr checking functionality from bugprone-string-constructor so it only lives in the new check
  • remove the constructor checking functionality from the new check so it only lives in bugprone-string-constructor
  • have both of the checks look to see whether the other check is enabled before issuing the diagnostic and pick one of them to be the "winner" so there's only one diagnostic

There's pros and cons to all of these approaches (and maybe there are other approaches to consider as well). I sort of lean towards the last option, but that could be confusing for people trying to suppress diagnostics. e.g., they could write NOLINT(bugprone-string-constructor) to silence the diagnostic and then they get a different diagnostic they also have to silence. However, I also sort of think that confusion is not likely to happen in practice because 1) users are more likely to fix their code than silence *this* warning, 2) users are more likely to write NOLINT without a specific check, so all tidy diagnostics would be suppressed. But it's still a bit of concern as a precedent.

@alexfh -- as code owner, do you have thoughts here?

clang-tools-extra/test/clang-tidy/checkers/bugprone-stringview-nullptr.cpp
85

This (and many others) also generates -Wbraced-scalar-init, is that intentional?

CJ-Johnson marked an inline comment as done.Nov 4 2021, 3:15 PM

Thanks for the additional info, @aaron.ballman! I had not considered the issues with duplicate warnings. I agree that it would be annoying for users. That being the case, the second option ("remove the string_view nullptr checking functionality from bugprone-string-constructor so it only lives in the new check") sounds the most appealing to me, personally.

clang-tools-extra/test/clang-tidy/checkers/bugprone-stringview-nullptr.cpp
85

My goal was just to be thorough in the cases tested. It's not an endorsement of the source patterns. :)

CJ-Johnson updated this revision to Diff 384881.Nov 4 2021, 3:20 PM
CJ-Johnson marked an inline comment as done.

Fix spelling error

CJ-Johnson updated this revision to Diff 384890.Nov 4 2021, 3:50 PM

Change static_cast suggested edit to use parens instead of braces

Remove double brackets for list initialization and use default initialization to avoid most vexing parse

CJ-Johnson updated this revision to Diff 385495.Nov 8 2021, 7:34 AM

Update test file to use truncated messages

CJ-Johnson added inline comments.Nov 8 2021, 9:20 AM
clang-tools-extra/test/clang-tidy/checkers/bugprone-stringview-nullptr.cpp
85

As a followup to this: I've added additional test cases and a minor change to the AST matchers to catch another form of the -Wbraced-scalar-init pattern.

This: accepts_string_view({{}});

This pattern will select the const CharT* constructor overload and then value-initialize the argument, causing a null deref. It happens to not include the nullptr literal but it has the exact same effect as accepts_string_view(nullptr); and accepts_string_view({nullptr});

CJ-Johnson updated this revision to Diff 385530.Nov 8 2021, 9:23 AM

Add AST matcher support and tests for {{}} case where a null pointer is implicitly created

CJ-Johnson updated this revision to Diff 385531.Nov 8 2021, 9:24 AM

Add missing newline

CJ-Johnson updated this revision to Diff 385532.Nov 8 2021, 9:28 AM

Rebase on head

Add additional tests for sv.empty() cases to ensure parens are removed where appropriate

CJ-Johnson updated this revision to Diff 385635.Nov 8 2021, 2:35 PM

Add missing test cases

This looks really good. An impressive effort! Just a few changes. Also, please ping Alex to get his opinion on the high level issue.

clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp
63
68
76

And more below...

91

Here and throughout the matchers: in general, has is a fallback when you don't have a more specific matcher. In this case, It think you mean to be testing the initialization expression, in which case you should query that directly: hasInitializer. I'd recommend you revisit the uses of has and check in each case if there's a more specific query. That's both cheaper (doesn't need to iterate through all children) and more readable (because it expresses the intent).

clang-tools-extra/test/clang-tidy/checkers/bugprone-stringview-nullptr.cpp
135–148

what are these lines doing?

CJ-Johnson marked 5 inline comments as done.Nov 12 2021, 11:09 AM
CJ-Johnson added inline comments.
clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp
76

Updated all 9 of them. Thanks!

91

Thanks for the suggestion! I went back through all of the has calls and replaced the ones I could with something more specific.

clang-tools-extra/test/clang-tidy/checkers/bugprone-stringview-nullptr.cpp
135–148

These lines are not "tests" themselves but they clearly document that all of the various permutations have been considered. If someone reads the test file and thinks "what about this other case?", these demonstrate that such other cases have been considered but are not valid :)

CJ-Johnson marked 3 inline comments as done.

Address review comments from Yitzie

Generally speaking, we prefer to improve the existing checks. I think bugprone-string-constructor would probably be a better place for the constructor-related functionality. But that still means we don't have a good place for things like the assignment and comparison functionality, and it seems more useful to keep all of the string_view-with-nullptr logic in one place. That said, we should be careful we're not too onerous when users enable all bugprone checks together.

As for "we should be careful we're not too onerous when users enable all bugprone checks together.", these fixes are about preventing UB. While I did put this in the "bugprone" module, perhaps "prone" is the wrong way to think about it. It's unconditionally UB in all cases, not just a potential bug. The suggested fixes are important for preventing crashes in the short term, but more importantly for allowing the code to build in the future, since the constructor overload basic_string_view(nullptr_t) = delete; is being added.

@aaron.ballman Maybe we should do something to ease the burden of bugprone- becoming a catch-all basket, and create a new checker group for the "unconditional UB" or "in almost all cases this will make you do bad things" kinds of stuff.

clang-tools-extra/docs/ReleaseNotes.rst
82–90

Usually we only put a one sentence short summary into the release notes.

clang-tools-extra/docs/clang-tidy/checks/bugprone-stringview-nullptr.rst
14–15

You mentioned in the review discussion that a constructor taking std::nullptr_t will be added with an explicit = delete;. Maybe it is worth mentioning it here, if we have a standard version or a proposal on track for changing the STL like that?

17–18

Will this work? Isn't the : making RST confused here? You usually need an empty line because the block "instance" may be given additional options in RST.

It might compile the docs still, so this is the kind of thing that must be verified visually.

47

There should be a .. note:: block which renders a nice box of note with an icon.

50

Clang-Tidy. Or just simply "this check".

54

ditto.

Generally speaking, we prefer to improve the existing checks. I think bugprone-string-constructor would probably be a better place for the constructor-related functionality. But that still means we don't have a good place for things like the assignment and comparison functionality, and it seems more useful to keep all of the string_view-with-nullptr logic in one place. That said, we should be careful we're not too onerous when users enable all bugprone checks together.

As for "we should be careful we're not too onerous when users enable all bugprone checks together.", these fixes are about preventing UB. While I did put this in the "bugprone" module, perhaps "prone" is the wrong way to think about it. It's unconditionally UB in all cases, not just a potential bug. The suggested fixes are important for preventing crashes in the short term, but more importantly for allowing the code to build in the future, since the constructor overload basic_string_view(nullptr_t) = delete; is being added.

@aaron.ballman Maybe we should do something to ease the burden of bugprone- becoming a catch-all basket, and create a new checker group for the "unconditional UB" or "in almost all cases this will make you do bad things" kinds of stuff.

bugprone was originally added because misc was becoming too much of a catch-all basket and we wanted a new group for the "doing this is a bug" kind of checks, so there's precedent for splitting groups when we need finer granularity. However, we had quite a few checks that could be moved from misc to bugrone during that switch. If we added a group for unconditional UB (or morally similar), it'd be good to know how many checks we think need to move and what sort of checks we envision the new module encouraging in the future. That'll help inform us whether it's good value to make a split or not.

This looks really good. An impressive effort! Just a few changes. Also, please ping Alex to get his opinion on the high level issue.

Since this check focuses really well on the issue of creating string_view from nullptr (and unlike the other check in this case, provides automated fixes), it seems best to limit the warning for nullptr argument in bugprone-string-constructor to std::string.

As for creating a "certainly bugs" / "unconditional UB" module: how many checks apart from this one would fall into that category? From a very brief look I found bugprone-dangling-handle and bugprone-undelegated-constructor. The latter could likely be reimplemented as a clang diagnostic. A separate module for two or three checks doesn't sound particularly exciting.

CJ-Johnson marked 6 inline comments as done.

Address reviewer comments

Rebase on head

Thank you all for the feedback! Would it be ok to proceed with removing the nullptr checking for basic_string_view in bugprone-string-constructor, as Alex indicated?

Address nits

Rebase on head

CJ-Johnson edited the summary of this revision. (Show Details)Nov 30 2021, 3:21 PM

Pinging this thread with https://reviews.llvm.org/D113148

If we land both this revision and that one, then duplicate warnings won't be a problem. :)

CJ-Johnson updated this revision to Diff 391068.Dec 1 2021, 9:54 AM

Add missing comment lines

ymandel accepted this revision.Dec 1 2021, 2:02 PM

To other reviewers: barring any additional comments, I will push this patch tomorrow morning (CJ doesn't have commit rights).

clang-tools-extra/test/clang-tidy/checkers/bugprone-stringview-nullptr.cpp
118–131

nit: personally, i'd just delete these -- anything that doens't compile isn't really of interest to clang tidy. While I see the educational value, these kinds of comments are prone to bitrot (since we're not actually testing anything) and therefore have questionable value even for education.

135–148

ah, i see. maybe put in a comment to that respect?

This revision is now accepted and ready to land.Dec 1 2021, 2:02 PM
CJ-Johnson updated this revision to Diff 391140.Dec 1 2021, 2:29 PM

Delete non-functional test cases (which were previously commented out)

CJ-Johnson marked 2 inline comments as done.Dec 1 2021, 2:29 PM
This revision was automatically updated to reflect the committed changes.