Handle interactions between reserved identifier and user-defined suffixes

[serge-sans-paille]

Summary

According to https://eel.is/c++draft/over.literal

double operator""_Bq(long double); OK: does not use the reserved identifier _­Bq ([lex.name])
double operator"" _Bq(long double); ill-formed, no diagnostic required: uses the reserved identifier _­Bq ([lex.name])

Obey that rule by keeping track of the operator literal name status wrt. leading whitespace.

Fix: https://bugs.llvm.org/show_bug.cgi?id=50644

[aaron.ballman]

clang/include/clang/Basic/IdentifierTable.h

124 ↗

(On Diff #352146)

I'm on the fence about adding "Leading" to the variable and function names as well. I figure the "leading" makes it clear that we're talking about the whitespace between the operator and the string literal and nowhere else.

127 ↗

(On Diff #352146)

I keep coming up with 25 bits (39 bits in use), so I think this got out of sync at some point.

385 ↗

(On Diff #352146)

390 ↗

(On Diff #352146)

clang/lib/Basic/IdentifierTable.cpp

289 ↗

(On Diff #352146)

You should fix the clang-format issue.

clang/test/Sema/reserved-identifier.cpp

89

Can you add comments about why there's no warning, as this behavior may be a bit surprising to folks. Bonus points for the comment having a link back to the standard.

[rsmith]

clang/include/clang/Basic/IdentifierTable.h

125 ↗

(On Diff #352146)

This is per-declaration state; I don't think we can reasonably store it on IdentifierInfo, which is global information about an identifier. This will do the wrong thing if (for example) literal operators with the same suffix but different whitespace are declared in different scopes or if the same operator is multiply declared with different whitespace in the same scope.

[serge-sans-paille]

Tried another not-so-intrusive approach. @rsmith?

[rsmith]

Thanks, looks good. Just some minor comments.

clang/include/clang/Sema/Sema.h

4132

Style nit: please start variables with uppercase letters.

clang/lib/AST/Decl.cpp

1083–1084

Can you now delete this along with the added code below?

1090

Style nit: comments should start with a capital letter and end in a period.

clang/lib/Sema/SemaExprCXX.cpp

489–499

Hmm, interesting. This differs from the behavior of regular identifiers in that it will warn on both declarations and uses of reserved literal operator names. But I think that's actually a desirable difference! For example:

int _Foo(); // should warn here (if not in system header)
int k1 = _Foo(); // no need to warn here, and we shouldn't if the previous line is in a system header

int operator""_Foo();
int k2 = operator"" _Foo(); // should warn here, regardless of whether the previous line is in a system header

Given that each appearance can make a different choice in this regard, and that the problem can be fixed locally by removing the space, warning on each appearance seems like the right approach.

490

It's useful to also include a brief quotation of the relevant text and a standard version, since paragraphs get moved around and renumbered over time.

497

Can we produce a FixItHint to remove the space? (That might require the parser to pass through a little more information, such as the location for the end of the final string literal token, so we can reconstruct the space range here.)

[serge-sans-paille]

Reviews taken into account, I'm just not 100% sure of the fixit part. The out is the following:

a.cpp:4:16: warning: identifier '_Bye' is reserved because it starts with '_' followed by a capital letter [-Wreserved-identifier]
int operator"" _Bye(const char* s) {
    ~~~~~~~~~~~^~~~
    operator""_Bye
1 warning generated.

Does that look good to you?

[aaron.ballman]

clang/lib/Parse/ParseExprCXX.cpp

2639

Minor style nits

clang/lib/Sema/SemaExprCXX.cpp

487

491–493

The flow of the previous comments was basically unreadable. I think this is more readable, but at the expense of being slightly different from the standard's example comments.

495–496

Please don't use auto without the type being spelled out in the initialization.

499

Because this situation is IFNDR, do we want to upgrade this from a warning to an error (that's controlled by the warning flag)?

clang/test/Sema/reserved-identifier.cpp

79–84

Can you add test cases that use the suffix as well? Richard had an especially interesting example:

int operator""_Foo();
int k2 = operator"" _Foo(); // should warn here, regardless of whether the previous line is in a system header

[aaron.ballman]

Does that look good to you?

That fix-it looks correct to me.

clang/include/clang/Sema/Sema.h

public:
   CXXMethodDecl *LookupCopyingAssignment(CXXRecordDecl *Class, unsigned Quals,
                                          bool RValueThis, unsigned ThisQuals);
   CXXConstructorDecl *LookupMovingConstructor(CXXRecordDecl *Class,
                                               unsigned Quals);
   CXXMethodDecl *LookupMovingAssignment(CXXRecordDecl *Class, unsigned Quals,
                                         bool RValueThis, unsigned ThisQuals);
   CXXDestructorDecl *LookupDestructor(CXXRecordDecl *Class);
 
-  bool checkLiteralOperatorId(const CXXScopeSpec &SS, const UnqualifiedId &Id);
+  bool checkLiteralOperatorId(const CXXScopeSpec &SS, const UnqualifiedId &Id,
+                              bool IsUDSuffix);
   LiteralOperatorLookupResult
   LookupLiteralOperator(Scope *S, LookupResult &R, ArrayRef<QualType> ArgTys,
                         bool AllowRaw, bool AllowTemplate,
                         bool AllowStringTemplate, bool DiagnoseMissing,
                         StringLiteral *StringLit = nullptr);
   bool isKnownName(StringRef name);
 
   /// Status of the function emission on the CUDA/HIP/OpenMP host/device attrs.

clang/lib/AST/Decl.cpp

bool NamedDecl::isLinkageValid() const {
   Linkage L = LinkageComputer{}
                   .computeLVForDecl(this, LVComputationKind::forLinkageOnly())
                   .getLinkage();
   return L == getCachedLinkage();
 }
 
 ReservedIdentifierStatus
 NamedDecl::isReserved(const LangOptions &LangOpts) const {
   const IdentifierInfo *II = getIdentifier();
-  if (!II)
-    if (const auto *FD = dyn_cast<FunctionDecl>(this))
-      II = FD->getLiteralIdentifier();
 
+  // This triggers at least for CXXLiteralIdentifiers, which we already checked
+  // at lexing time.
   if (!II)
     return ReservedIdentifierStatus::NotReserved;
 
   ReservedIdentifierStatus Status = II->isReserved(LangOpts);
   if (Status == ReservedIdentifierStatus::StartsWithUnderscoreAtGlobalScope) {
     // Check if we're at TU level or not.
     if (isa<ParmVarDecl>(this) || isTemplateParameter())
       return ReservedIdentifierStatus::NotReserved;
     const DeclContext *DC = getDeclContext()->getRedeclContext();
     if (!DC->isTranslationUnit())
       return ReservedIdentifierStatus::NotReserved;
   }

clang/lib/Parse/ParseExprCXX.cpp

 //===--- ParseExprCXX.cpp - C++ Expression Parsing ------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements the Expression parsing implementation for C++.
 //
 //===----------------------------------------------------------------------===//
-#include "clang/Parse/Parser.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/Basic/PrettyStackTrace.h"
 #include "clang/Lex/LiteralSupport.h"
 #include "clang/Parse/ParseDiagnostic.h"
+#include "clang/Parse/Parser.h"
 #include "clang/Parse/RAIIObjectsForParser.h"
 #include "clang/Sema/DeclSpec.h"
 #include "clang/Sema/ParsedTemplate.h"
 #include "clang/Sema/Scope.h"
 #include "llvm/Support/ErrorHandling.h"
 #include <numeric>
 
 using namespace clang;
if (getLangOpts().CPlusPlus11 && isTokenStringLiteral()) {
     }
 
     StringLiteralParser Literal(Toks, PP);
     if (Literal.hadError)
       return true;
 
     // Grab the literal operator's suffix, which will be either the next token
     // or a ud-suffix from the string literal.
+    const bool isUDSuffix = !Literal.getUDSuffix().empty();
     IdentifierInfo *II = nullptr;
     SourceLocation SuffixLoc;
-    if (!Literal.getUDSuffix().empty()) {
+    if (isUDSuffix) {
       II = &PP.getIdentifierTable().get(Literal.getUDSuffix());
       SuffixLoc =
         Lexer::AdvanceToTokenCharacter(TokLocs[Literal.getUDSuffixToken()],
                                        Literal.getUDSuffixOffset(),
                                        PP.getSourceManager(), getLangOpts());
     } else if (Tok.is(tok::identifier)) {
       II = Tok.getIdentifierInfo();
       SuffixLoc = ConsumeToken();
if (DiagId) {
       Str += "\"\"";
       Str += II->getName();
       Diag(DiagLoc, DiagId) << FixItHint::CreateReplacement(
           SourceRange(TokLocs.front(), TokLocs.back()), Str);
     }
 
     Result.setLiteralOperatorId(II, KeywordLoc, SuffixLoc);
 
-    return Actions.checkLiteralOperatorId(SS, Result);
+    return Actions.checkLiteralOperatorId(SS, Result, isUDSuffix);
   }
 
   // Parse a conversion-function-id.
   //
   //   conversion-function-id: [C++ 12.3.2]
   //     operator conversion-type-id
   //
   //   conversion-type-id:

clang/lib/Sema/SemaExprCXX.cpp

Diag(DS.getTypeSpecTypeLoc(), diag::err_destructor_expr_type_mismatch)
       << T << SearchType;
     return nullptr;
   }
 
   return ParsedType::make(T);
 }
 
 bool Sema::checkLiteralOperatorId(const CXXScopeSpec &SS,
-                                  const UnqualifiedId &Name) {
+                                  const UnqualifiedId &Name, bool isUDSuffix) {
   assert(Name.getKind() == UnqualifiedIdKind::IK_LiteralOperatorId);
+  if (!isUDSuffix) {
+    // [over.literal] p8
+    // double operator""_Bq(long double);                  // OK: does not use
+    // the reserved identifier _­Bq ([lex.name]) double operator"" _Bq(long
+    // double);                 // ill-formed, no diagnostic required:
+    IdentifierInfo *II = Name.Identifier;
+    auto Status = II->isReserved(PP.getLangOpts());
+    auto Loc = Name.getEndLoc();
+    if (Status != ReservedIdentifierStatus::NotReserved &&
+        !PP.getSourceManager().isInSystemHeader(Loc)) {
+      Diag(Loc, diag::warn_reserved_extern_symbol)
+          << II << static_cast<int>(Status)
+          << FixItHint::CreateReplacement(
+                 Name.getSourceRange(),
+                 (StringRef("operator\"\"") + II->getName()).str());
+    }
+  }
 
   if (!SS.isValid())
     return false;
 
   switch (SS.getScopeRep()->getKind()) {
   case NestedNameSpecifier::Identifier:
   case NestedNameSpecifier::TypeSpec:
   case NestedNameSpecifier::TypeSpecWithTemplate:

clang/test/Sema/reserved-identifier.cpp

 int foobar() {
   return foo__bar(); // no-warning
 }
 
 namespace {
 int _barbatruc; // no-warning
 }
 
-long double operator"" _BarbeBleue(long double) // expected-warning {{identifier 'operator""_BarbeBleue' is reserved because it starts with '_' followed by a capital letter}}
+long double operator"" _BarbeBleue(long double) // expected-warning {{identifier '_BarbeBleue' is reserved because it starts with '_' followed by a capital letter}}
+{
+  return 0.;
+}
+
+long double operator""_SacreBleue(long double) // no-warning
 {
   return 0.;
 }
 
 struct _BarbeRouge { // expected-warning {{identifier '_BarbeRouge' is reserved because it starts with '_' followed by a capital letter}}
 } p;
 struct _BarbeNoire { // expected-warning {{identifier '_BarbeNoire' is reserved because it starts with '_' followed by a capital letter}}
 } * q;
 
 struct Any {
   friend void _barbegrise(); // expected-warning {{identifier '_barbegrise' is reserved because it starts with '_' at global scope}}
 };