[PR,c++/84789] do not resolve typename into template-independent

Message ID orpo43vgzj.fsf@lxoliva.fsfla.org
State New
Headers show
Series
  • [PR,c++/84789] do not resolve typename into template-independent
Related show

Commit Message

Alexandre Oliva March 16, 2018, 9:38 p.m.
resolve_typename_type may peek into template types that might still be
specialized.  In some cases, e.g. g++.dg/template/friend48.C or
g++.dg/template/decl2.C, that is exactly the right thing to do.  In
others, like the newly-added testcase g++.dg/template/pr84789.C, it
isn't, and if the qualifying scope happens to resolve to a non-template
type, we resolve to that and then fail the assert that checks we still
have a template-dependent scope.

It appears to me that, in cases in which the assert would fail, we are
missing the typename keyword, and we ought to report an error; if we
just return the incoming type unchanged, that's exactly what we get.
So, I'm turning such failed asserts into early returns, so that the
parser can recover and report an error.

Regstrapped on i686- and x86_64-linux-gnu.  Ok to install?


for  gcc/cp/ChangeLog

	PR c++/84789
	* pt.c (resolve_typename_type): Keep the type
	template-dependent.

for  gcc/testsuite/ChangeLog

	PR c++/84789
	* g++.dg/template/pr84789.C: New.
---
 gcc/cp/pt.c                             |   19 +++++++++++++++++--
 gcc/testsuite/g++.dg/template/pr84789.C |   13 +++++++++++++
 2 files changed, 30 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/template/pr84789.C



-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Comments

Jason Merrill March 20, 2018, 8:26 p.m. | #1
On Fri, Mar 16, 2018 at 5:38 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
> resolve_typename_type may peek into template types that might still be

> specialized.  In some cases, e.g. g++.dg/template/friend48.C or

> g++.dg/template/decl2.C, that is exactly the right thing to do.  In

> others, like the newly-added testcase g++.dg/template/pr84789.C, it

> isn't, and if the qualifying scope happens to resolve to a non-template

> type, we resolve to that and then fail the assert that checks we still

> have a template-dependent scope.


We're looking inside them because we're trying to parse a declarator;
the tentative parse will fail in this case, because we aren't in a
declarator, but that doesn't mean it's wrong to peek.

Though I'm not sure why cp_parser_parse_and_diagnose_invalid_type_name
is passing true for declarator_p to cp_parser_id_expression.  I'm
going to try changing that to false and see if anything breaks.

> It appears to me that, in cases in which the assert would fail, we are

> missing the typename keyword, and we ought to report an error; if we

> just return the incoming type unchanged, that's exactly what we get.

> So, I'm turning such failed asserts into early returns, so that the

> parser can recover and report an error.


I disagree; it seems to me that the assert should allow the case where
the scope was originally dependent, but got resolved earlier in the
function.

Jason
Alexandre Oliva March 20, 2018, 10:07 p.m. | #2
On Mar 20, 2018, Jason Merrill <jason@redhat.com> wrote:

> On Fri, Mar 16, 2018 at 5:38 PM, Alexandre Oliva <aoliva@redhat.com> wrote:

>> resolve_typename_type may peek into template types that might still be

>> specialized.  In some cases, e.g. g++.dg/template/friend48.C or

>> g++.dg/template/decl2.C, that is exactly the right thing to do.  In

>> others, like the newly-added testcase g++.dg/template/pr84789.C, it

>> isn't, and if the qualifying scope happens to resolve to a non-template

>> type, we resolve to that and then fail the assert that checks we still

>> have a template-dependent scope.


> We're looking inside them because we're trying to parse a declarator;

> the tentative parse will fail in this case, because we aren't in a

> declarator, but that doesn't mean it's wrong to peek.


Huh?  We're referencing members of an unrelated template that AFAIK
needs not even be defined at that point, and even if it is, it could
still be specialized afterwards.  How can it possibly be right to
short-circuit such nested names?  How would template
instantiation/specialization get a chance to do the proper mapping?

template<typename> struct B : A {}; // would /*: A {}*/ make any diff?
template<typename T> struct C : B<T> // would /* : B<T>*/ make any diff?
{
  B<T>::A::I::I i; // { dg-error "typename" }
};

Is it by any chance the fact that B<T> is a base class for C<T> that
makes it correct to peek into it?  I don't recall any exception of this
sort.

> I disagree; it seems to me that the assert should allow the case where

> the scope was originally dependent, but got resolved earlier in the

> function.


Doesn't the function always take dependent scopes?  I for some reason
thought that was the case.

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
Jason Merrill March 21, 2018, 1:36 a.m. | #3
On Tue, Mar 20, 2018 at 6:07 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Mar 20, 2018, Jason Merrill <jason@redhat.com> wrote:

>

>> On Fri, Mar 16, 2018 at 5:38 PM, Alexandre Oliva <aoliva@redhat.com> wrote:

>>> resolve_typename_type may peek into template types that might still be

>>> specialized.  In some cases, e.g. g++.dg/template/friend48.C or

>>> g++.dg/template/decl2.C, that is exactly the right thing to do.  In

