Page MenuHomePhabricator

Introduce -Wreserved-identifier
ClosedPublic

Authored by serge-sans-paille on Dec 11 2020, 2:03 AM.

Details

Summary

Warn when a declaration uses an identifier that doesn't obey the reserved
identifier rule from C and/or C++.

Do not perform the check at preprocessor level because:

  1. it will be quite costly
  2. we don't have accurate linkage info at that point

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
rsmith added inline comments.Feb 3 2021, 3:42 PM
clang/test/Sema/reserved-identifier.c
14

It'd be useful to test that we don't diagnose

void foo(unsigned int _not_reserved) { ... }
32

You don't seem to have a test for TUK_Reference:

struct _Zebulon3 *p;

(Nor for TUK_Friend.)

50–51

_preserved is not an identifier with file scope, so I think we shouldn't warn here. Perhaps the problem is that we're doing this check before the struct gets reparented into the function declaration (which only happens after we finish parsing all the parameters and build the function declaration).

We could perhaps check the Scope in addition to checking the DeclContext to detect such cases.

54

You mean, specifically for _strdup? In general, this looks exactly the like the kind of declaration we'd want to warn on. If we don't want a warning here, we could perhaps recognize _strdup as a builtin lib function. (I think they shouldn't warn, because we'll inject a builtin declaration, so this would be a redeclaration. We should test that!)

clang/test/Sema/reserved-identifier.cpp
22

Please also test the _not_reserved case here. Parameters are weird, because they're parsed before we have a DeclContext created to correspond with their scope, so they start off with the translation unit as their parent.

64

This should warn: this declaration would conflict with an int _barbeFleurie(); in a system header.

... although weirdly Clang doesn't diagnose that conflict. Hm. That looks like a bug.

mizvekov added a subscriber: mizvekov.EditedFeb 3 2021, 4:29 PM

There is a GNU extension where the linker provides symbols for section / start end, and using these with this patch would produce a warning.
Example:

[[gnu::section("foo")]] void bar() {}
extern "C" void __start_foo();
extern "C" void __stop_foo();
aaron.ballman added inline comments.Feb 4 2021, 5:22 AM
clang/test/Sema/reserved-identifier.c
50–51

_preserved is not an identifier with file scope, so I think we shouldn't warn here.

Hmm, my thinking was that if the implementation added a conflicting definition of what _preserved is, it'd break the user's code. But you're right, the identifier is in the tag name space but not with file scope, so not warning is correct.

54

You mean, specifically for _strdup?

Yes, but it's a more general pattern than just _strdup. C code (esp older C code) will sometimes add an external declaration to avoid having to use a #include for just one symbol. Additionally, some popular platforms (like Windows) add a bunch of functions in the implementation namespace like _strdup (and many, many others).

Perhaps an option would be to always warn, and if we find that users run into this situation a lot in practice, we could split the diagnostic so that users can control whether to warn on forward declarations of functions.

Patch rebased on main.

Updated error message to report the reason why an identifier is reserved.

Fix some formatting
Support literal operator

aaron.ballman added inline comments.Mar 16 2021, 12:58 PM
clang/include/clang/AST/Decl.h
82

Because this is not a scoped enum, should these enumerators get an RIR_ prefix added to them?

361–362

Any reason not to return the enumeration as the result? We could add NotReserved = 0 to the enumeration so that calls to D->isReserved() will do the right thing.

(Also, might as well name the LangOptions parameter here.)

clang/include/clang/Basic/DiagnosticGroups.td
806

Were you planning to use this new diagnostic group?

clang/lib/AST/Decl.cpp
1084

Might as well address the lint warning.

1104–1105

Adress final(?) comments from @rsmith and @aaron.ballman :
Don't warn on argument (or template argument) of top-level decl.
Extra test cases
Return an enumeration as part of the test

serge-sans-paille marked 10 inline comments as done.Mar 29 2021, 4:12 AM
serge-sans-paille added inline comments.
clang/include/clang/AST/Decl.h
82

I used a class enum with a stream operator instead. Not a big fan of prefixed enum ;-)

clang/test/Sema/reserved-identifier.c
54

yeah,that would require checking if the name with trailing underscores is a libc function. I agree we should wait for more user feedback to install such check.

aaron.ballman added inline comments.Mar 30 2021, 8:55 AM
clang/include/clang/AST/Decl.h
82

Totally fine by me, thank you!

clang/include/clang/Basic/DiagnosticSemaKinds.td
384

You may want to add a comment here to alert people to the fact that passing 0 for %1 is not valid but the | exists to make the enum order match the diagnostic. Otherwise it looks like it's possible to get a diagnostic identifier 'whatever' is reserved because .

clang/lib/AST/Decl.cpp
1105–1111
1118–1120
1125–1127
clang/lib/Sema/SemaDecl.cpp
5564

Still missing a full stop at the end of the comment here.

5567–5568

Richard's comment is still unaddressed as well.

13736

I'm still wondering about this as well.

clang/lib/Sema/SemaStmt.cpp
546

This check should be reversed as well.

serge-sans-paille marked 10 inline comments as done.

Address all the needed changes pointed out by @aaron.ballman *except* the most critical one on the call to warnOnReservedIdentifier being spread at several place in Sema. I'll try to find a better approach for that particular point.

clang/lib/Sema/SemaDecl.cpp
13736

I'll investigate that one more deeply, I do share your concern.

rsmith added inline comments.Mar 30 2021, 4:22 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
384

