[cfe-dev] using declarations

[Douglas Gregor]

Hello John,

On Apr 16, 2009, at 5:20 PM, John Thompson wrote:
> I've been experimenting with an implementation for using  
> declarations that I'd like to bounce off you, to make sure I'm going  
> down the right path.  I've enclosed a patch of the current changes,  
> in case you want to use them now.  These are a high-level  
> description of the key changes/additions so far:
>
> 1. Create a new decl class, UsingAliasDecl, derived form NamedDecl,  
> mainly storing a pointer to the target decl.
>
> 2. Add parsing code to Parser::ParseUsingDeclaration.  (Note: the  
> 'typename' keyword is parsed, but the semantic actions elsewhere  
> don't support type using decls.  No attempt to parse the 'concept'  
> stuff yet)
>
> 3. Handle parsing using declarations in classes  
> (Parser::ParseCXXClassMemberDeclaration).
>
> 4. Define and implement Action::ActOnUsingDeclaration (both  
> MinimalAction.cpp and SemaDeclCXX.cpp).
>
> 5. Where the declared using identifier is used in expressions (i.e.  
> Sema::ActOnDeclarationNameExpr), substitute the target decl, i.e.:
>
>   if (UsingAliasDecl *UsingAlias = dyn_cast<UsingAliasDecl>(D))
>     D = UsingAlias->getTargetDecl();
> 6. For the case of global or member functions with the same name as  
> the using declaration, in the semantic action functions dealing with  
> same-named functions, don't match a using declaration.  I'm hoping  
> it doesn't matter if the using decl hides a previous declaration of  
> the same name.
>
> Question:  Is the UsingAliasDecl class the right way to go, and is  
> the name okay?

Yes, I think it's the right way to go, and the name is fine.

> Question:  When encountering a name corresponding to a using  
> declaration in an expression, is just substituting the target  
> declaration okay? I'm worried that losing the semantic information  
> about an identifier in a statement or expression corresponding to a  
> using declaration might be an issue.  But just substituting the  
> target decl makes for simpler code so far.

I think this is the right thing to do, but I suspect that we should do  
this substitution of the target declaration much sooner, e.g., as part  
of name lookup (in SemaLookup.cpp) when we are not looking for a  
redeclaration.

> I'm writing a cxx-using-declaration.cpp test file, which will later  
> have examples from the C++ standard draft or comparible ones.  I've  
> enclosed the simple non-error case version I've used so far.  While  
> I'm waiting for feedback, I'll work on adding more test cases, and  
> getting the 'using typename' to work.

Great! Some more detailed review follows.

Index: tools/clang/include/clang/AST/DeclCXX.h
===================================================================
--- tools/clang/include/clang/AST/DeclCXX.h	(revision 68578)
+++ tools/clang/include/clang/AST/DeclCXX.h	(working copy)

@@ -1003,6 +1003,37 @@

    }
    static bool classof(const NamespaceAliasDecl *D) { return true; }
  };
+
+/// UsingAliasDecl - Represents C++ using-directive. For example:
+///    using someNameSpace::someIdentifier;

UsingAliasDecl represents a using declaration, not a using directive.

