Page MenuHomePhabricator

[C++2a] P0634r3: Down with typename!
Needs ReviewPublic

Authored by Rakete1111 on Oct 29 2018, 4:16 PM.

Details

Reviewers
rsmith
Summary

This patch implements P0634r3 that removes the need for 'typename' in certain contexts.

For example,

template <typename T>
using foo = T::type; // ok

This is also allowed in previous language versions as an extension, because I think it's pretty useful. :)

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
rsmith added inline comments.Nov 1 2018, 5:07 PM
lib/Sema/SemaDecl.cpp
317–320

I'm very unhappy with this approach. The proposal is careful to make sure that we can always know whether we're in a context where typename is optional in advance, and never need to guess. We should propagate that knowledge into all the places that parse types where typename is implicit.

test/CXX/temp/temp.res/p5.cpp
135–141

This error doesn't make much sense. We should instead be rejecting due to the lookup for NN::f being ambiguous (between a function and a variable).

test/SemaCXX/MicrosoftCompatibility.cpp
218

Looks like you broke an MS compatibility feature here. We should continue to accept this under -fms-compatibility.

test/SemaCXX/unknown-type-name.cpp
121–122

This is marked "done" but doesn't seem to be done?

Rakete1111 updated this revision to Diff 173056.Nov 7 2018, 3:29 PM

Addressed review comments! :)

Rakete1111 marked 9 inline comments as done.Nov 7 2018, 3:38 PM

I also found another diagnostic regression:

template <typename T>
T::type();

The previous error message was something with "nested name specifier does not refer to class ...". Now, the T::type part is interpreted as type and so we get a "expected unqualified-id" error message.

include/clang/Parse/Parser.h
2054

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.

2117

This overload is unnecessary; I accidentally included it. I'll change it later so that Phab doesn't lose the review comments.

test/SemaCXX/unknown-type-name.cpp
121–122

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.

Thank you, this is looking really good.

include/clang/Parse/Parser.h
2054

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.

lib/Parse/ParseDecl.cpp
6427

We should determine this up-front rather than computing it once per parameter that we parse.

6433

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.

lib/Parse/Parser.cpp
1517

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.

1779

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.)

lib/Sema/Sema.cpp
2006–2019 ↗(On Diff #173056)

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)?
2010 ↗(On Diff #173056)

Should this be forRedeclarationInCurContext()?

2011 ↗(On Diff #173056)

EnteringContext should presumably be true here, so that we can look into class templates.

2016 ↗(On Diff #173056)

UsingDecls in this set should not disqualify us from the "function-like" interpretation.

2017 ↗(On Diff #173056)

I think you need to getUnderlyingDecl of Dcl here; lookup might find UsingShadowDecls that you need to look through.

test/SemaCXX/unknown-type-name.cpp
121–122

OK, fair enough. This one does seem hard to diagnose well.

Rakete1111 marked 11 inline comments as done.

Addressed review comments :)

Rakete1111 added inline comments.Nov 14 2018, 10:39 PM
lib/Sema/Sema.cpp
2006–2019 ↗(On Diff #173056)

I don't think so, because HandleDeclarator is called after having already parsed the function declaration. I cached it now, but the cleanest solution that I could think of is to use new. Is this appropriate? Or do you have an alternative suggestion?

Rakete1111 added inline comments.Nov 14 2018, 10:41 PM
include/clang/Parse/Parser.h
2117

Actually, it's less repetitive if I leave it. So I'll just leave it be.

Rebase and friendly ping! :)

Rebase + friendly ping :)

@rsmith do you have any more comments?

ping/rebase.

Fix a bug where T::value inside a functional cast in a template argument would be interpreted as a function type.

Also added the missing test file that got lost in a previous revision for some reason. :)

Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2019, 10:19 AM
Herald added a subscriber: jdoerfert. · View Herald Transcript

rsmith:

Some thoughts on this:

  • Can this be unified with the lookup code in HandleDeclarator? This is really the same lookup, repeated in two places.

What I wrote:

I don't think so, because HandleDeclarator is called after having already parsed the function declaration. I cached it now, but the cleanest solution that I could think of is to use new. Is this appropriate? Or do you have an alternative suggestion?

Maybe a llvm::Optional is more appropriate?

  • 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)

Hmm let me see what I can do about that.

  • 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)?

Don't relookup the qualified-id and use the cached one instead if possible.

Rakete1111 updated this revision to Diff 198139.May 4 2019, 7:27 AM

Don't leak memory and friendly ping :)