>>> others, like the newly-added testcase g++.dg/template/pr84789.C, it

>>> isn't, and if the qualifying scope happens to resolve to a non-template

>>> type, we resolve to that and then fail the assert that checks we still

>>> have a template-dependent scope.

>

>> We're looking inside them because we're trying to parse a declarator;

>> the tentative parse will fail in this case, because we aren't in a

>> declarator, but that doesn't mean it's wrong to peek.

>

> Huh?  We're referencing members of an unrelated template that AFAIK

> needs not even be defined at that point, and even if it is, it could

> still be specialized afterwards.  How can it possibly be right to

> short-circuit such nested names?  How would template

> instantiation/specialization get a chance to do the proper mapping?

>

> template<typename> struct B : A {}; // would /*: A {}*/ make any diff?

> template<typename T> struct C : B<T> // would /* : B<T>*/ make any diff?

> {

>   B<T>::A::I::I i; // { dg-error "typename" }

> };


> Is it by any chance the fact that B<T> is a base class for C<T> that

> makes it correct to peek into it?  I don't recall any exception of this

> sort.


No, we look inside when we're trying to parse the qualified-id as the
name of a declaration; in a declarator-id we can look into
uninstantiated classes, otherwise there would be no way to define
members of class templates.

  void X<T>::N::f() { } // looks inside X<T>

>> I disagree; it seems to me that the assert should allow the case where

>> the scope was originally dependent, but got resolved earlier in the

>> function.

>

> Doesn't the function always take dependent scopes?  I for some reason

> thought that was the case.


Yes, as the comment says, a TYPENAME_TYPE should always have a
dependent scope.  In this case, the dependent scope was "typename
B<T>::A", but just above we resolved it to just A, making it no longer
dependent.

Jason
Alexandre Oliva March 21, 2018, 3:27 a.m. | #4
On Mar 20, 2018, Jason Merrill <jason@redhat.com> wrote:

> On Tue, Mar 20, 2018 at 6:07 PM, Alexandre Oliva <aoliva@redhat.com> wrote:

>> On Mar 20, 2018, Jason Merrill <jason@redhat.com> wrote:

>>> that doesn't mean it's wrong to peek.


>> Huh?  We're referencing members of an unrelated template that AFAIK

>> needs not even be defined at that point, and even if it is, it could

>> still be specialized afterwards.  How can it possibly be right to

>> short-circuit such nested names?


>> template<typename> struct B : A {}; // would /*: A {}*/ make any diff?

>> template<typename T> struct C : B<T> // would /* : B<T>*/ make any diff?

>> {

>>   B<T>::A::I::I i; // { dg-error "typename" }

>> };


> No, we look inside when we're trying to parse the qualified-id as the

> name of a declaration


Yeah, but that's not the case at hand.  I guess we're miscommunicating.
I understood you were saying it was ok to peek in this case.  Would you
please state, for clarity, what your stance is on peeking in this case,
specifically, in the B<T>::A::I::I within the definition of C above?

> in a declarator-id we can look into uninstantiated classes, otherwise

> there would be no way to define members of class templates.


>   void X<T>::N::f() { } // looks inside X<T>


Of course, but then, we wouldn't get to a template-independent member.
A member of a template-dependent name is still template-dependent.  It's
only when a qualified name maps to an external name that it might become
template-independent, and when this happens, I believe the qualified-id
is not one that can be used to define a member.  Specifically:

struct K { struct L { static double j; }; };
template <typename T> struct M { struct N { static int i; }; };
template <typename T> struct O { typedef M<T> P; typedef K Q; };
template <typename T> int O<T>::P::N::i = 42;
template <typename T> double O<T>::Q::L::j = 42.0;

if we remap O<T>::P to M<T> and O<T>::Q to K, how will we realize the
given type-ids are not appropriate?  Where will the template parmlist
belong when the qualified-id is taken as equivalent to K::L::j?

