[Clang] restrict use of attribute names reserved by the C++ standard #106036
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Oleksandr T. (a-tarasyuk) ChangesFixes #92196 https://eel.is/c++draft/macro.names#2 Full diff: https://github.com/llvm/llvm-project/pull/106036.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 2c6c7e083b9c91..b8b76eb0ff380c 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -254,6 +254,8 @@ Improvements to Clang's diagnostics
compilation speed with modules. This warning is disabled by default and it needs
to be explicitly enabled or by ``-Weverything``.
+- Clang now diagnoses the use of attribute names reserved by the C++ standard. (#GH92196).
+
Improvements to Clang's time-trace
----------------------------------
diff --git a/clang/lib/Lex/PPDirectives.cpp b/clang/lib/Lex/PPDirectives.cpp
index 4e77df9ec444c7..bbe1b4a640c4ac 100644
--- a/clang/lib/Lex/PPDirectives.cpp
+++ b/clang/lib/Lex/PPDirectives.cpp
@@ -177,6 +177,26 @@ static bool isLanguageDefinedBuiltin(const SourceManager &SourceMgr,
return false;
}
+static bool isReservedAttrName(Preprocessor &PP, IdentifierInfo *II) {
+ const LangOptions &Lang = PP.getLangOpts();
+ const StringRef Name = II->getName();
+
+ if (Lang.CPlusPlus26)
+ return Name == "indeterminate";
+ if (Lang.CPlusPlus23)
+ return Name == "assume";
+ if (Lang.CPlusPlus20)
+ return Name == "no_unique_address";
+ if (Lang.CPlusPlus17)
+ return Name == "fallthrough" || Name == "nodiscard" ||
+ Name == "maybe_unused";
+ if (Lang.CPlusPlus14)
+ return Name == "deprecated";
+ if (Lang.CPlusPlus11)
+ return Name == "noreturn" || Name == "carries_dependency";
+ return false;
+}
+
static MacroDiag shouldWarnOnMacroDef(Preprocessor &PP, IdentifierInfo *II) {
const LangOptions &Lang = PP.getLangOpts();
StringRef Text = II->getName();
@@ -186,6 +206,8 @@ static MacroDiag shouldWarnOnMacroDef(Preprocessor &PP, IdentifierInfo *II) {
return MD_KeywordDef;
if (Lang.CPlusPlus11 && (Text == "override" || Text == "final"))
return MD_KeywordDef;
+ if (isReservedAttrName(PP, II))
+ return MD_KeywordDef;
return MD_NoWarn;
}
diff --git a/clang/test/Preprocessor/macro-reserved-attrs.cpp b/clang/test/Preprocessor/macro-reserved-attrs.cpp
new file mode 100644
index 00000000000000..b4336f801e438a
--- /dev/null
+++ b/clang/test/Preprocessor/macro-reserved-attrs.cpp
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 -fsyntax-only -verify=expected,cxx11 -pedantic -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -verify=expected,cxx14 -pedantic -std=c++14 %s
+// RUN: %clang_cc1 -fsyntax-only -verify=expected,cxx17 -pedantic -std=c++17 %s
+// RUN: %clang_cc1 -fsyntax-only -verify=expected,cxx20 -pedantic -std=c++20 %s
+// RUN: %clang_cc1 -fsyntax-only -verify=expected,cxx23 -pedantic -std=c++23 %s
+// RUN: %clang_cc1 -fsyntax-only -verify=expected,cxx26 -pedantic -std=c++26 %s
+
+#define noreturn 1 // cxx11-warning {{keyword is hidden by macro definition}}
+#undef noreturn
+
+#define carries_dependency 1 // cxx11-warning {{keyword is hidden by macro definition}}
+#undef carries_dependency
+
+#define deprecated 1 // cxx14-warning {{keyword is hidden by macro definition}}
+#undef deprecated
+
+#define fallthrough 1 // cxx17-warning {{keyword is hidden by macro definition}}
+#undef fallthrough
+
+#define maybe_unused 1 // cxx17-warning {{keyword is hidden by macro definition}}
+#undef maybe_unused
+
+#define nodiscard 1 // cxx17-warning {{keyword is hidden by macro definition}}
+#undef nodiscard
+
+#define no_unique_address 1 // cxx20-warning {{keyword is hidden by macro definition}}
+#undef no_unique_address
+
+#define assume 1 // cxx23-warning {{keyword is hidden by macro definition}}
+#undef assume
+
+#define indeterminate 1 // cxx26-warning {{keyword is hidden by macro definition}}
+#undef indeterminate
|
There was a problem hiding this comment.
I would expect these to be controlled by -Wreserved-identifier rather than keyword hiding (these are not keywords), probably in a new group like -Wreserved-attribute-identifier, WDYT?
SourceGraph's backend is currently falling over, so I don't have links to share, but I did see examples of #define likely in projects like MongoDB and #define assume in a fair number of projects. I'm slightly worried about the amount of noise this will generate in practice -- have you tried running these changes over a large corpus of C++ code?
a-tarasyuk
force-pushed
the
feat/92196
branch
from
0c2b5af to
8314ba1
Compare
a-tarasyuk
force-pushed
the
feat/92196
branch
from
8314ba1 to
d4b07c7
Compare
|
FWIW, LWG is still discussing the issue while trying to prioritize it, and it's got a reasonable amount of discussion on it. I think we should hold off making any changes or landing this patch until we have a better sense of whether LWG is going to refine this restriction or not. |
|
@AaronBallman oke, thanks for the update. |
|
I saw this got updates over break. Did LWG make its decision already? Are you expecting re-review? I didn't see anything go across my emails about LWG, but I could definitely have missed it. |
The LWG issue is still open: https://cplusplus.github.io/LWG/issue4149 but doesn't seem to be under active discussion any longer. My reading of the reflector thread is that my suggested approach is not quite correct. Consider a case like: where the namespace isn't going to be declared (aka header not included) at the time the WDYT @erichkeane? |
Right part of the point is that the rule is SUPPOSED to protect against that case, and frankly, doesn't give a rip about the alternative. The standard doesn't REALLY care if you put that define AFTER all STL header includes, only if it is before, so trying to determine it at declaration time isn't really possible. We discussed it offline: And I think making this diagnostic, "IF you use the STL and have this define, UB", with an easy enough to disable warning flag is about as good as we can get here. Trying to intuit whether the person is EVER going to use the STL in the TU is a fools errand. |
There was a problem hiding this comment.
We discussed it offline: And I think making this diagnostic, "IF you use the STL and have this define, UB", with an easy enough to disable warning flag is about as good as we can get here. Trying to intuit whether the person is EVER going to use the STL in the TU is a fools errand.
Could we collect a list on Sema of the directives that we would have diagnosed if std were already declared, and then produce the diagnostics at the point where we see the first declaration of the std namespace? That seems like it could be relatively straightforward.
I had considered this, but frankly, I think that ends up with a bit of a chicken/egg problem, and quite a bit of extra work for something that affects basically no one. Part of the problem is figuring out WHEN a library causes the restrictions in macro.names. Paragraph 2 (via CWG issue, and, IMO, by the rights that the Library section of the wording have) states that a TU that includes a standard library header shall not #define/#undef/etc. SO at that point, you'd have to decide WHAT makes a standard library header sufficiently enough to opt its users into the The only one who knows whether their library IS an Standard Library Header for this purpose, so the 'right' way to do this would be some sort of opt-in (via an attribute, config file, etc). AND if they're doing that, perhaps the StdLib should be the one enforcing this rule in the first place, via #ifdefs and #warning. However, this would be awful for various reasons. We COULD assume that anything that declared a namespace In the end, I came down on, "The one who best knows that they aren't using any headers from the standard library are the ones who wrote the code, so they should be the ones who assert one way or another via a flag". And the -Wno flag seems to be as good as any. I Strongly suspect that any effort to recognize an StdLib header is going to be fairly wasted, and likely not as accurate as "always assume they are going to use the Standard Library with this file... somewhere". |
|
Oh, I see, you're suggesting we remove the But I'd somewhat question whether this PR and warning really has anything to do with the attribute names being "reserved" at that point -- we're not checking whether they're reserved or not, and it really doesn't matter. Warning on a |
Yep, that is my suggestion, sorry I was insufficiently clear.
I agree with this 100%. The link to the 'reserved by the standard' is I think a good additional justification. I think the proposal, complaining about these as reserved, is a good idea/good patch. BUT I think getting caught up in the "well, when is it technically NOT UB" is a waste of time, given that the warning is a good idea even without that justification. |
I think the warning is justified even without a standard library header being included, but I also wonder if that means putting this under WDYT? |
First, I think this needs its OWN warning group, as I can see justification for disabling just this. Second, I think that the 'standard' attribute interference is a level of severity HIGHER than the others thanks to being in the standard (and thus, perhaps, likely more to be used/interfered with). Third: DOES annotate conflict with attribute annotate? Isn't that a function/would have to be a function macro? Fourth: I think the 'reserved name' has a level of gravitas/concreteness that makes the diagnostic more meaningful/immediately obvious to folks. A diagnostic of, "This name you chose for your macro might make this attribute no workie" yields "Yeah, but i wont use that so I'm ok". A diagnostic of, "This name is UB because the standard reserves it!" yields a level of pause/consideration that we otherwise wouldn't get. SO I think the diagnostic being associated with it being reserved is COMPLETELY valid/justifiable. I think "this is a bad idea, UB or not" is a reason to not try at all to suppress this if we think you're not ACTUALLY breaking the rule (by not including an StdLib header). |
Agreed, I didn't mean to imply otherwise, I was speaking about the top-level umbrella (sorry for the confusion).
I think the problem is the same regardless of whether the attribute is a standard attribute or a vendor attribute -- either way, redefining the meaning of something known to the implementation seems worth of letting the user know about.
Ah, good point, but the basic premise still stands.
Yes, but the point is: these names aren't reserved but the pain can still happen. e.g.,
If we're only going to diagnose conflicts with standard attribute names, then I can squint enough to agree. But I think that's an arbitrary limitation; we all just agreed that this isn't about using a reserved identifier so much as about writing a macro which will conflict with uses of attributes of the same name. |
a-tarasyuk
force-pushed
the
feat/92196
branch
from
2064013 to
ef0f355
Compare
|
@AaronBallman @erichkeane I’ve switched back to the version without the std check. should the diagnostic group be changed? |
I think we're fine to leave it as-is. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/50/builds/9302 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/20722 Here is the relevant piece of the build log for the reference |
Fixes #92196
https://eel.is/c++draft/macro.names#2