Page MenuHomePhabricator

[clang-tidy] Add `bugprone-reserved-identifier`
ClosedPublic

Authored by logan-5 on Jan 7 2020, 5:34 PM.

Details

Summary

This patch adds bugprone-reserved-identifier, which flags uses of __names _Like ::_this, which are reserved for the implementation. The check can optionally be inverted, i.e. configured to flag any names that are _not_ reserved, which may be useful for e.g. standard library implementors.

This diff is relative to, and dependent on, https://reviews.llvm.org/D72284. Not sure if there's a way to chain it or flag it as such within Phabricator, if so let me know.

Diff Detail

Event Timeline

logan-5 created this revision.Jan 7 2020, 5:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 7 2020, 5:34 PM
Eugene.Zelenko added inline comments.
clang-tools-extra/docs/ReleaseNotes.rst
100

One sentence is enough, details belong to documentation.

logan-5 updated this revision to Diff 236754.Jan 7 2020, 8:21 PM
logan-5 marked an inline comment as done.

Addressed some nits.

JonasToth added inline comments.Jan 8 2020, 2:09 AM
clang-tools-extra/docs/clang-tidy/checks/bugprone-reserved-identifier.rst
34

i think you need double-tick here. The highlighting seems weird, usually its orangy, isn't it?

clang-tools-extra/test/clang-tidy/checkers/bugprone-reserved-identifier.cpp
13

missing tests with templates, in this file.

Eugene.Zelenko added inline comments.Jan 8 2020, 6:43 AM
clang-tools-extra/docs/clang-tidy/checks/bugprone-reserved-identifier.rst
34

No, single back-ticks for option values, double back-ticks for language constructs.

logan-5 updated this revision to Diff 236891.Jan 8 2020, 12:47 PM
logan-5 marked 3 inline comments as done.

Added tests for template parameters.

This check is missing a whole lot of reserved identifiers. For instance, in C++ it is missing everything from http://eel.is/c++draft/zombie.names and for C it is missing everything from future library directions. Are you intending to cover those cases as well?

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
124
clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp
36

Please drop top-level const qualifiers for parameters and locals -- that's not a style we typically use.

49–52

Can't this still result in a reserved identifier? e.g.,

int ___Foobar; // three underscores
93

if (llvm::any_of(Whitelist, Name)) return None;

99

AppendFailure per our usual naming conventions.

107

InProgressFixup

120–121

You can just return Info here without the if statement -- the effect is the same.

clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.h
22

Why just C++? C has reserved identifiers as well, and there's a lot of overlap between the two languages in terms of what's reserved.

logan-5 marked 2 inline comments as done.Jan 9 2020, 10:39 AM

This check is missing a whole lot of reserved identifiers. For instance, in C++ it is missing everything from http://eel.is/c++draft/zombie.names and for C it is missing everything from future library directions. Are you intending to cover those cases as well?

I admit that those are outside the scope of what I had originally planned for this check -- I was primarily concerned about 'uglified' names, and writing a check that was 'invertible' in that regard. Now that you mention these, though, I do feel like this check doesn't live up to its name without including them.

I'd be interested in incorporating them. It doesn't sound difficult, but it does sound like it'd be a sizable addition to this diff. Still familiarizing with the workflow around here... would it be alright to leave a TODO comment in this patch and add them in an upcoming patch, to keep this one more self-contained?

clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp
49–52

___Foobar with 3 underscores will be fixed to _Foobar by this fixup, which is then passed through the underscore capital fixup, and that will be caught there. So it still works.

Thinking about it more, though, I do think the consume_front is unnecessary. Without it, __foo would get changed (by this fixup) to _foo, which will be corrected by a later fixup if that's still invalid. If not, that's a smaller and less opinionated change to the name than the current __foo -> foo.

I think I'll take out the consume_front("__") and update the tests to match.

clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.h
22

That's a good point. I'll do some tweaking to make sure this works well for C, including any places where C and C++ differ.

logan-5 updated this revision to Diff 237244.Jan 9 2020, 10:39 PM
logan-5 marked 8 inline comments as done.

Addressed nits. Added CERT aliases. Adjusted the check to work for both C and C++, including where they differ.

Please rebase from trunk. I sorted Clang-tidy entries in Release Notes and aliases have own subsubsection. Please keep alphabetical order there.