rsmith added inline comments.May 4 2019, 6:24 PM
clang/include/clang/Sema/Sema.h
1784 ↗(On Diff #198139)

It's dangerous to add a bool parameter like this, in a long list of bool parameters with default arguments. Please add an enum for AllowImplicitTypename and use it instead of the boolean flag.

clang/lib/Parse/ParseDecl.cpp
2652–2654 ↗(On Diff #198139)

Can you split out this error recovery improvement and commit it separately before the rest of this work? It doesn't appear to have any dependency on the rest of the change.

4859 ↗(On Diff #198139)

This seems surprising to me; I'd expect to have an implicit typename here approximately when not DisambiguatingWithExpression. Also basing this off the scope seems wrong, as we can switch into and out of implicit typename contexts multiple times within a scope. Eg, in template<typename T, T::template U<T::V>> we get an implicit typename for T::template U but not for T::V despite them being in the same scope.

Should the callers of this function be passing in an "implicit typename" flag?

5882 ↗(On Diff #198139)

A citation of the relevant wording here would be useful.

6444 ↗(On Diff #198139)

Likewise a citation of the relevant rule here would be useful.

clang/lib/Parse/ParseTentative.cpp
1280 ↗(On Diff #198139)

This seems unclear to me.

I think we should be passing AllowImplicitTypename directly to TryAnnotate..., rather than taking HasMissingTypename into account -- AllowImplicitTypename reflects the language rules, whereas HasMissingTypename is just an error recovery tool, so HasMissingTypename should not have any influence on how we annotate the token stream unless we actually detect an error.

1342 ↗(On Diff #198139)

These recursive steps need to pass in the AllowImplicitTypename flag. Maybe the default argument should be removed for safety.

clang/lib/Parse/Parser.cpp
1490 ↗(On Diff #198139)

Have you checked that this is never called in an implicit typename context?

Please update the documentation for this to say that it can never be called in such a context, or add an AllowImplicitTypename parameter as necessary.

clang/lib/Sema/SemaTemplate.cpp
9503 ↗(On Diff #198139)

Please parenthesize the left-hand-side of this ternary operator.

clang/test/CXX/drs/dr1xx.cpp
61 ↗(On Diff #198139)

For each of these test/CXX/drs tests that you change, please add a C++20 RUN: line and update the diagnostic expectations to match in pre- and post-C++20 mode as applicable.

Rakete1111 marked 10 inline comments as done.
  • Addressed review comments
Rakete1111 marked an inline comment as done.May 21 2019, 1:31 AM
Rakete1111 marked an inline comment as done.May 21 2019, 1:33 AM
Rakete1111 added inline comments.
clang/lib/Parse/ParseDecl.cpp
4859 ↗(On Diff #198139)

Seems like template <typename T, T::template U<T::V>> isn't accepted as a context with an implicit typename (for the T::template). I'll look into it :)

Add support for implicit typenames of the form T::template U</*...*>

rsmith added inline comments.May 21 2019, 5:24 PM
clang/include/clang/Sema/DeclSpec.h
1753–1758 ↗(On Diff #200440)

Consider using an enum class here.

clang/lib/Parse/ParseDecl.cpp
2652–2654 ↗(On Diff #198139)

Looks like you need to rebase; the change was committed but is still in the latest version of this patch.

4321 ↗(On Diff #200440)

It seems like a wording oversight that we don't assume typename in an enum-base. Probably would be good to raise this on the core reflector.

4322 ↗(On Diff #200440)

Using a different ITC value for non-C++ compilations seems surprising. (It should never make any difference outside of C++, but leaves the reader wondering why the two are different.) Can we use ITC_Never here for consistency?

5100–5101 ↗(On Diff #200440)

Oh, this could be a problem.

If this *is* a constructor declaration, then this is implicit typename context: this is either a "parameter-declaration in a member-declaration" ([temp.res]/5.2.3) or a "parameter-declaration in a declarator of a function or function template declaration whose declarator-id is qualified". But if it's *not* a constructor declaration, then this is either the declarator-id of a declaration or the nested-name-specifier of a pointer-to-member declarator:

template<typename T>
struct C {
  C(T::type); // implicit typename context
  friend C (T::fn)(); // not implicit typename context, declarator-id of friend declaration
  C(T::type::*x)[3]; // not implicit typename context, pointer-to-member type
};

I think we need to use ITC_Yes here, in order to correctly disambiguate the first example above. Please add tests for the other two cases to make sure this doesn't break them -- but I'm worried this will break the second case, because it will incorrectly annotate T::fn as a type.

clang/lib/Sema/Sema.cpp
2219 ↗(On Diff #200440)

Consider moving the make_unique earlier (directly before LookupQualifiedName) to avoid needing to move the LookupResult object into different storage here.

clang/lib/Sema/SemaTemplate.cpp
3369 ↗(On Diff #200440)

Are there any cases where we would call this for which C++20 still requires a typename keyword? Should this function be passed an ImplicitTypenameContext?

3377–3379 ↗(On Diff #200440)

This FIXME is concerning. Is this a problem with this patch? (Is the FIXME wrong now?)

clang/test/CXX/drs/dr5xx.cpp
485 ↗(On Diff #200440)

A comment here explaining that A and B stop being aggregates in C++20 would be nice. (Nicer would be changing the testcase so it still tests the relevant rule in C++20 mode, if that's possible...)

clang/test/CXX/temp/temp.res/p5.cpp
1 ↗(On Diff #200440)

Please add tests for enum-base and conversion-type-id:

template<typename T> struct A {
  enum E : T::type {};
  operator T::type() {}
  void f() { this->operator T::type(); }
};

... which should currently be rejected.

Rakete1111 marked 11 inline comments as done.
  • rebased
  • addressed review comments
clang/lib/Parse/ParseDecl.cpp
4321 ↗(On Diff #200440)

Do you know why this isn't allowed in operator ids?

5100–5101 ↗(On Diff #200440)

Yeah it does the break the second. Would just checking whether a friend is there be good enough? clang doesn't seem to actively propose to add a friend if there's one missing, so if we add a IsFriend parameter and then check if it is set than always return false, it should work. Is that acceptable? It would break the nice error message you get if you write friend C(int);, but if we restrict it when isDeclarationSpecifier return false (with Never) would that be better (that's what I'm doing right now)?

Another solution would be to tentatively parse the whole thing, but that can be pretty expensive I believe.

clang/lib/Sema/SemaTemplate.cpp
3377–3379 ↗(On Diff #200440)

Yes you're right, I believe it not longer applies due to the change in ParseDecl.cpp in ParseDeclarationSpecifiers.

Rakete1111 marked an inline comment as done.May 22 2019, 8:54 AM
Rakete1111 added inline comments.
clang/lib/Sema/SemaTemplate.cpp
3377–3379 ↗(On Diff #200440)

Although I'm not that sure I fully understand what that comments alludes to.

rsmith marked 2 inline comments as done.May 22 2019, 2:52 PM
rsmith added inline comments.
clang/lib/Parse/ParseDecl.cpp
4321 ↗(On Diff #200440)

There's already been discussion of that on the core reflector, and people seem to agree it's an oversight in the wording. If you want to accept that here, I think that's fine, under the assumption that this will be fixed by DR. (If you want to follow the wording-as-moved, that's fine too.)

5100–5101 ↗(On Diff #200440)

This seems a little fragile against future grammar changes, but I think the IsFriend check is correct -- I *think* the only way we can see a qualified-id here in a valid non-constructor, non-nested-name-specifier case is in a friend declaration.

clang/lib/Sema/SemaTemplate.cpp
3377–3379 ↗(On Diff #200440)

The example would be something like this:

template<typename T> struct X {
  template<typename U> struct Y { Y(); using V = int; };
  template<typename U> typename Y<U>::V f();
};
template<typename T> template<typename U>
X<T>::Y<U>::V X<T>::f() {}

Clang trunk today points out that the typename keyword is missing on the final line, but fails to point out that the template keyword is also missing. The reason is that in the case where that construct is valid:

template<typename T> template<typename U>
X<T>::Y<U>::Y() {}

... we are "entering the context" of the nested-name-specifier, which means we don't need a template keyword.

If the FIXME is fixed, then we should diagnose the missing template in the above program.

Also, because we don't rebuild the nested-name-specifier as a dependent nested name specifier in this case, we fail to match it against the declaration in the class, so in my example above, we also produce an "out-of-line definition does not match" error.

A closely-related issue can be seen in an example such as:

template<typename T> struct X {
  template<typename U> struct Y { Y(); typedef void V; }; 
  template<typename U> typename Y<U>::V::W f();
};
template<typename T> template<typename U>
X<T>::template Y<U>::V::W X<T>::f() { return 0; }

Here, we produce a completely bogus error:

<stdin>:6:13: error: 'X::Y::V' (aka 'void') is not a class, namespace, or enumeration
X<T>::template Y<U>::V::W X<T>::f() { return 0; } 
            ^

... because we parse this in "entering context" mode and so resolve X<T>::Y<U>::V to the type in the primary template (that is, void). That's wrong: we should defer resolving this name to a type until we know what T and U are, or until we know that we're *actually* entering the context. Specifically, the above program can be extended as follows:

template<> template<> struct X<int>::Y<int> {
  struct V { using W = int; };
};
void call(X<int> x) { x.f<int>(); } // OK, f returns int

The above program should be accepted by this patch, if the FIXME is indeed now fixed. Please add it as a testcase :)

Rakete1111 marked an inline comment as done.May 27 2019, 1:50 AM
Rakete1111 added inline comments.
clang/lib/Sema/SemaTemplate.cpp
3377–3379 ↗(On Diff #200440)

Oh ok, got it thanks. So no, the program is still not accepted in clang, and a pretty dumb side effect of it is that

template<typename T> struct X {
  template<typename U> struct Y { Y(); using V = int; };
  template<typename U> typename Y<U>::V f();
};
template<typename T> template<typename U>
X<T>::Y<U>::V X<T>::f() {}

is accepted without the diagnostic for the missing template. :(

Minor suggestion, you may want to update http://clang.llvm.org/cxx_status.html page for "typename optional in more contexts" with in this change.