From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-1.mimecast.com (us-smtp-2.mimecast.com [205.139.110.61]) by sourceware.org (Postfix) with ESMTP id EA9983857C53 for ; Mon, 10 Aug 2020 18:51:54 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org EA9983857C53 Received: from mail-qv1-f72.google.com (mail-qv1-f72.google.com [209.85.219.72]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-246-hbX2OLWAOK-KJcVhZqnPHA-1; Mon, 10 Aug 2020 14:51:53 -0400 X-MC-Unique: hbX2OLWAOK-KJcVhZqnPHA-1 Received: by mail-qv1-f72.google.com with SMTP id d9so7866061qvl.10 for ; Mon, 10 Aug 2020 11:51: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=10I/uCy6KCO1wMm/k+YUySKKpz1Kw0jiHi7eI4Z2V+Q=; b=CG+UOxxzlg7VajDe3WOjyMudpRescTmi8Hjjz4ZY6isLMc5PFVd/8nTqDLkmg9kUOQ qkjIaOQzK1b1WLSiPgcyOBHnkxZ3E54QD8Digk4E2zJ0t5MYQsraD+PvHADt9T42i+AJ GZO9/NosBOpCBeMgPILRQm9B+C5ayJ4L+NqivWm7fVXABygCfxxEjDT6MwbXV2P2UdcZ kkhuhnLdhb4O3Vy+Ld+QODBwOXVWhyIajZXpyMQSL1+xecukzsz+DsQo6/JUUCefEGzr BdFZFdQ+PZNOlyO/KIC8ge1GoTVDoi0XK3FtOvMTFf4elm9Z/9yg/lGG43YkhVO0rP/Y cUog== X-Gm-Message-State: AOAM531J86l+2VAezUnsCwvb2TVun+xguMQeSN44I+t5EL91BFbxI4er bBthBNftRJuwB5qO3NEeoEVTWmqX9LK5Y3J86v/q6Yv9pR00eJNCX60+3ghQHVe5q715tEoeMjR qU1e6EDlFNOmGy7Rmtw== X-Received: by 2002:ac8:1382:: with SMTP id h2mr29256295qtj.228.1597085512437; Mon, 10 Aug 2020 11:51:52 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxswjBctrIC7gKvuRsUjDU09I3cyGoBdUAnJZib8O4a+AYdh6o3cTjvKRUvr/hHhsCVB5YJ8g== X-Received: by 2002:ac8:1382:: with SMTP id h2mr29256274qtj.228.1597085512126; Mon, 10 Aug 2020 11:51:52 -0700 (PDT) Received: from [192.168.1.130] (ool-457d493a.dyn.optonline.net. [69.125.73.58]) by smtp.gmail.com with ESMTPSA id z24sm14167415qki.57.2020.08.10.11.51.51 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 10 Aug 2020 11:51:51 -0700 (PDT) From: Patrick Palka X-Google-Original-From: Patrick Palka Date: Mon, 10 Aug 2020 14:51:50 -0400 (EDT) To: Patrick Palka cc: Jason Merrill , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] c++: dependent constraint on placeholder return type [PR96443] In-Reply-To: <26f82e21-8fb-2b6-7389-541bddf55d8f@idea> Message-ID: <422b89c9-3d4d-3eab-745a-846afc7f2d4@idea> References: <20200804192234.2282332-1-ppalka@redhat.com> <6dec3a7d-d5c3-9a9e-9ed0-a886384ebfa4@redhat.com> <26f82e21-8fb-2b6-7389-541bddf55d8f@idea> 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.5 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, RCVD_IN_MSPIKE_H2, 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: Mon, 10 Aug 2020 18:51:56 -0000 On Thu, 6 Aug 2020, Patrick Palka wrote: > 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? Please ignore the previous message :) I had missed that the substitution failure should result in the constraint evaluating to false as part of satisfaction. Sorry for the noise. > > > > > > 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 "" } > > > } > > > > > > > >