FWIW, we silently swallow the definition of i above (I think we
shouldn't), and we fail the same assert in resolve_typename_type when
parsing the definition of j.

So, you see, we do have problems with declarators, and we have problems
with types.  AFAICT, we shouldn't simplify qualified-ids in declarators,
otherwise we'd fail to issue errors for the definitions above, and we
shouldn't simplify qualified-ids in types involving templates other than
the one we'd in.

So it's not clear to me what purpose the resolver is supposed to serve,
considering that simplifying the types seem to almost never be wanted.
But one issue is whether we can peek into dependent types, another is
whether we can and should simplify them.  The answers to these questions
don't seem to be very clear to me.  What is clear to me is that
refraining from simplifying from dependent to non-dependent enables us
to report errors that we were missing in both declarators and types, and
that simplifying from dependent to unrelated dependent prevents us from
reporting the error we should in the definition of i above.  But is
there any case in which *not* simplifying causes problems?

>>> I disagree; it seems to me that the assert should allow the case where

>>> the scope was originally dependent, but got resolved earlier in the

>>> function.

>> 

>> Doesn't the function always take dependent scopes?  I for some reason

>> thought that was the case.


> Yes, as the comment says, a TYPENAME_TYPE should always have a

> dependent scope.  In this case, the dependent scope was "typename

> B<T>::A", but just above we resolved it to just A, making it no longer

> dependent.


Then you got me thoroughly confused: what did you mean by 'the assert
should allow' a case that's a given?

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
Jason Merrill March 21, 2018, 3:48 p.m. | #5
On Tue, Mar 20, 2018 at 11:27 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Mar 20, 2018, Jason Merrill <jason@redhat.com> wrote:

>

>> On Tue, Mar 20, 2018 at 6:07 PM, Alexandre Oliva <aoliva@redhat.com> wrote:

>>> On Mar 20, 2018, Jason Merrill <jason@redhat.com> wrote:

>>>> that doesn't mean it's wrong to peek.

>

>>> Huh?  We're referencing members of an unrelated template that AFAIK

>>> needs not even be defined at that point, and even if it is, it could

>>> still be specialized afterwards.  How can it possibly be right to

>>> short-circuit such nested names?

>

>>> template<typename> struct B : A {}; // would /*: A {}*/ make any diff?

>>> template<typename T> struct C : B<T> // would /* : B<T>*/ make any diff?

>>> {

>>>   B<T>::A::I::I i; // { dg-error "typename" }

>>> };

>

>> No, we look inside when we're trying to parse the qualified-id as the

>> name of a declaration

>

> Yeah, but that's not the case at hand.  I guess we're miscommunicating.

> I understood you were saying it was ok to peek in this case.  Would you

> please state, for clarity, what your stance is on peeking in this case,

> specifically, in the B<T>::A::I::I within the definition of C above?


It's OK when we're tentatively trying to parse it as a declarator, not
when we're trying to parse it as a type-specifier.

We seem to be talking past each other on this point: We already don't
peek when parsing a type-specifier, we only call resolve_typename_type
with false for only_current_p in the context of trying to parse a
declarator.  The parser tries both interpretations for B<T>::A::I::I.
The type interpretation is ill-formed because of missing 'typename',
so it tries again as a declarator, and therefore tries to resolve the
TYPENAME_TYPE that it built while trying for a type, and trips the
assert.

>> in a declarator-id we can look into uninstantiated classes, otherwise

>> there would be no way to define members of class templates.

>

>>   void X<T>::N::f() { } // looks inside X<T>

>

> Of course, but then, we wouldn't get to a template-independent member.

> A member of a template-dependent name is still template-dependent.  It's

> only when a qualified name maps to an external name that it might become

> template-independent, and when this happens, I believe the qualified-id

> is not one that can be used to define a member. Specifically:

>

> struct K { struct L { static double j; }; };

> template <typename T> struct M { struct N { static int i; }; };

> template <typename T> struct O { typedef M<T> P; typedef K Q; };

> template <typename T> int O<T>::P::N::i = 42;

> template <typename T> double O<T>::Q::L::j = 42.0;

>

> if we remap O<T>::P to M<T> and O<T>::Q to K, how will we realize the

> given type-ids are not appropriate?  Where will the template parmlist

> belong when the qualified-id is taken as equivalent to K::L::j?


Agreed, these look bizarre because the template parm is used in O<T>,
which is not an enclosing class of i or j.  And j is clearly
ill-formed because it declares a non-template as a template.

But it's not clear to me that i is ill-formed; we allow

struct A { static int i; };
struct B { typedef ::A A; };
int B::A::i = 0;

and the i example seems analogous.  I wouldn't argue against making it
ill-formed, but I don't see a rule that would make it so in the
current draft standard.  And even resolving the scope to be
non-dependent doesn't necessarily mean the declaration will be, if the
template parameter list ends up being for a member template:

struct K { struct L { template <typename T> static void f(T); }; };
template <typename T> struct O { typedef K Q; };
template <typename T> void O<T>::Q::L::f(T) { }

>>>> I disagree; it seems to me that the assert should allow the case where

>>>> the scope was originally dependent, but got resolved earlier in the

>>>> function.

>>>

>>> Doesn't the function always take dependent scopes?  I for some reason

>>> thought that was the case.

>

>> Yes, as the comment says, a TYPENAME_TYPE should always have a

>> dependent scope.  In this case, the dependent scope was "typename

>> B<T>::A", but just above we resolved it to just A, making it no longer

>> dependent.

>

> Then you got me thoroughly confused: what did you mean by 'the assert

> should allow' a case that's a given?


asserts are often used to verify that things we think of as givens are
actually true.  :)

I suppose we could remove the assert, but I'd probably move it up
above where we start messing with 'scope'.

Jason
Alexandre Oliva March 22, 2018, 1:20 a.m. | #6
On Mar 21, 2018, Jason Merrill <jason@redhat.com> wrote:

> On Tue, Mar 20, 2018 at 11:27 PM, Alexandre Oliva <aoliva@redhat.com> wrote:


>> I understood you were saying it was ok to peek in this case.  Would you

>> please state, for clarity, what your stance is on peeking in this case,

>> specifically, in the B<T>::A::I::I within the definition of C above?


