From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 24D1C3844047 for ; Thu, 6 Aug 2020 13:47:54 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 24D1C3844047 Received: from mail-qv1-f71.google.com (mail-qv1-f71.google.com [209.85.219.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-243-WSd0zGTsND2j99Fln9Z6LQ-1; Thu, 06 Aug 2020 09:47:52 -0400 X-MC-Unique: WSd0zGTsND2j99Fln9Z6LQ-1 Received: by mail-qv1-f71.google.com with SMTP id k17so19865733qvj.12 for ; Thu, 06 Aug 2020 06:47:52 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:date:to:cc:subject:in-reply-to:message-id :references:mime-version; bh=v7WTgcPmPqHzjkdDMkkDoT/lKMJOh/3I5VK0PXJbtFo=; b=DE/KoP8XGdouiuTvEVEB9462t4IdRRwiM15/zHkxUQYCYBbzSwtlYX0uyWD48y5P3h u/3ipyhWsEKSX23aLVopkzynWrhc5GMqKjD8Ycpgq6j37vLVKuCbGmb20u5QnPnCisNh EYQuwxf3S9nzddoD3aK+IKBNNtXHYFwWsCvyB8JR+7gpbEa6uzrLeR4xM7gB6EjCgj5y fUDnrdakHQ7eatgL+G8xbUJvppPvmn1w1aBVGr2QQY08eilk5Q/06O6THJMbTx7UyGdN SRrl3tANCqSN24JIJ065VQeqwtOtPWmsZTuaFOaikK8hQ7HmttIbi5gvjhqGoRyDFagx IDSQ== X-Gm-Message-State: AOAM533XHTYreTFeR5kDSEofmWb2OQlqD8b375Sg+eJXIENcrpsGntdS i5g4vdoIuAifBtkroeA8smLPk+W0/lAyDatWyP/EJmCKadVKuv/crqvvW2r4Ba2bXewm5bGB5Lh WkUtgO/h1PH/VGbcBkQ== X-Received: by 2002:ac8:6c49:: with SMTP id z9mr9194580qtu.110.1596721671514; Thu, 06 Aug 2020 06:47:51 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwH/r+yZFYxdLIEJTwjYHDz97NlIvtAwuV7Iw6CwvW1pZHH5lAC5sqRUYx2CZ7FCCK5W9CgKg== X-Received: by 2002:ac8:6c49:: with SMTP id z9mr9194554qtu.110.1596721671211; Thu, 06 Aug 2020 06:47:51 -0700 (PDT) Received: from [192.168.1.130] (ool-457d493a.dyn.optonline.net. [69.125.73.58]) by smtp.gmail.com with ESMTPSA id g3sm4746433qtq.70.2020.08.06.06.47.50 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 06 Aug 2020 06:47:50 -0700 (PDT) From: Patrick Palka X-Google-Original-From: Patrick Palka Date: Thu, 6 Aug 2020 09:47:49 -0400 (EDT) To: Jason Merrill cc: Patrick Palka , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] c++: dependent constraint on placeholder return type [PR96443] In-Reply-To: <6dec3a7d-d5c3-9a9e-9ed0-a886384ebfa4@redhat.com> Message-ID: <26f82e21-8fb-2b6-7389-541bddf55d8f@idea> References: <20200804192234.2282332-1-ppalka@redhat.com> <6dec3a7d-d5c3-9a9e-9ed0-a886384ebfa4@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-16.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 06 Aug 2020 13:47:55 -0000 On Wed, 5 Aug 2020, Jason Merrill wrote: > On 8/4/20 8:00 PM, Patrick Palka wrote: > > On Tue, 4 Aug 2020, Patrick Palka wrote: > > > > > In the testcase below, we never substitute function-template arguments > > > into f15's placeholder-return-type constraint, which leads to us > > > incorrectly rejecting this instantiation in do_auto_deduction due to > > > satisfaction failure (of the constraint SameAs). > > > > > > The fact that we incorrectly reject this testcase is masked by the > > > other instantiation f15, which we correctly reject and diagnose > > > (by accident). > > > > > > A good place to do this missing substitution seems to be during > > > TEMPLATE_TYPE_PARM level lowering. So this patch adds a call to > > > tsubst_constraint there, and also adds dg-bogus directives to this > > > testcase wherever we expect instantiation to succeed. (So without the > > > substitution fix, this last dg-bogus would FAIL). > > > > Successfully tested on x86_64-pc-linux-gnu, and also on the cmcstl2 and > > > range-v3 projects. Does this look OK to commit? > > > > > > gcc/cp/ChangeLog: > > > > > > PR c++/96443 > > > * pt.c (tsubst) : Substitute into > > > the constraints on a placeholder type when its level. > > > > > > gcc/testsuite/ChangeLog: > > > > > > PR c++/96443 > > > * g++.dg/cpp2a/concepts-ts1.C: Add dg-bogus wherever we expect > > > instantiation to succeed. > > > > Looking back at this patch with fresh eyes, I realized that the commit > > message is not the best. I rewrote the commit message to hopefully be > > more coherent below: > > > > -- >8 -- > > > > Subject: [PATCH] c++: dependent constraint on placeholder return type > > [PR96443] > > > > In the testcase concepts-ts1.C, we're incorrectly rejecting the call to > > 'f15(0)' due to satisfaction failure of the function's > > placeholder-return-type constraint. > > > > The testcase doesn't spot this rejection because the error we emit for > > the constraint failure points to f15's return statement instead of the > > call site, and we already have a dg-error at the return statement to > > verify the (correct) rejection of the call f15('a'). So in order to > > verify that we indeed accept the call 'f15(0)', we need to add a > > dg-bogus directive at the call site to look for the "required from here" > > diagnostic line that generally accompanies an instantiation failure. > > > > As for why satisfaction failure occurs, it turns out that we never > > substitute the template arguments of a function template specialization > > in to its placeholder-return-type constraint. So in this case during > > do_auto_deduction, we end up checking satisfaction of the still-dependent > > constraint SameAs from do_auto_deduction, which fails > > because it's dependent. > > > > A good place to do this missing substitution seems to be during > > TEMPLATE_TYPE_PARM level lowering; so this patch adds a call to > > tsubst_constraint there. > > Doing substitution seems like the wrong approach here; requirements should > never be substituted except as part of satisfaction calculation (or, rarely, > for declaration matching). If we aren't communicating all the necessary > template arguments to the later satisfaction, that's what we need to fix. Ah okay, that makes sense. It also looks like the question of perform a single full substitution (during auto deduction) vs two substitutions may also be a correctness issue in light of SFINAE. Consider the following testcase: template concept C = true; auto f(auto x) -> C auto { return 0; } auto f(auto x, ...) { return 1; } int a = f(0); If we do a single substitution, then the substitution failure is a hard error and we'll reject this testcase. If we do two substitutions, then it's a SFINAE error and we select the second overload. Would we be correct to issue a hard error here? > > > Successfully tested on x86_64-pc-linux-gnu, and also on the cmcstl2 and > > range-v3 projects. Does this look OK to commit? > > > > gcc/cp/ChangeLog: > > > > PR c++/96443 > > * pt.c (tsubst) : Substitute into > > the constraints on a placeholder type when reducing its level. > > > > gcc/testsuite/ChangeLog: > > > > PR c++/96443 > > * g++.dg/cpp2a/concepts-ts1.C: Add dg-bogus to the call to f15 > > that we expect to accept. > > --- > > gcc/cp/pt.c | 7 ++++--- > > gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C | 2 +- > > 2 files changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c > > index e7496002c1c..9f3426f8249 100644 > > --- a/gcc/cp/pt.c > > +++ b/gcc/cp/pt.c > > @@ -15524,10 +15524,11 @@ tsubst (tree t, tree args, tsubst_flags_t > > complain, tree in_decl) > > if (TREE_CODE (t) == TEMPLATE_TYPE_PARM) > > { > > - /* Propagate constraints on placeholders since they are > > - only instantiated during satisfaction. */ > > + /* Substitute constraints on placeholders when reducing > > + their level. */ > > if (tree constr = PLACEHOLDER_TYPE_CONSTRAINTS (t)) > > - PLACEHOLDER_TYPE_CONSTRAINTS (r) = constr; > > + PLACEHOLDER_TYPE_CONSTRAINTS (r) > > + = tsubst_constraint (constr, args, complain, in_decl); > > else if (tree pl = CLASS_PLACEHOLDER_TEMPLATE (t)) > > { > > pl = tsubst_copy (pl, args, complain, in_decl); > > diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C > > b/gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C > > index 1cefe3b243f..a116cac4ea4 100644 > > --- a/gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C > > +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-ts1.C > > @@ -40,7 +40,7 @@ void driver() > > f3('a'); // { dg-error "" } > > f4(0, 0); > > f4(0, 'a'); // { dg-error "" } > > - f15(0); > > + f15(0); // { dg-bogus "" } > > f15('a'); // { dg-message "" } > > } > > > >