This is hopefully more useful to future readers and also terser. (I like including <ERROR> in the diagnostic in the bad case because if it ever happens we're more likely to get a bug report that way.)

clang/lib/AST/Decl.cpp
1084

Please reorder the literal operator case after the plain getIdentifier case. I think this'd be more readable if the common and expected case is first, and it doesn't make any functional difference since a literal operator doesn't have a "regular" identifier name.

1097

Would it make sense to move the rest of this function to a member on IdentifierInfo? That is, the rest of this function would become:

ReservedIdentifierStatus Status = II->isReserved(LangOpts);
if (Status == ReservedIdentifierStatus::StartsWithUnderscoreAtGlobalScope &&
    isa<ParmVarDecl>(this) || isTemplateParameter() ||
    !getDeclContext()->getRedeclContext()->isTranslationUnit())
  return ReservedIdentifierStatus::NotReserved;
return Status;

That way, we should be able to reuse the same implementation to detect reserved macros too.

1113–1119

This can be simplified a bit; I suggested edits above. Also, we want the semantic parent here, not the lexical parent, in order to warn on cases like

struct A {
  friend struct _b; // warning, starts with underscore at global scope
  friend void _f(); // warning, starts with underscore at global scope
};
void g() {
  void _h(); // warning, starts with underscore at global scope
  extern int _n; // warning, starts with underscore at global scope
}

Another round of review :-) I addressed both the scattering of warnOnReservedIdentifier and the code split between Decl.cpp and IdentifierInfo.cpp.

clang/lib/AST/Decl.cpp
1097

I did the II->isReserved(LangOpts) part, but the updated condition !getDeclContext()->getRedeclContext()->isTranslationUnit() fails the validation. I did already spend a lot of time getting one that catches the behavior we wanted, so 'm not super-eager to investigate why this change would not work ;-)

rsmith added inline comments.Mar 31 2021, 10:48 AM
clang/lib/AST/Decl.cpp
1097

The current lexical context check does not look correct -- the lexical context is not relevant here, because the language rule is constrained by the semantic context. What test cases does the new condition fail?

Please also add something like the testcases from my other comment; I can't find any tests for friends or for local extern declarations in the current set of tests, and that's the kind of corner case that would be affected by this.

Do not use lexical parent, as suggested by @rsmith
Add test case for extern function worward-declared in function body, as suggested by @rsmith

Warn on friend functions. I failed to support friend classes, but they are only declared and not defined, so that should be fine, right?

serge-sans-paille marked 3 inline comments as done.Apr 1 2021, 6:00 AM
aaron.ballman added inline comments.Apr 1 2021, 7:49 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
384

Ooh, I like that suggestion, thanks!

clang/lib/AST/Decl.cpp
1084

It looks like this code still needs to move.

1096

This comment should be updated -- we no longer walk the lexical parents.

clang/lib/Basic/IdentifierTable.cpp
297

I think comments here may help -- we don't know that the identifier is at global scope within this call, so the return value looks a bit strange. This is a case where the enumerator name is a bit awkward in this context but makes a lot of sense in the context of the check.

clang/lib/CodeGen/CGDebugInfo.cpp
4060–4063

There's a slight functionality change here in that the code used to allow identifiers that start with a single underscore followed by a lowercase letter and that now early returns. Is that expected (and tested)?

clang/lib/Sema/SemaDecl.cpp
5568–5569
clang/lib/Sema/SemaDeclCXX.cpp
11031

Spurious whitespace change.

12704

Spurious whitespace change.

16781

Doesn't PushOnScopeChains() do this already?

clang/lib/Sema/SemaStmt.cpp
546

This still needs to be reversed so we check for system headers after checking the reserved status.

clang/lib/Sema/SemaTemplate.cpp
1680–1681

Is this handled by PushOnScopeChains()?

Warn on friend functions. I failed to support friend classes, but they are only declared and not defined, so that should be fine, right?

They can still conflict, so I think we should warn. For example:

// user code
struct A { friend struct _b; };
// system header
enum _b {}; // error
clang/include/clang/AST/Decl.h
81–82

This should be declared with the definition of ReservedIdentifierStatus -- or simply removed. We usually include the cast at the diagnostic emission site, which I find makes it locally clear that we're making an assumption about the numerical values of the enumerators.

clang/include/clang/Basic/DiagnosticGroups.td
806

Some of the uses of this warning are for non-external identifiers. It'd also be nice for the diagnostics in this area to have consistent names. Currently we have: -Wreserved-id-macro, -Wreserved-extern-identifier, -Wreserved-identifier. I'd like to see either consistent use of "identifier" or consistent use of "id' (preferably "identifier") and consistent word order.

clang/lib/CodeGen/CGDebugInfo.cpp
4060–4063

Should we instead be asking whether CalleeDecl has a reserved name, now that we can?

clang/lib/Sema/SemaCodeComplete.cpp
743

Should we be using ND->isReserved here?

clang/lib/Sema/SemaDecl.cpp
5568–5569

I'm surprised we don't warn in user headers. Is that intentional?

serge-sans-paille marked 10 inline comments as done.

Take into account latest reviews

aaron.ballman accepted this revision.May 3 2021, 5:47 AM

Aside from the formatting nit and the question about the diagnostic, I think this LGTM!

clang/include/clang/Basic/DiagnosticGroups.td
641

Do we want to keep this old name around as an alias to the new name (this makes upgrades easier on users who mention the diagnostic by name in their build script or within diagnostic pragmas)?

clang/lib/AST/Decl.cpp
1084

The formatting issue is worth fixing.

This revision is now accepted and ready to land.May 3 2021, 5:47 AM
This revision was landed with ongoing or failed builds.May 4 2021, 2:20 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 4 2021, 2:20 AM