> It's OK when we're tentatively trying to parse it as a declarator, not

> when we're trying to parse it as a type-specifier.


Thanks for the clarification; it had seemed to me that you were
rejecting my understanding that we shouldn't look up within the
unrelated template type when parsing the type id.  Now I understand you
were only rejecting the notion that we were parsing it as a type id.
Once I understood that, and realized why you were speaking of
declarators although the testcase at hand had a template-dependent type
id that was obviously (to me) not a declarator, it all made sense ;-)
Sorry for being so dense.


> I wouldn't argue against making it ill-formed, but I don't see a rule

> that would make it so in the current draft standard.


I have just spent some time looking for such a rule and failed to find
it too.  So I put in all of the bizarre declarators from this
conversation into new testcases, but expecting them to be accepted.

>>>>> I disagree; it seems to me that the assert should allow the case where

>>>>> the scope was originally dependent, but got resolved earlier in the

>>>>> function.


>>>> Doesn't the function always take dependent scopes?  I for some reason

>>>> thought that was the case.


>>> Yes, as the comment says, a TYPENAME_TYPE should always have a

>>> dependent scope.  In this case, the dependent scope was "typename

>>> B<T>::A", but just above we resolved it to just A, making it no longer

>>> dependent.


>> Then you got me thoroughly confused: what did you mean by 'the assert

>> should allow' a case that's a given?


> asserts are often used to verify that things we think of as givens are

> actually true.  :)


Yeah, but the wording 'allow' rather than 'require'/'verify' set me off.
We used to require the opposite of X, so allowing X amounted to assert
(!X || X), which didn't sound like something you'd mean.

> I suppose we could remove the assert, but I'd probably move it up

> above where we start messing with 'scope'.


Now that makes sense to me, and it works, but I was not happy with the
errors we got, so I went ahead and arranged for the type name to be
reparsed as a non-declarator for the diagnostic, so that we get a better
diagnostic.

Regstrapping, after a successful check-g++ of a non-bootstrapped
x86_64-linux-gnu build.  Ok to install if the regstrap passes?


[PR c++/84789] do not fail to resolve typename into template-independent

Although resolve_typename_type always takes a template-dependent
type-id, and it usually resolves it to another template-dependent
type-id, it is not correct to require the latter: in declarators,
template-dependent scopes may turn out to name template-independent
types, as in the pr84789-2.C and pr84789-3.C testcases.

The ill-formed testcase pr84789.C trips the same too-strict assert,
and also gets fixed by removing the assertion on the simplified scope.
However, whereas when the dependent type cannot be resolved, we get an
error that suggests 'typename' is missing:

pr84789.C:12:3: error: need ‘typename’ before ‘typename B<T>::A::I::I’
because ‘typename B<T>::A::I’ is a dependent scope
   B<T>::A::I::I i;
   ^~~~

when it can, we got errors that did not point at that possibility,
which may be confusing:

pr84789.C:9:15: error: ‘A::I’ {aka ‘int’} is not a class type
   B<T>::A::I::I i; // { dg-error "typename" }
               ^
pr84789.C:9:15: error: ‘I’ in ‘A::I’ {aka ‘int’} does not name a type

Changing the parser diagnostic code that reports an invalid type name
so that it does not attempt to reparse the name as a declarator gets
us the superior diagnostic of a missing 'typename' keyword.


for  gcc/cp/ChangeLog

	PR c++/84789
	* pt.c (resolve_typename_type): Drop assert that stopped
	simplification to template-independent types.  Add assert to
	verify the initial scope is template dependent.
	* parser.c (cp_parser_parse_and_diagnose_invalid_type_name):
	Reparse the id expression as a type-name, not a declarator.

for  gcc/testsuite/ChangeLog

	PR c++/84789
	* g++.dg/template/pr84789.C: New.
	* g++.dg/template/pr84789-2.C: New.
	* g++.dg/template/pr84789-3.C: New.
	* g++.dg/parse/dtor11.C: Accept alternate error message.
---
 gcc/cp/parser.c                           |    2 +-
 gcc/cp/pt.c                               |    5 +++--
 gcc/testsuite/g++.dg/parse/dtor11.C       |    2 +-
 gcc/testsuite/g++.dg/template/pr84789-2.C |   26 ++++++++++++++++++++++++
 gcc/testsuite/g++.dg/template/pr84789-3.C |   31 +++++++++++++++++++++++++++++
 gcc/testsuite/g++.dg/template/pr84789.C   |   13 ++++++++++++
 6 files changed, 75 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/template/pr84789-2.C
 create mode 100644 gcc/testsuite/g++.dg/template/pr84789-3.C
 create mode 100644 gcc/testsuite/g++.dg/template/pr84789.C

diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 0f7df14ce148..cf4865148849 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -3455,7 +3455,7 @@ cp_parser_parse_and_diagnose_invalid_type_name (cp_parser *parser)
 				/*template_keyword_p=*/false,
 				/*check_dependency_p=*/true,
 				/*template_p=*/NULL,
