[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