[PATCH] Provide fixit if unscoped enumeration is used in nested name specifier. This fixes PR16951.

Aaron Ballman aaron at aaronballman.com
Fri Jan 2 14:30:25 PST 2015


On Mon, Dec 22, 2014 at 3:27 PM, Serge Pavlov <sepavloff at gmail.com> wrote:
> Any feedback?

Sorry for the delayed review -- holiday season can be a bit hectic.
Comments below.

> Index: include/clang/Basic/DiagnosticParseKinds.td
> ===================================================================
> --- include/clang/Basic/DiagnosticParseKinds.td
> +++ include/clang/Basic/DiagnosticParseKinds.td
> @@ -539,6 +539,10 @@
>    "unexpected %0 in function call; perhaps remove the %0?">;
>  def err_super_in_using_declaration : Error<
>    "'__super' cannot be used with a using declaration">;
> +def err_nested_name_spec_is_enum : Error<
> +  "unscoped enumeration name shall not be specified in nested name specifier">;

We don't typically use "shall" in our diagnostics.

"unscoped enumeration %0 cannot be specified as a nested name specifier" ?

> +def err_expected_class_or_namespace_or_enum : Error<"%0 is not a class"
> +  "%select{ or namespace|, namespace, or scoped enumeration}1">;

Neither of these strike me as being parsing diagnostics, but instead
as semantic ones. In both cases, the identifier is correct from a
parsing perspective, but the semantic restrictions are not met. Is
there a reason this is not implemented within
Sema::ActOnCXXNestedNameSpecifier?

>
>  // C++ derived classes
>  def err_dup_virtual : Error<"duplicate 'virtual' in base specifier">;
> Index: include/clang/Sema/Sema.h
> ===================================================================
> --- include/clang/Sema/Sema.h
> +++ include/clang/Sema/Sema.h
> @@ -4610,6 +4610,23 @@
>                                      IdentifierInfo &II,
>                                      ParsedType ObjectType);
>
> +  /// \brief Collects information that may be used to recover erroneous nested
> +  /// name specifier.
> +  struct NameSpecRecoveryInfo {
> +    bool CorrectToColon; //> suggestions to replace '::' -> ':' are allowed
> +    bool CorrectionToColonRequested; //> Replacement '::' -> ':' recovers error
> +    TypeDecl *FoundDecl; //> Declaration if found for name spec

Comments should be proper sentences (capitalization and punctuation).

> +
> +    NameSpecRecoveryInfo(bool CorrectColon) {

This should probably be an explicit constructor since it only takes a
single bool parameter.

> +      clear();
> +      CorrectToColon = CorrectColon;

Would be better to use an initializer list.

> +    }
> +    void clear() {
> +      CorrectToColon = CorrectionToColonRequested = false;
> +      FoundDecl = nullptr;
> +    }

This method is never called from anywhere, so it should probably be removed.

> +  };
> +
>    bool BuildCXXNestedNameSpecifier(Scope *S,
>                                     IdentifierInfo &Identifier,
>                                     SourceLocation IdentifierLoc,
> @@ -4619,7 +4636,7 @@
>                                     CXXScopeSpec &SS,
>                                     NamedDecl *ScopeLookupResult,
>                                     bool ErrorRecoveryLookup,
> -                                   bool *IsCorrectedToColon = nullptr);
> +                                   NameSpecRecoveryInfo *RecovInfo = nullptr);

As best I can tell, this does not need to be an optional parameter
(the only callers of this overload of the function all pass in a
nonnull pointer) -- instead, RecovInfo could be accepted by reference.
(The desire for colon correction is already encoded in the
NameSpecRecoveryInfo object itself.)

>
>    /// \brief The parser has parsed a nested-name-specifier 'identifier::'.
>    ///
> @@ -4645,9 +4662,8 @@
>    /// \param ErrorRecoveryLookup If true, then this method is called to improve
>    /// error recovery. In this case do not emit error message.
>    ///
> -  /// \param IsCorrectedToColon If not null, suggestions to replace '::' -> ':'
> -  /// are allowed.  The bool value pointed by this parameter is set to 'true'
> -  /// if the identifier is treated as if it was followed by ':', not '::'.
> +  /// \param RecovInfo If not null, points to a structure that is contains info

Spurious "is".