clang-tools-extra/docs/clang-tidy/checks/cert-dcl51-cpp.rst
6 ↗(On Diff #237244)

Header length.

This check is missing a whole lot of reserved identifiers. For instance, in C++ it is missing everything from http://eel.is/c++draft/zombie.names and for C it is missing everything from future library directions. Are you intending to cover those cases as well?

I admit that those are outside the scope of what I had originally planned for this check -- I was primarily concerned about 'uglified' names, and writing a check that was 'invertible' in that regard. Now that you mention these, though, I do feel like this check doesn't live up to its name without including them.

I'd be interested in incorporating them. It doesn't sound difficult, but it does sound like it'd be a sizable addition to this diff. Still familiarizing with the workflow around here... would it be alright to leave a TODO comment in this patch and add them in an upcoming patch, to keep this one more self-contained?

I would be fine putting a TODO in the code and addressing it in a follow-up patch. It's not critical for the initial offering.

clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp
122

Sorry, I missed this before -- you can drop the else as well -- we don't use else after an unconditional return as part of our usual style guidelines.

153–162

Rather than use a complex conditional, I would prefer to see some string constants and a variable. It's a bit hard to reason about the diagnostic text as-is.

clang-tools-extra/docs/clang-tidy/checks/bugprone-reserved-identifier.rst
32

You should probably add some links here back to the CERT checks with some words like "this check also covers the following CERT secure coding guidelines" or somesuch.

logan-5 updated this revision to Diff 237433.Jan 10 2020, 2:20 PM
logan-5 marked 4 inline comments as done.

Added a TODO comment for catching more reserved names. Added links in documentation to CERT guidelines covered by the check. Pulled strings into named constants and made that logic easier to read.

Adding Depends on D72284 would add the patches to the stack.
I like the patch!

I think this patch is getting pretty close to finished -- have you tried running it over a large corpus of code to see if it spots reserved identifiers (or unreserved ones)? If not, I would recommend running it over LLVM + clang + clang-tools-extra to see how it operates when looking for reserved identifiers and libc++ for unreserved ones.

clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp
28–35

I think you can reasonably combine these into a single diagnostic using %select. e.g.,
"declaration uses identifier %0, which is %select{a reserved identifier|not a reserved identifier|reserved in the global namespace}1"

I took out the bits about causing undefined behavior because that doesn't really add much to the diagnostic (the "is a reserved identifier" bit should be sufficient). Also, I removed the manual quoting around the %0 because it shouldn't be needed if you pass in a NamedDecl* as opposed to a string (which you should prefer doing because it automatically formats the identifier properly).

66

Elide braces.

clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.h
36

Personal preference to name this AllowedIdentifiers or something along those lines (and same for the user-facing option).

43

This class is marked final, so why are these protected rather than private?

clang-tools-extra/docs/clang-tidy/checks/bugprone-reserved-identifier.rst
9

You should mention that it currently does not check for all reserved names such as future library or zombie names.

logan-5 updated this revision to Diff 238064.Jan 14 2020, 12:08 PM
logan-5 marked 6 inline comments as done.

Renamed check option from "Whitelist" to "AllowedIdentifiers". Added note about missing checks in documentation. Changed to use a %select for diagnostic text. Some nits.

The check does due diligence when run over LLVM (after whitelisting a couple things), correctly flagging a few suspicious names scattered about.

The inverted mode is a bit cumbersome at the moment to use on libc++ (since libc++ legitimately defines lots of non-reserved names (that's its job)). One could whitelist every name the standard defines, but I'd like to add some logic in a future patch that is smarter about which names need to be reserved in inverted mode -- something like, flagging private class members but not public ones, for example.

clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp
28–35

I don't have access to a NamedDecl* for a couple reasons, one being that the same diagnostic is used for both declarations and macro definitions. The %select change sounds like a good one--I'll do that (or I already did it, depending when this comment shows up).

aaron.ballman accepted this revision.Jan 16 2020, 6:58 AM

LGTM!

clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp
28–35

Ah, okay, that's a good point about macros.

This revision is now accepted and ready to land.Jan 16 2020, 6:58 AM

Great! In that case, I'll need help committing this, as well as the thing it depends on, https://reviews.llvm.org/D72284 (which has also been LGTM'd).

Great! In that case, I'll need help committing this, as well as the thing it depends on, https://reviews.llvm.org/D72284 (which has also been LGTM'd).

Great, can you rebase this patch on master? It does not apply cleanly currently.

c:\llvm-project>git apply C:\Users\aballman\Desktop\D72378.diff
C:/Users/aballman/Desktop/D72378.diff:364: trailing whitespace.
Checks for usages of identifiers reserved for use by the implementation.
C:/Users/aballman/Desktop/D72378.diff:371: trailing whitespace.
while the C++ standard strengthens this to reserve names with a double
C:/Users/aballman/Desktop/D72378.diff:378: trailing whitespace.
  namespace NS {
C:/Users/aballman/Desktop/D72378.diff:385: trailing whitespace.
The check can also be inverted, i.e. it can be configured to flag any
C:/Users/aballman/Desktop/D72378.diff:386: trailing whitespace.
identifier that is _not_ a reserved identifier. This mode is for use by e.g.
error: patch failed: clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp:271
error: clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp: patch does not apply
error: patch failed: clang-tools-extra/docs/ReleaseNotes.rst:94
error: clang-tools-extra/docs/ReleaseNotes.rst: patch does not apply
logan-5 updated this revision to Diff 238640.Jan 16 2020, 3:11 PM

Should be good now.

aaron.ballman closed this revision.Jan 17 2020, 5:56 AM

Thank you for the new checker -- I've committed on your behalf in 42a0355816d3bc125d59cbd07052c8886e78ca86

Looks like this isn't building on Windows: http://45.33.8.238/win/6039/step_4.txt

So, i'm seeing an issue here:
https://godbolt.org/z/KM2qLa

I can't NOLINT it, because it is defined not in the source file, but in compile line.
And i can't whitelist it since there is no whitelist..

PTAL.

So, i'm seeing an issue here:
https://godbolt.org/z/KM2qLa

I can't NOLINT it, because it is defined not in the source file, but in compile line.
And i can't whitelist it since there is no whitelist..

PTAL.

Doesn't the AllowedIdentifiers option work for you?

So, i'm seeing an issue here:
https://godbolt.org/z/KM2qLa

I can't NOLINT it, because it is defined not in the source file, but in compile line.
And i can't whitelist it since there is no whitelist..

PTAL.

Doesn't the AllowedIdentifiers option work for you?

Uuuh. To be honest somehow i did not observe it when looking through
the docs (as indicated in it since there is no whitelist..),
It does work in this case, thanks. Sorry for false alarm.

So, i'm seeing an issue here:
https://godbolt.org/z/KM2qLa

I can't NOLINT it, because it is defined not in the source file, but in compile line.
And i can't whitelist it since there is no whitelist..

PTAL.

Doesn't the AllowedIdentifiers option work for you?

Uuuh. To be honest somehow i did not observe it when looking through
the docs (as indicated in it since there is no whitelist..),
It does work in this case, thanks. Sorry for false alarm.

No worries, I'm glad it's working for you!