Bug 22136 - [4.1/4.2 regression] Rejects old-style using declaration
Summary: [4.1/4.2 regression] Rejects old-style using declaration
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 4.1.0
: P1 normal
Target Milestone: 4.1.0
Assignee: Mark Mitchell
URL:
Keywords: rejects-valid
Depends on:
Blocks: 26102
  Show dependency treegraph
 
Reported: 2005-06-21 16:24 UTC by Wolfgang Bangerth
Modified: 2006-09-07 21:37 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2005-10-12 00:17:49


Attachments
Patch that disallows nested-name-specifiers for constructors when in class scope. (902 bytes, patch)
2006-01-16 00:04 UTC, Mark Mitchell
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wolfgang Bangerth 2005-06-21 16:24:36 UTC
This used to compile but doesn't any more on mainline (ok on 4.0.x branch): 
----------------------- 
struct B { 
    void foo(); 
}; 
 
template <typename T> class I : public B {}; 
 
template <typename T> class D : private I<T> { 
    I<T>::B::foo; 
}; 
-------------------------- 
 
g/x> /home/bangerth/bin/gcc-4.0.1-pre/bin/c++ -c x.cc 
g/x> /home/bangerth/bin/gcc-4.1-pre/bin/c++ -c x.cc 
x.cc:8: error: type &#8216;B&#8217; is not a base type for type &#8216;D<T>&#8217; 
 
W.
Comment 1 Andrew Pinski 2005-06-21 16:28:19 UTC
Confirmed.
Comment 2 Andrew Pinski 2005-07-16 21:12:25 UTC
Hmm, I almost think this was caused by:
2005-06-08  Nathan Sidwell  <nathan@codesourcery.com>

        PR c++/19497
        * cp-tree.def (USING_DECL): Update documentation.
        * cp-tree.h (DECL_DEPENDENT_P): New.
        (USING_DECL_DECLS, USING_DECL_SCOPE): New.
        * class.c (handle_using_decl): Move most of the processing to ...
        * name-lookup.c (do_class_using_decl): ... here.  Make stricter.
        (push_using_decl): Use USING_DECL_SCOPE.
        (cp_emit_debug_info_for_using): Make extern.
        * cxx-pretty-print.c (pp_cxx_statement) <USING_DECL case>: Adjust.
        * name-lookup.h (cp_emit_debug_info_for_using): Declare.
        * pt.c (tsubst_decl) <USING_DECL case>: Use do_class_using_decl
        when tsubsting.
        (tsubst_expr): Use USING_DECL_SCOPE.
        * search.c (lookup_field_1): Use DECL_DEPENDENT_P.
        * semantics.c (finish_member_declaration): Likewise.
Comment 3 Andrew Pinski 2005-10-12 00:17:49 UTC
Any news on this one?
Comment 4 Mark Mitchell 2005-10-31 03:50:31 UTC
Leaving as P2; we should fix this.
Comment 5 Andrew Pinski 2005-11-11 03:36:13 UTC
For some reason we get the record B and not just I<T>::B which would be dependent.

Looking more into it.
Comment 6 Andrew Pinski 2005-11-11 04:30:38 UTC
The difference between having the use and the no use case is that we get
a nondependent name for naming the struct B which is just wrong.
Hmm, I think I found related rejects valid for before 4.1.0:
struct B {
            void foo();
};

namespace a
{
struct B {
};
}

template <typename T> class I : public a::B {};
template<> class I<int> : public B {};
template <typename T> class D : private I<T> {
            I<T>::B::foo;
};

D<int> i;

--------
This is not a regression though.
The error we get:
t.cc: In instantiation of `D<int>':
t.cc:18:   instantiated from here
t.cc:14: error: type `a::B' is not a base type for type `D<int>'