-				/*declarator_p=*/true,
+				/*declarator_p=*/false,
 				/*optional_p=*/false);
   /* If the next token is a (, this is a function with no explicit return
      type, i.e. constructor, destructor or conversion op.  */
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index d7c0c7bec81b..5293c2b5491b 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -25249,6 +25249,9 @@ resolve_typename_type (tree type, bool only_current_p)
   gcc_assert (TREE_CODE (type) == TYPENAME_TYPE);
 
   scope = TYPE_CONTEXT (type);
+  /* We shouldn't have built a TYPENAME_TYPE with a non-dependent scope.  */
+  gcc_checking_assert (uses_template_parms (scope));
+
   /* Usually the non-qualified identifier of a TYPENAME_TYPE is
      TYPE_IDENTIFIER (type). But when 'type' is a typedef variant of
      a TYPENAME_TYPE node, then TYPE_NAME (type) is set to the TYPE_DECL representing
@@ -25285,8 +25288,6 @@ resolve_typename_type (tree type, bool only_current_p)
     /* scope is either the template itself or a compatible instantiation
        like X<T>, so look up the name in the original template.  */
     scope = CLASSTYPE_PRIMARY_TEMPLATE_TYPE (scope);
-  /* We shouldn't have built a TYPENAME_TYPE with a non-dependent scope.  */
-  gcc_checking_assert (uses_template_parms (scope));
   /* If scope has no fields, it can't be a current instantiation.  Check this
      before currently_open_class to avoid infinite recursion (71515).  */
   if (!TYPE_FIELDS (scope))
diff --git a/gcc/testsuite/g++.dg/parse/dtor11.C b/gcc/testsuite/g++.dg/parse/dtor11.C
index 63ffb60bac10..83fd93489f11 100644
--- a/gcc/testsuite/g++.dg/parse/dtor11.C
+++ b/gcc/testsuite/g++.dg/parse/dtor11.C
@@ -8,5 +8,5 @@ struct A
 
 struct B
 {
-  A::~B B();  // { dg-error "as member of" }
+  A::~B B();  // { dg-error "as member of|as a type" }
 };
diff --git a/gcc/testsuite/g++.dg/template/pr84789-2.C b/gcc/testsuite/g++.dg/template/pr84789-2.C
new file mode 100644
index 000000000000..0b42148ef3e4
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/pr84789-2.C
@@ -0,0 +1,26 @@
+// { dg-do compile }
+
+struct K {
+  struct L {
+    static double j;
+  };
+};
+
+template <typename T>
+struct M {
+  struct N {
+    static int i;
+  };
+};
+
+template <typename T>
+struct O {
+  typedef M<T> P;
+  typedef K Q;
+};
+
+template <typename T>
+int O<T>::P::N::i = 42; // This is obfuscated, but apparently ok.
+
+template <typename T>
+double O<T>::Q::L::j = 42.0; // { dg-error "non-template" }
diff --git a/gcc/testsuite/g++.dg/template/pr84789-3.C b/gcc/testsuite/g++.dg/template/pr84789-3.C
new file mode 100644
index 000000000000..bc38c1544ba9
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/pr84789-3.C
@@ -0,0 +1,31 @@
+// { dg-do compile }
+
+struct A
+{
+  static int i;
+};
+struct B
+{
+  typedef ::A A;
+};
+int B::A::i = 0;
+
+struct K
+{
+  struct L
+  {
+    template <typename T>
+    static void f(T);
+  };
+};
+
+template <typename T>
+struct O
+{
+  typedef K Q;
+};
+
+template <typename T>
+void O<T>::Q::L::f(T)
+{
+}
diff --git a/gcc/testsuite/g++.dg/template/pr84789.C b/gcc/testsuite/g++.dg/template/pr84789.C
new file mode 100644
index 000000000000..bc1567f3fe77
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/pr84789.C
@@ -0,0 +1,13 @@
+// { dg-do compile }
+
+struct A
+{
+  typedef int I;
+};
+
+template<typename> struct B : A {};
+
+template<typename T> struct C : B<T>
+{
+  B<T>::A::I::I i; // { dg-error "typename" }
+};


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
Jason Merrill March 22, 2018, 3:52 a.m. | #7
OK, thanks.

On Wed, Mar 21, 2018 at 11:48 AM, Jason Merrill <jason@redhat.com> wrote:
> On Tue, Mar 20, 2018 at 11:27 PM, Alexandre Oliva <aoliva@redhat.com> wrote:

>> On Mar 20, 2018, Jason Merrill <jason@redhat.com> wrote:

>>

>>> On Tue, Mar 20, 2018 at 6:07 PM, Alexandre Oliva <aoliva@redhat.com> wrote:

>>>> On Mar 20, 2018, Jason Merrill <jason@redhat.com> wrote:

>>>>> that doesn't mean it's wrong to peek.

>>

>>>> Huh?  We're referencing members of an unrelated template that AFAIK

>>>> needs not even be defined at that point, and even if it is, it could

>>>> still be specialized afterwards.  How can it possibly be right to

>>>> short-circuit such nested names?

>>

>>>> template<typename> struct B : A {}; // would /*: A {}*/ make any diff?