+class UsingAliasDecl : public NamedDecl {
+
+  // Target declaration.
+  NamedDecl* TargetDecl;
+  // Had 'typename' keyword.
+  bool IsTypeName;
+

We'll probably want to store more location information, e.g., the  
source range covering the nested-name-specifier, (e.g., "N::M::"), the  
source location of the target declaration's name, and the source  
location of the "using" itself. Also, how about keeping track of the  
NestedNameSpecifier used to refer to the target declaration? It's good  
for pretty-printing, and will also be needed when the using directive  
occurs within a template and the nested-name-specifier is dependent.

+  // Parse namespace-name.
+  if (SS.isInvalid() || Tok.isNot(tok::identifier)) {
+    Diag(Tok, diag::err_expected_namespace_name);
+    // If there was invalid namespace name, skip to end of decl, and  
eat ';'.
+    SkipUntil(tok::semi);
+    return DeclPtrTy();
+  }

err_expected_namespace_name isn't the right diagnostic, because the  
nested-name-specifier can refer to a namespace or a class (or, in C+ 
+0x, an enumeration or concept map).

Index: tools/clang/lib/Sema/SemaDecl.cpp

===================================================================

--- tools/clang/lib/Sema/SemaDecl.cpp	(revision 68578)

+++ tools/clang/lib/Sema/SemaDecl.cpp	(working copy)

@@ -1322,6 +1322,7 @@

      DC = computeDeclContext(D.getCXXScopeSpec());
      // FIXME: RequireCompleteDeclContext(D.getCXXScopeSpec()); ?
      PrevDecl = LookupQualifiedName(DC, Name, LookupOrdinaryName,  
true);
+    // If the declaration is a using alias, use the target instead.

      // C++ 7.3.1.2p2:
      // Members (including explicit specializations of templates) of  
a named
@@ -2118,7 +2119,7 @@

        Diag(NewFD->getLocation(), diag::err_out_of_line_declaration)
          << D.getCXXScopeSpec().getRange();
        InvalidDecl = true;
-    } else if (!Redeclaration) {
+    } else if (!Redeclaration && (!PrevDecl || ! 
isa<UsingAliasDecl>(PrevDecl))) {
        // The user tried to provide an out-of-line definition for a
        // function that is a member of a class or namespace, but there
        // was no such member function declared (C++ [class.mfct]p2,
@@ -2258,7 +2259,8 @@


      if (PrevDecl &&
          (!AllowOverloadingOfFunction(PrevDecl, Context) ||
-         !IsOverload(NewFD, PrevDecl, MatchedDecl))) {
+           !IsOverload(NewFD, PrevDecl, MatchedDecl)) &&
+         !isa<UsingAliasDecl>(PrevDecl)) {

I think UsingAliasDecls are going to need more specific rules, here,  
to cope with overloaded functions. There are a few tricky cases to  
deal with, e.g., using declarations that occur before or after  
function declarations with the same name:

namespace N1 {
   int& f0(int);
   int& f1(int);
   float& f1(float);
   int& f2(int);
   int& f3(int);
   float& f3(float);
}

using N1::f0; // brings in one function N1::f0
float& f0(float); // overloads N1::f0

using N1::f1; // brings in two functions N1::f1
double& f1(double); // overloads with the two functions N1::f1

float& f2(float);
using N1::f2;  //  overloads N1::f2 with ::f2

using N1::f3; // brings in two functions N1::f3
float& f3(float); // error: conflicts with the using-declaration above

+  // Lookup target name.
+  LookupResult R = LookupParsedName(S, &SS, TargetName,
+                                    LookupOrdinaryName, false);
+
+  if (NamedDecl *NS = R) {
+    if (IsTypeName && !isa<TypeDecl>(NS)) {
+      Diag(IdentLoc, diag::err_using_typename_non_type);
+      return DeclPtrTy();
+    }

In this case, it's pretty harmless (from the AST perspective) if  
"typename" was given but the using directive refers to a non-type.  
Once we've emitted the error, why not just set IsTypeName=false and  
continue building the using declaration?

Index: tools/clang/lib/Sema/SemaExpr.cpp
===================================================================
--- tools/clang/lib/Sema/SemaExpr.cpp	(revision 68578)
+++ tools/clang/lib/Sema/SemaExpr.cpp	(working copy)
@@ -631,6 +631,10 @@

    } else
      D = Lookup.getAsDecl();

+  // Handle using alias.  Get the target declaration.
+  if (UsingAliasDecl *UsingAlias = dyn_cast<UsingAliasDecl>(D))
+    D = UsingAlias->getTargetDecl();
+

Here's where we see through a using declaration, but it won't always  
work. D could be an OverloadedFunctionDecl, where some of the  
overloaded functions were found via using declarations and others were  
declared in scope. This is why I think that using declarations need to  
be resolved as part of name lookup, so that the other parts of  
semantic analysis (expressions, member expression references, etc.)  
don't ever see the UsingAliasDecl directly... they just see the  
entities it refers to.

	- Doug