Now to figure out why we are getting a::B instead of D<T>::B like we get in the using case.
Comment 7 Andrew Pinski 2005-11-11 05:22:24 UTC
parser->scope is wrong when we call make_id_declarator.
Comment 8 Andrew Pinski 2005-11-11 05:35:07 UTC
For some reason we are calling cp_parser_pre_parsed_nested_name_specifier.
Comment 9 Andrew Pinski 2005-11-11 05:40:01 UTC
(In reply to comment #8)
> For some reason we are calling cp_parser_pre_parsed_nested_name_specifier.

Actually that is fine.
Though we got the wrong answer already in there.
Comment 10 Andrew Pinski 2006-01-14 05:57:09 UTC
I think this should get higher than a P2 as this is a rejects valid and I even identified which patch caused the regression.  CCing Mark.  Also the regression which that patch fixed was an accepts invalid and just being diagnostic too late (after template instantiate).  Also I should note that it has been more than 48 hours (the rule goes into effect once the person who caused is notified) quote from http://gcc.gnu.org/develop.html:


Patch Reversion

If a patch is committed which introduces a regression on any target which the Steering Committee considers to be important and if:

the problem is reported to the original poster;
48 hours pass without the original poster or any other party indicating that a fix will be forthcoming in the very near future;
two people with write privileges to the affected area of the compiler determine that the best course of action is to revert the patch;
then they may revert the patch.

(The list of important targets will be revised at the beginning of each release cycle, if necessary, and is part of the release criteria.)

After the patch has been reverted, the poster may appeal the decision to the Steering Committee.

Note that no distinction is made between patches which are themselves buggy and patches that expose latent bugs elsewhere in the compiler.
Comment 11 Nathan Sidwell 2006-01-15 14:04:10 UTC
What's happening is that we parse I<T>::B as a type specifier and then squirrel away a preparsed template type specifier, but because I<T>::B is a non-dependent type, the preparsed specifier becomes simply ::B, losing the base path information.

I think the bug is that the preparsed template id is saved regardless of the check_dependency, is_declaration and type_p flags.  I spent some time looking at this and got confused.  I really must allocate some time to look again.
Comment 12 Mark Mitchell 2006-01-15 23:12:55 UTC
I agree that this should be a P1.

Why do we think I<T>::B is a non-dependent type?  It should be considered dependent, because we may have a specialization of I for which B is not a base class.  There are some cases in the parser where we must resolve dependent types; I wonder if we're incorrectly doing that in this case.
Comment 13 Mark Mitchell 2006-01-15 23:57:08 UTC
The problem is that we originally encounter the nested name specifier I<T>::B during a call from cp_parser_constructor_declarator_p.  That function sets check_dependency_p to false, because we do have to resolve dependency names when parsing declarators.  The result of the nested name specifier lookup is cached at that point, with the result that when we re-encounter it, we use the value we got before, even though that is no longer correct.

Because the caching should not be required for correctness (although it does eliminate duplicate error messages in some cases), we could probably disable the caching.  

Another approach would be not to look for the nested name specifier in class scope, since there it is not valid to declare a constructor in-class as "C::C", although we have long accepted that, with a warning.  Preliminary testing shows that will fix the bug, although it will break 
  struct S { S::S(); };
which is current accepted with -fpermissive.  I will attach that patch for reference.

Because of the problem with -fpermissive, I think it's better to try conditionalizing the caching in cp_parser_nested_name_specifier_opt on check_dependency.  That also fixes the bug; I'm testing that patch now.

For the original submitter: ARM-style access declarations (e.g., "I<T>::B::foo;") are deprecated in current C++.  The preferred way to write the code is now "using I<T>::B::foo;", and that additional contextual information avoids the bug in G++ as well.  So, the best immediate workaround is to insert the "using" keyword.
Comment 14 Mark Mitchell 2006-01-16 00:04:44 UTC
Created attachment 10650 [details]
Patch that disallows nested-name-specifiers for constructors when in class scope.


This patch is not recommended, but it does seem to work, modulo the fact that it makes this code fail even with -fpermissive:

  struct S { S::S(); };
Comment 15 bangerth@math.tamu.edu 2006-01-16 00:10:37 UTC
Subject: Re:  [4.1/4.2 regression] Rejects old-style using
 declaration


> For the original submitter: ARM-style access declarations (e.g.,
> "I<T>::B::foo;") are deprecated in current C++.  The preferred way to write the
> code is now "using I<T>::B::foo;", and that additional contextual information
> avoids the bug in G++ as well.  So, the best immediate workaround is to insert
> the "using" keyword.

That is of course exactly what I did :-)