>>>> template<typename T> struct C : B<T> // would /* : B<T>*/ make any diff?

>>>> {

>>>>   B<T>::A::I::I i; // { dg-error "typename" }

>>>> };

>>

>>> No, we look inside when we're trying to parse the qualified-id as the

>>> name of a declaration

>>

>> Yeah, but that's not the case at hand.  I guess we're miscommunicating.

>> I understood you were saying it was ok to peek in this case.  Would you

>> please state, for clarity, what your stance is on peeking in this case,

>> specifically, in the B<T>::A::I::I within the definition of C above?

>

> It's OK when we're tentatively trying to parse it as a declarator, not

> when we're trying to parse it as a type-specifier.

>

> We seem to be talking past each other on this point: We already don't

> peek when parsing a type-specifier, we only call resolve_typename_type

> with false for only_current_p in the context of trying to parse a

> declarator.  The parser tries both interpretations for B<T>::A::I::I.

> The type interpretation is ill-formed because of missing 'typename',

> so it tries again as a declarator, and therefore tries to resolve the

> TYPENAME_TYPE that it built while trying for a type, and trips the

> assert.

>

>>> in a declarator-id we can look into uninstantiated classes, otherwise

>>> there would be no way to define members of class templates.

>>

>>>   void X<T>::N::f() { } // looks inside X<T>

>>

>> Of course, but then, we wouldn't get to a template-independent member.

>> A member of a template-dependent name is still template-dependent.  It's

>> only when a qualified name maps to an external name that it might become

>> template-independent, and when this happens, I believe the qualified-id

>> is not one that can be used to define a member. Specifically:

>>

>> struct K { struct L { static double j; }; };

>> template <typename T> struct M { struct N { static int i; }; };

>> template <typename T> struct O { typedef M<T> P; typedef K Q; };

>> template <typename T> int O<T>::P::N::i = 42;

>> template <typename T> double O<T>::Q::L::j = 42.0;

>>

>> if we remap O<T>::P to M<T> and O<T>::Q to K, how will we realize the

>> given type-ids are not appropriate?  Where will the template parmlist

>> belong when the qualified-id is taken as equivalent to K::L::j?

>

> Agreed, these look bizarre because the template parm is used in O<T>,

> which is not an enclosing class of i or j.  And j is clearly

> ill-formed because it declares a non-template as a template.

>

> But it's not clear to me that i is ill-formed; we allow

>

> struct A { static int i; };

> struct B { typedef ::A A; };

> int B::A::i = 0;

>

> and the i example seems analogous.  I wouldn't argue against making it

> ill-formed, but I don't see a rule that would make it so in the

> current draft standard.  And even resolving the scope to be

> non-dependent doesn't necessarily mean the declaration will be, if the

> template parameter list ends up being for a member template:

>

> struct K { struct L { template <typename T> static void f(T); }; };

> template <typename T> struct O { typedef K Q; };

> template <typename T> void O<T>::Q::L::f(T) { }

>

>>>>> I disagree; it seems to me that the assert should allow the case where

>>>>> the scope was originally dependent, but got resolved earlier in the

>>>>> function.

>>>>

>>>> Doesn't the function always take dependent scopes?  I for some reason

>>>> thought that was the case.

>>

>>> Yes, as the comment says, a TYPENAME_TYPE should always have a

>>> dependent scope.  In this case, the dependent scope was "typename

>>> B<T>::A", but just above we resolved it to just A, making it no longer

>>> dependent.

>>

>> Then you got me thoroughly confused: what did you mean by 'the assert

>> should allow' a case that's a given?

>

> asserts are often used to verify that things we think of as givens are

> actually true.  :)

>

> I suppose we could remove the assert, but I'd probably move it up

> above where we start messing with 'scope'.

>

> Jason
Jakub Jelinek March 23, 2018, 2:12 p.m. | #8
On Wed, Mar 21, 2018 at 11:52:33PM -0400, Jason Merrill wrote:
> OK, thanks.


Note the testcase FAILs with -fconcepts when I do make check-c++-all,
FAIL: g++.dg/template/pr84789.C  -std=c++17 -fconcepts  (test for errors, line 12)
FAIL: g++.dg/template/pr84789.C  -std=c++17 -fconcepts (test for excess errors)
Excess errors:
/usr/src/gcc/gcc/testsuite/g++.dg/template/pr84789.C:12:15: error: 'A::I' {aka 'int'} is not a class type
/usr/src/gcc/gcc/testsuite/g++.dg/template/pr84789.C:12:15: error: 'A::I' {aka 'int'} is not a class type
/usr/src/gcc/gcc/testsuite/g++.dg/template/pr84789.C:12:15: error: 'A::I' {aka 'int'} is not a class type
/usr/src/gcc/gcc/testsuite/g++.dg/template/pr84789.C:12:15: error: 'A::I' {aka 'int'} is not a class type
/usr/src/gcc/gcc/testsuite/g++.dg/template/pr84789.C:12:15: error: 'A::I' {aka 'int'} is not a class type
/usr/src/gcc/gcc/testsuite/g++.dg/template/pr84789.C:12:15: error: 'A::I' {aka 'int'} is not a class type
/usr/src/gcc/gcc/testsuite/g++.dg/template/pr84789.C:12:15: error: 'I' in 'A::I' {aka 'int'} does not name a type
In all other standard modes it is fine.

	Jakub