> +  /// for error recovery.
>    ///
>    /// \returns true if an error occurred, false otherwise.
>    bool ActOnCXXNestedNameSpecifier(Scope *S,
> @@ -4658,7 +4674,7 @@
>                                     bool EnteringContext,
>                                     CXXScopeSpec &SS,
>                                     bool ErrorRecoveryLookup = false,
> -                                   bool *IsCorrectedToColon = nullptr);
> +                                   NameSpecRecoveryInfo *RecovInfo = nullptr);
>
>    ExprResult ActOnDecltypeExpression(Expr *E);
>
> Index: lib/Parse/ParseExprCXX.cpp
> ===================================================================
> --- lib/Parse/ParseExprCXX.cpp
> +++ lib/Parse/ParseExprCXX.cpp
> @@ -472,20 +472,47 @@
>
>        CheckForLParenAfterColonColon();
>
> -      bool IsCorrectedToColon = false;
> -      bool *CorrectionFlagPtr = ColonIsSacred ? &IsCorrectedToColon : nullptr;
> +      Sema::NameSpecRecoveryInfo RecovInfo(ColonIsSacred);
>        if (Actions.ActOnCXXNestedNameSpecifier(getCurScope(), II, IdLoc, CCLoc,
>                                                ObjectType, EnteringContext, SS,
> -                                              false, CorrectionFlagPtr)) {
> +                                              false, &RecovInfo)) {
>          // Identifier is not recognized as a nested name, but we can have
>          // mistyped '::' instead of ':'.
> -        if (CorrectionFlagPtr && IsCorrectedToColon) {
> +        if (RecovInfo.CorrectToColon && RecovInfo.CorrectionToColonRequested) {
>            ColonColon.setKind(tok::colon);
>            PP.EnterToken(Tok);
>            PP.EnterToken(ColonColon);
>            Tok = Identifier;
>            break;
>          }
> +        if (RecovInfo.FoundDecl) {
> +          // The current identifier does not represents entity that may be used

"does not represent an entity"

> +          // as nested name spec, it nevertheless resolves to a declaration. See
> +          // if we can do error recover here.
> +          bool ErrorReported = false;
> +          if (EnumDecl *ED = dyn_cast<EnumDecl>(RecovInfo.FoundDecl)) {
> +            // Maybe non scoped enum is used as scoped?  If so this identifier
> +            // must be followed by another, which is corresponding enumerator.
> +            if (!ED->isScoped() && Tok.is(tok::identifier) &&
> +                NextToken().isNot(tok::coloncolon)) {
> +              IdentifierInfo *Id = Tok.getIdentifierInfo();
> +              for (auto Enumerator : ED->enumerators()) {
> +                if (Enumerator->getName().equals(Id->getName())) {
> +                  // We can recover parsing by throwing away enumeration name.
> +                  Diag(IdLoc, diag::err_nested_name_spec_is_enum) <<
> +                      FixItHint::CreateRemoval(SourceRange(IdLoc, CCLoc));
> +                  ErrorReported = true;
> +                  break;
> +                }
> +              }
> +            }
> +          }
> +          if (!ErrorReported) {
> +            Diag(IdLoc, diag::err_expected_class_or_namespace_or_enum)
> +              << QualType(RecovInfo.FoundDecl->getTypeForDecl(), 0)
> +              << getLangOpts().CPlusPlus;
> +          }

Can elide the braces from the if statement.

> +        }
>          SS.SetInvalid(SourceRange(IdLoc, CCLoc));
>        }
>        HasScopeSpecifier = true;
> Index: lib/Sema/SemaCXXScopeSpec.cpp
> ===================================================================
> --- lib/Sema/SemaCXXScopeSpec.cpp
> +++ lib/Sema/SemaCXXScopeSpec.cpp
> @@ -427,10 +427,8 @@
>  ///        definition time.
>  /// \param ErrorRecoveryLookup Specifies if the method is called to improve
>  ///        error recovery and what kind of recovery is performed.
> -/// \param IsCorrectedToColon If not null, suggestion of replace '::' -> ':'
> -///        are allowed.  The bool value pointed by this parameter is set to
> -///       'true' if the identifier is treated as if it was followed by ':',
> -///        not '::'.
> +/// \param RecovInfo If not null, points to a structure that contains info
> +///        for error recovery.
>  ///
>  /// This routine differs only slightly from ActOnCXXNestedNameSpecifier, in
>  /// that it contains an extra parameter \p ScopeLookupResult, which provides
> @@ -452,15 +450,17 @@
>                                         CXXScopeSpec &SS,
>                                         NamedDecl *ScopeLookupResult,
>                                         bool ErrorRecoveryLookup,
> -                                       bool *IsCorrectedToColon) {
> +                                       NameSpecRecoveryInfo *RecovInfo) {
>    LookupResult Found(*this, &Identifier, IdentifierLoc,
>                       LookupNestedNameSpecifierName);
>
>    // Determine where to perform name lookup
>    DeclContext *LookupCtx = nullptr;
>    bool isDependent = false;
> -  if (IsCorrectedToColon)
> -    *IsCorrectedToColon = false;
> +  if (RecovInfo) {
> +    RecovInfo->CorrectionToColonRequested = false;
> +    RecovInfo->FoundDecl = nullptr;
> +  }

This shouldn't be required given that the constructor already clears
these, and the structure is not meant to be long-lived.

>    if (!ObjectType.isNull()) {
>      // This nested-name-specifier occurs in a member access expression, e.g.,
>      // x->B::f, and we are looking into the type of the object.
> @@ -553,8 +553,8 @@
>      if (!R.empty()) {
>        // The identifier is found in ordinary lookup. If correction to colon is
>        // allowed, suggest replacement to ':'.
> -      if (IsCorrectedToColon) {
> -        *IsCorrectedToColon = true;
> +      if (RecovInfo && RecovInfo->CorrectToColon) {
> +        RecovInfo->CorrectionToColonRequested = true;
>          Diag(CCLoc, diag::err_nested_name_spec_is_not_class)
>              << &Identifier << getLangOpts().CPlusPlus
>              << FixItHint::CreateReplacement(CCLoc, ":");
> @@ -754,10 +754,13 @@
>    }
>
>    if (!Found.empty()) {
> -    if (TypeDecl *TD = Found.getAsSingle<TypeDecl>())
> -      Diag(IdentifierLoc, diag::err_expected_class_or_namespace)
> +    if (TypeDecl *TD = Found.getAsSingle<TypeDecl>()) {
> +      if (RecovInfo)
> +        RecovInfo->FoundDecl = TD;
> +      else
> +        Diag(IdentifierLoc, diag::err_expected_class_or_namespace)
>            << QualType(TD->getTypeForDecl(), 0) << getLangOpts().CPlusPlus;
> -    else {
> +    } else {
>        Diag(IdentifierLoc, diag::err_expected_class_or_namespace)
>            << &Identifier << getLangOpts().CPlusPlus;
>        if (NamedDecl *ND = Found.getAsSingle<NamedDecl>())
> @@ -780,15 +783,15 @@
>                                         bool EnteringContext,
>                                         CXXScopeSpec &SS,
>                                         bool ErrorRecoveryLookup,
> -                                       bool *IsCorrectedToColon) {
> +                                       NameSpecRecoveryInfo *RecovInfo) {
>    if (SS.isInvalid())
>      return true;
>
>    return BuildCXXNestedNameSpecifier(S, Identifier, IdentifierLoc, CCLoc,
>                                       GetTypeFromParser(ObjectType),
>                                       EnteringContext, SS,
>                                       /*ScopeLookupResult=*/nullptr, false,
> -                                     IsCorrectedToColon);
> +                                     RecovInfo);
>  }
>
>  bool Sema::ActOnCXXNestedNameSpecifierDecltype(CXXScopeSpec &SS,
> Index: test/SemaCXX/nested-name-spec.cpp
> ===================================================================
> --- test/SemaCXX/nested-name-spec.cpp
> +++ test/SemaCXX/nested-name-spec.cpp
> @@ -116,7 +116,7 @@
>      };
>
>      void f() {
> -      return E::X; // expected-error{{'E::Nested::E' is not a class, namespace, or scoped enumeration}}
> +      return E::X; // expected-error{{unscoped enumeration name shall not be specified in nested name specifier}}

I find the original diagnostic to be more clear than the new one. I
also find the test case *really* strange since f() returns void. :-P

>      }
>    }
>  }
> @@ -410,3 +410,15 @@
>  };
>
>  }
> +
> +
> +namespace PR16951 {
> +  namespace ns {
> +    enum an_enumeration {
> +      ENUMERATOR
> +    };
> +  }
> +
> +  int x1 = ns::an_enumeration::ENUMERATOR; // expected-error{{unscoped enumeration name shall not be specified in nested name specifier}}
> +  int x2 = ns::an_enumeration::ENUMERATOR::vvv; // expected-error{{'PR16951::ns::an_enumeration' is not a class, namespace, or scoped enumeration}}
> +}
>

~Aaron



More information about the cfe-commits mailing list