I don't particularly care about this bug, we've already fixed this in our
sources. I do not know, however, how many people use older versions of our
library with newer compilers.

Best
  Wolfgang

-------------------------------------------------------------------------
Wolfgang Bangerth                email:            bangerth@math.tamu.edu
                                 www: http://www.math.tamu.edu/~bangerth/

Comment 16 Mark Mitchell 2006-01-18 21:55:05 UTC
I'm still wrestling with this PR.

As I suggested earlier, I turned off the caching of nested-name-specifiers unless we're in the check_dependency_p case.  However, that causes g++.dg/parse/operator2.C to fail, for essentially the opposite reason.

On:

template <typename T> B<T>::C::operator typename B<T>::Y::X() const { return 0;\
 }

we cache the check_dependency_p lookup for B<T>::C, which is "typename B<T>::C".  As a result, we don't enter the scope of B<T>::C when looking up names in the type-specifier following the operator.

I think that we could fix that in cp_paser_conversion_function_id (by using resolve_typename_type), but I'm afraid that it's not really safe to cache either version of the nested-name-specifier lookup, and then return it for the other case of check_dependency_p.

Still thinking.
Comment 17 Mark Mitchell 2006-01-18 22:25:20 UTC
In retrospect, I wonder if we should be complaining about a using-declaration in a template in the first place.  

For example, is:

  struct X { void f(); };

  template <typename T>
  struct S : public T {
    using X::f;
  };

actually invalid? 

Both EDG and G++ complain about this (saying that X is not a base class of S<T>), but should that matter at this stage?
Comment 18 Mark Mitchell 2006-01-19 21:27:51 UTC
I've spoken with the folks at EDG, and we all agree that we should not be checking that, in "using S::f", "S" is a base class of the current class if we're in a template; the set of base classes of the template is not, in general, known at that point.  Adjusting do_class_using_decl fixes that problem.

However, that fix will likely reopen the problem that our class-scope using declarations are really just ARM access declarations, and, therefore, don't work as people expect in templates.  At some point, we're really going to need to *fix* that problem; in the meanwhile, I guess I can look for another way to hack around that.
Comment 19 Mark Mitchell 2006-01-20 03:07:55 UTC
Subject: Bug 22136

Author: mmitchel
Date: Fri Jan 20 03:07:49 2006
New Revision: 110016

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=110016
Log:
	PR c++/22136
	* name-lookup.c (do_class_using_decl): Don't try to look up base
	classes in templates with dependent base types.
	PR c++/22136
	* g++.dg/template/using10.C: New test.
	* g++.dg/temlpate/using11.C: Likewise.
	* g++.dg/inherit/using5.C: Tweak error messages.

Added:
    branches/gcc-4_1-branch/gcc/testsuite/g++.dg/template/using10.C
    branches/gcc-4_1-branch/gcc/testsuite/g++.dg/template/using11.C
Modified:
    branches/gcc-4_1-branch/gcc/cp/ChangeLog
    branches/gcc-4_1-branch/gcc/cp/name-lookup.c
    branches/gcc-4_1-branch/gcc/testsuite/ChangeLog
    branches/gcc-4_1-branch/gcc/testsuite/g++.dg/inherit/using5.C

Comment 20 Mark Mitchell 2006-01-20 03:08:06 UTC
Subject: Bug 22136

Author: mmitchel
Date: Fri Jan 20 03:07:58 2006
New Revision: 110017

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=110017
Log:
	PR c++/22136
	* name-lookup.c (do_class_using_decl): Don't try to look up base
	classes in templates with dependent base types.
	PR c++/22136
	* g++.dg/template/using10.C: New test.
	* g++.dg/temlpate/using11.C: Likewise.
	* g++.dg/inherit/using5.C: Tweak error messages.

Added:
    trunk/gcc/testsuite/g++.dg/template/using10.C
    trunk/gcc/testsuite/g++.dg/template/using11.C
Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/name-lookup.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/g++.dg/inherit/using5.C

Comment 21 Mark Mitchell 2006-01-20 03:36:52 UTC
Fixed in 4.1.0.