Alexandre Oliva March 23, 2018, 4:45 p.m. | #9
On Mar 23, 2018, Jakub Jelinek <jakub@redhat.com> wrote:

> On Wed, Mar 21, 2018 at 11:52:33PM -0400, Jason Merrill wrote:

>> OK, thanks.


> Note the testcase FAILs with -fconcepts when I do make check-c++-all,


Hmm, I don't get that with check or check-g++.  Should we expand the
default std_list in g++-dg.exp, or should I manually test these
additional variants?

Anyway, thanks for the report, I'll try to figure out why this fails
with concepts.

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
Jakub Jelinek March 23, 2018, 4:53 p.m. | #10
On Fri, Mar 23, 2018 at 01:45:01PM -0300, Alexandre Oliva wrote:
> On Mar 23, 2018, Jakub Jelinek <jakub@redhat.com> wrote:

> 

> > On Wed, Mar 21, 2018 at 11:52:33PM -0400, Jason Merrill wrote:

> >> OK, thanks.

> 

> > Note the testcase FAILs with -fconcepts when I do make check-c++-all,

> 

> Hmm, I don't get that with check or check-g++.  Should we expand the

> default std_list in g++-dg.exp, or should I manually test these

> additional variants?


I think we should add the -std=c++17 mode to the list of standards cycled by
default, now that it is a released standard.  -fconcepts is experimental
and so is -std=c++2a, so perhaps those can stay out of the default and be
just checked occassionally.

The full set of check-c++-all FAILs I'm getting that don't show up with
check-g++ is:
FAIL: g++.dg/spellcheck-macro-ordering-2.C  -std=c++17 -fconcepts  (test for errors, line 10)
FAIL: g++.dg/spellcheck-macro-ordering-2.C  -std=c++17 -fconcepts (test for excess errors)
FAIL: g++.dg/spellcheck-macro-ordering.C  -std=c++17 -fconcepts  (test for errors, line 6)
FAIL: g++.dg/spellcheck-macro-ordering.C  -std=c++17 -fconcepts  (test for warnings, line 6)
FAIL: g++.dg/spellcheck-macro-ordering.C  -std=c++17 -fconcepts  (test for warnings, line 15)
FAIL: g++.dg/spellcheck-macro-ordering.C  -std=c++17 -fconcepts (test for excess errors)
FAIL: g++.dg/template/pr84789.C  -std=c++17 -fconcepts  (test for errors, line 12)
FAIL: g++.dg/template/pr84789.C  -std=c++17 -fconcepts (test for excess errors)
FAIL: g++.dg/warn/string1.C  -std=gnu++17 (test for excess errors)
FAIL: g++.dg/warn/string1.C  -std=gnu++2a (test for excess errors)
FAIL: g++.dg/warn/string1.C  -std=gnu++17 -fconcepts (test for excess errors)

	Jakub
Alexandre Oliva March 23, 2018, 7:17 p.m. | #11
On Mar 23, 2018, Jakub Jelinek <jakub@redhat.com> wrote:

> On Wed, Mar 21, 2018 at 11:52:33PM -0400, Jason Merrill wrote:

>> OK, thanks.


> Note the testcase FAILs with -fconcepts when I do make check-c++-all,

> FAIL: g++.dg/template/pr84789.C  -std=c++17 -fconcepts  (test for errors, line 12)

> FAIL: g++.dg/template/pr84789.C  -std=c++17 -fconcepts (test for excess errors)

> Excess errors:

> /usr/src/gcc/gcc/testsuite/g++.dg/template/pr84789.C:12:15: error:

> 'A::I' {aka 'int'} is not a class type

> /usr/src/gcc/gcc/testsuite/g++.dg/template/pr84789.C:12:15: error: 'I'

> in 'A::I' {aka 'int'} does not name a type


Is this ok to install?

[PR c++/84789] adjust testcase for -fconcepts

When compiling with -fconcepts,
cp_parser_template_declaration_after_export calls
cp_parser_template_introduction and that preparses qualified-ids not
preceded by typename in such a way that, when we get to
cp_parser_parse_and_diagnose_invalid_type_name and then
cp_parser_diagnose_invalid_type_name, the nested name specifier no
longer carries the previous template-dependent context, so we don't
stand a chance to suggest the use of 'typename' any more.  Thus,
tolerate in the testcase the poorer error messages we get.

for  gcc/testsuite/ChangeLog

	PR c++/84789
	* g++.dg/template/pr84789.C: Adjust for testing with
	-fconcepts too.
---
 gcc/testsuite/g++.dg/template/pr84789.C |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/testsuite/g++.dg/template/pr84789.C b/gcc/testsuite/g++.dg/template/pr84789.C
