[PATCH] D53847: [C++2a] P0634r3: Down with typename!

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Thank you, this is looking really good.



================
Comment at: include/clang/Parse/Parser.h:2054
+    case DeclSpecContext::DSC_template_param:
+    case DeclSpecContext::DSC_template_type_arg:
+    case DeclSpecContext::DSC_normal:
----------------
Rakete1111 wrote:
> rsmith wrote:
> > This doesn't seem right to me. Doesn't this mean that `X<T::U>` will change from being a non-type template argument to being a type template argument?
> > 
> > Maybe that's a bug in the wording paper, though, since that context *is* a /type-id/.
> AFAIK no it won't, but I can't really tell if that's the right behavior or not. This patch doesn't touch the `X<T::U>` case.
I don't think I agree. `ParseTemplateArgument`, after disambiguation, parses a type name in `TemplateArgContext`, which results in a `DeclSpecContext` of `DSC_template_type_arg`, which means that we treat the context as an implicit typename context.

It looks to me like that will cause us to accept implicit `typename` in cases that we can disambiguate as being type template arguments, which would be wrong. For example, it looks like `X<const T::U>` would be incorrectly accepted: `typename` is required here because this type-id is not an implicit typename context.

Perhaps the right way to fix this would be to change `Parser::getDeclSpecContextFromDeclaratorContext` to map `TemplateArgContext` to a different kind of `DeclSpecContext` than `TemplateTypeArgContext` so that we can tell the difference here.


================
Comment at: lib/Parse/ParseDecl.cpp:6433
 
-    ParseDeclarationSpecifiers(DS);
+    bool AllowImplicitTypename;
+    if (D.getContext() == DeclaratorContext::MemberContext ||
----------------
We should determine this up-front rather than computing it once per parameter that we parse.


================
Comment at: lib/Parse/ParseDecl.cpp:6439
+      AllowImplicitTypename =
+          D.getCXXScopeSpec().isSet() && Actions.isDeclaratorFunctionLike(D);
 
----------------
I don't think you need to do name lookup here. I think you can use `D.isFunctionDeclaratorAFunctionDeclaration()` instead of `isDeclaratorFunctionLike(D)` -- because we've already committed to parsing this as a function declaration in that case, a qualified function name will either redeclare something from its context (in which case implicit typename is permitted) or be ill-formed.


================
Comment at: lib/Parse/Parser.cpp:1518
+    if (TryAnnotateTypeOrScopeTokenAfterScopeSpec(
+            SS, !WasScopeAnnotation, /*AllowImplicitTypename=*/true))
       return ANK_Error;
----------------
What justifies allowing implicit typename here? Don't we need the caller to tell us if that's OK?

... actually, perhaps this tentatively-declared identifiers logic should only be performed if `SS.isEmpty()` (because a tentatively-declared identifier can never be found within a scope named by a nested-name-specifier), at which point the value of this flag doesn't matter.


================
Comment at: lib/Parse/Parser.cpp:1783
+            AllowImplicitTypename &&
+                getCurScope()->isFunctionPrototypeScope())) {
       SourceLocation BeginLoc = Tok.getLocation();
----------------
Do we need this scope check? (It would seem preferable to trust the caller to have passed in the right flag value, if we can.)


================
Comment at: lib/Sema/Sema.cpp:2006-2019
+bool Sema::isDeclaratorFunctionLike(const Declarator &D) {
+  assert(D.getCXXScopeSpec().isSet() &&
+         "can only be called for qualified names");
+  LookupResult LR(*this, D.getIdentifier(), D.getBeginLoc(), LookupOrdinaryName,
+                  ForVisibleRedeclaration);
+  DeclContext *DC = computeDeclContext(D.getCXXScopeSpec());
+  if (!DC)
----------------
Some thoughts on this:

 * Can this be unified with the lookup code in `HandleDeclarator`? This is really the same lookup, repeated in two places.
 * It'd be nice to cache this lookup, rather than performing it three times (once when disambiguating a parenthesized initializer from a function declaration, once when we're about to parse a parameter-declaration-clause, and once in `HandleDeclarator` after parsing completes -- though at least that's reduced to two lookups if you make the change I suggested in `ParseParameterDeclarationClause`)
 * If we don't do the caching, what happens if lookup fails due to ambiguity? Do we get the same error multiple times (once for each time we perform the lookup)?


================
Comment at: lib/Sema/Sema.cpp:2010
+  LookupResult LR(*this, D.getIdentifier(), D.getBeginLoc(), LookupOrdinaryName,
+                  ForVisibleRedeclaration);
+  DeclContext *DC = computeDeclContext(D.getCXXScopeSpec());
----------------
Should this be `forRedeclarationInCurContext()`?


================
Comment at: lib/Sema/Sema.cpp:2011
+                  ForVisibleRedeclaration);
+  DeclContext *DC = computeDeclContext(D.getCXXScopeSpec());
+  if (!DC)
----------------
`EnteringContext` should presumably be `true` here, so that we can look into class templates.


================
Comment at: lib/Sema/Sema.cpp:2016
+  LookupQualifiedName(LR, DC);
+  return std::all_of(LR.begin(), LR.end(), [](Decl *Dcl) {
+    return isa<FunctionDecl>(Dcl) || isa<FunctionTemplateDecl>(Dcl);
----------------
`UsingDecl`s in this set should not disqualify us from the "function-like" interpretation.


================
Comment at: lib/Sema/Sema.cpp:2017
+  return std::all_of(LR.begin(), LR.end(), [](Decl *Dcl) {
+    return isa<FunctionDecl>(Dcl) || isa<FunctionTemplateDecl>(Dcl);
+  });
----------------
I think you need to `getUnderlyingDecl` of `Dcl` here; lookup might find `UsingShadowDecl`s that you need to look through.


================
Comment at: test/SemaCXX/unknown-type-name.cpp:121-122
 template<typename T>
-A<T>::g() { } // expected-error{{requires a type specifier}}
+A<T>::g() { } // expected-error{{expected unqualified-id}}
+// expected-warning at -1{{implicit 'typename' is a C++2a extension}}
----------------
Rakete1111 wrote:
> rsmith wrote:
> > rsmith wrote:
> > > This is a diagnostic quality regression. Perhaps that's an inevitable consequence of P0634, but we should at least try to do better.
> > This is marked "done" but doesn't seem to be done?
> Oops, don't know why I did that. I can't figure out a way to fix this. I can implement a basic heuristic to detect some very basic cases like this one, but I don't think it's worth it.
OK, fair enough. This one does seem hard to diagnose well.


Repository:
  rC Clang

https://reviews.llvm.org/D53847