index bc1567f3fe77..63b9832fecf8 100644
--- a/gcc/testsuite/g++.dg/template/pr84789.C
+++ b/gcc/testsuite/g++.dg/template/pr84789.C
@@ -9,5 +9,5 @@ template<typename> struct B : A {};
 
 template<typename T> struct C : B<T>
 {
-  B<T>::A::I::I i; // { dg-error "typename" }
+  B<T>::A::I::I i; // { dg-error "not a class type|does not name a type|typename" }
 };


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
Jason Merrill March 23, 2018, 7:35 p.m. | #12
OK.

On Fri, Mar 23, 2018 at 3:17 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Mar 23, 2018, Jakub Jelinek <jakub@redhat.com> wrote:

>

>> On Wed, Mar 21, 2018 at 11:52:33PM -0400, Jason Merrill wrote:

>>> OK, thanks.

>

>> Note the testcase FAILs with -fconcepts when I do make check-c++-all,

>> FAIL: g++.dg/template/pr84789.C  -std=c++17 -fconcepts  (test for errors, line 12)

>> FAIL: g++.dg/template/pr84789.C  -std=c++17 -fconcepts (test for excess errors)

>> Excess errors:

>> /usr/src/gcc/gcc/testsuite/g++.dg/template/pr84789.C:12:15: error:

>> 'A::I' {aka 'int'} is not a class type

>> /usr/src/gcc/gcc/testsuite/g++.dg/template/pr84789.C:12:15: error: 'I'

>> in 'A::I' {aka 'int'} does not name a type

>

> Is this ok to install?

>

> [PR c++/84789] adjust testcase for -fconcepts

>

> When compiling with -fconcepts,

> cp_parser_template_declaration_after_export calls

> cp_parser_template_introduction and that preparses qualified-ids not

> preceded by typename in such a way that, when we get to

> cp_parser_parse_and_diagnose_invalid_type_name and then

> cp_parser_diagnose_invalid_type_name, the nested name specifier no

> longer carries the previous template-dependent context, so we don't

> stand a chance to suggest the use of 'typename' any more.  Thus,

> tolerate in the testcase the poorer error messages we get.

>

> for  gcc/testsuite/ChangeLog

>

>         PR c++/84789

>         * g++.dg/template/pr84789.C: Adjust for testing with

>         -fconcepts too.

> ---

>  gcc/testsuite/g++.dg/template/pr84789.C |    2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

>

> diff --git a/gcc/testsuite/g++.dg/template/pr84789.C b/gcc/testsuite/g++.dg/template/pr84789.C

> index bc1567f3fe77..63b9832fecf8 100644

> --- a/gcc/testsuite/g++.dg/template/pr84789.C

> +++ b/gcc/testsuite/g++.dg/template/pr84789.C

> @@ -9,5 +9,5 @@ template<typename> struct B : A {};

>

>  template<typename T> struct C : B<T>

>  {

> -  B<T>::A::I::I i; // { dg-error "typename" }

> +  B<T>::A::I::I i; // { dg-error "not a class type|does not name a type|typename" }

>  };

>

>

> --

> Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/

> You must be the change you wish to see in the world. -- Gandhi

> Be Free! -- http://FSFLA.org/   FSF Latin America board member

> Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Patch

diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 89024c10fe2b..067221fa78ea 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -25195,8 +25195,23 @@  resolve_typename_type (tree type, bool only_current_p)
     /* scope is either the template itself or a compatible instantiation
        like X<T>, so look up the name in the original template.  */
     scope = CLASSTYPE_PRIMARY_TEMPLATE_TYPE (scope);
-  /* We shouldn't have built a TYPENAME_TYPE with a non-dependent scope.  */
-  gcc_checking_assert (uses_template_parms (scope));
+  /* We shouldn't have built a TYPENAME_TYPE with a non-dependent
+     scope.  However, it might be a dependent scope that's being
+     resolved to a non-dependent scope just because we're looking up
+     scopes we shouldn't, e.g.
+
+       template <typename> class B { typedef int I; };
+       template <typename T> class C : B<T> { B<T>::I i; };
+
+     We need 'typename' before C<T>::i's type, because we can't enter
+     the scope of B<T> for an unbound template parameter T to tell I
+     identifies a type (that's why we need typename), but the parser
+     attempts to do so, presumably so that it can produce better error
+     messages.  However, we'd skip necessary errors if we resolved a
+     template-dependent type to a template-independent one, so don't
+     do that.  */
+  if (!uses_template_parms (scope))
+    return type;
   /* If scope has no fields, it can't be a current instantiation.  Check this
      before currently_open_class to avoid infinite recursion (71515).  */
   if (!TYPE_FIELDS (scope))
diff --git a/gcc/testsuite/g++.dg/template/pr84789.C b/gcc/testsuite/g++.dg/template/pr84789.C
new file mode 100644
index 000000000000..bc1567f3fe77
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/pr84789.C
@@ -0,0 +1,13 @@ 
+// { dg-do compile }
+
+struct A
+{
+  typedef int I;
+};
+
+template<typename> struct B : A {};
+
+template<typename T> struct C : B<T>
+{
+  B<T>::A::I::I i; // { dg-error "typename" }
+};