[v3,6/6] c++/modules: Diagnose TU-local lambdas, give mangling scope to lambdas in concepts
Checks
Context |
Check |
Description |
linaro-tcwg-bot/tcwg_gcc_build--master-arm |
success
|
Build passed
|
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 |
success
|
Build passed
|
linaro-tcwg-bot/tcwg_gcc_check--master-arm |
success
|
Test passed
|
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 |
success
|
Test passed
|
Commit Message
Happy to defer this till GCC16 if preferred.
-- >8 --
This fills in a hole left in r15-6378-g9016c5ac94c557 with regards to
detection of TU-local lambdas. Now that LAMBDA_EXPR_EXTRA_SCOPE is
properly set for most lambdas we can use it to detect lambdas that are
TU-local.
Lambdas in concept definitions I believe should not be considered
TU-local, since they are always unevaluated and should never be emitted.
This patch gives these lambdas a mangling scope (though it will never be
actually used in name mangling).
Namespace-scope alias declarations currently do not have an extra scope
set either, so this patch will cause them to be falsely detected as
TU-local and error. However, I believe this is better than the
alternative, as such lambdas cannot be properly referred to from other
TUs now anyway causing strange issues down the line if they were
allowed. (Clang makes such lambdas internal linkage.)
If in the future we ever correctly give an extra scope to these lambdas
they will automatically start working in modules again, but in the
meantime they should be avoided (or wrapped in an unnamed namespace).
gcc/cp/ChangeLog:
* cp-tree.h (finish_concept_definition): Adjust parameters.
(start_concept_definition): Declare.
* module.cc (depset::hash::is_tu_local_entity): Use
LAMBDA_EXPR_EXTRA_SCOPE to detect TU-local lambdas.
* parser.cc (cp_parser_concept_definition): Start a lambda scope
for concept definitions.
* pt.cc (tsubst_lambda_expr): Namespace-scope lambdas may now
have extra scope.
(finish_concept_definition): Split into...
(start_concept_definition): ...this new function.
gcc/testsuite/ChangeLog:
* g++.dg/modules/internal-4_b.C: Remove XFAILs, add new XFAIL
for lambda alias.
* g++.dg/modules/lambda-9.h: New test.
* g++.dg/modules/lambda-9_a.H: New test.
* g++.dg/modules/lambda-9_b.C: New test.
Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
gcc/cp/cp-tree.h | 3 ++-
gcc/cp/module.cc | 7 ++++---
gcc/cp/parser.cc | 14 +++++++++++++-
gcc/cp/pt.cc | 21 +++++++++++++++------
gcc/testsuite/g++.dg/modules/internal-4_b.C | 6 +++++-
gcc/testsuite/g++.dg/modules/lambda-9.h | 2 ++
gcc/testsuite/g++.dg/modules/lambda-9_a.H | 4 ++++
gcc/testsuite/g++.dg/modules/lambda-9_b.C | 6 ++++++
8 files changed, 51 insertions(+), 12 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/modules/lambda-9.h
create mode 100644 gcc/testsuite/g++.dg/modules/lambda-9_a.H
create mode 100644 gcc/testsuite/g++.dg/modules/lambda-9_b.C
Comments
On 1/6/25 7:24 AM, Nathaniel Shead wrote:
> Happy to defer this till GCC16 if preferred.
>
> -- >8 --
>
> This fills in a hole left in r15-6378-g9016c5ac94c557 with regards to
> detection of TU-local lambdas. Now that LAMBDA_EXPR_EXTRA_SCOPE is
> properly set for most lambdas we can use it to detect lambdas that are
> TU-local.
>
> Lambdas in concept definitions I believe should not be considered
> TU-local, since they are always unevaluated and should never be emitted.
It seems to me that they are also TU-local under
https://eel.is/c++draft/basic#link-15.2 . It might be feasible to
change that, but that is the status quo.
Jason
On Thu, Jan 23, 2025 at 04:47:32PM -0500, Jason Merrill wrote:
> On 1/6/25 7:24 AM, Nathaniel Shead wrote:
> > Happy to defer this till GCC16 if preferred.
> >
> > -- >8 --
> >
> > This fills in a hole left in r15-6378-g9016c5ac94c557 with regards to
> > detection of TU-local lambdas. Now that LAMBDA_EXPR_EXTRA_SCOPE is
> > properly set for most lambdas we can use it to detect lambdas that are
> > TU-local.
> >
> > Lambdas in concept definitions I believe should not be considered
> > TU-local, since they are always unevaluated and should never be emitted.
>
> It seems to me that they are also TU-local under
> https://eel.is/c++draft/basic#link-15.2 . It might be feasible to change
> that, but that is the status quo.
>
> Jason
>
Right, sorry I wasn't clear; I agree that this is the status quo, I just
think that we should accept them; otherwise, currently we would reject
exporting any concept with a lambda contained within it, which seems
needless hostile (and causes a number of tests to fail).
In particular, lambdas within concepts are actually made use of within
the standard library, e.g. optional:1683, so we would either need to
continue to allow this to work somehow (either via mangling scope or a
new flag on the lambda, because there's no other way I was able to find
of determining this), or rewrite the concept to not use a lambda.
// _GLIBCXX_RESOLVE_LIB_DEFECTS
// 3746. optional's spaceship with U with a type derived from optional
// causes infinite constraint meta-recursion
template<typename _Tp>
concept __is_derived_from_optional = requires (const _Tp& __t) {
[]<typename _Up>(const optional<_Up>&){ }(__t);
};
Nathaniel
On 1/23/25 5:11 PM, Nathaniel Shead wrote:
> On Thu, Jan 23, 2025 at 04:47:32PM -0500, Jason Merrill wrote:
>> On 1/6/25 7:24 AM, Nathaniel Shead wrote:
>>> Happy to defer this till GCC16 if preferred.
>>>
>>> -- >8 --
>>>
>>> This fills in a hole left in r15-6378-g9016c5ac94c557 with regards to
>>> detection of TU-local lambdas. Now that LAMBDA_EXPR_EXTRA_SCOPE is
>>> properly set for most lambdas we can use it to detect lambdas that are
>>> TU-local.
>>>
>>> Lambdas in concept definitions I believe should not be considered
>>> TU-local, since they are always unevaluated and should never be emitted.
>>
>> It seems to me that they are also TU-local under
>> https://eel.is/c++draft/basic#link-15.2 . It might be feasible to change
>> that, but that is the status quo.
>
> Right, sorry I wasn't clear; I agree that this is the status quo, I just
> think that we should accept them; otherwise, currently we would reject
> exporting any concept with a lambda contained within it, which seems
> needless hostile (and causes a number of tests to fail).
>
> In particular, lambdas within concepts are actually made use of within
> the standard library, e.g. optional:1683, so we would either need to
> continue to allow this to work somehow (either via mangling scope or a
> new flag on the lambda, because there's no other way I was able to find
> of determining this), or rewrite the concept to not use a lambda.
>
> // _GLIBCXX_RESOLVE_LIB_DEFECTS
> // 3746. optional's spaceship with U with a type derived from optional
> // causes infinite constraint meta-recursion
> template<typename _Tp>
> concept __is_derived_from_optional = requires (const _Tp& __t) {
> []<typename _Up>(const optional<_Up>&){ }(__t);
> };
Agreed, I've sent mail to CWG proposing adding concept-definition to the
list in basic.link/15.2. Thanks for the example.
Jason
@@ -8679,7 +8679,8 @@ struct diagnosing_failed_constraint
extern cp_expr finish_constraint_or_expr (location_t, cp_expr, cp_expr);
extern cp_expr finish_constraint_and_expr (location_t, cp_expr, cp_expr);
extern cp_expr finish_constraint_primary_expr (cp_expr);
-extern tree finish_concept_definition (cp_expr, tree, tree);
+extern tree start_concept_definition (cp_expr);
+extern tree finish_concept_definition (tree, tree, tree);
extern tree combine_constraint_expressions (tree, tree);
extern tree append_constraint (tree, tree);
extern tree get_constraints (const_tree);
@@ -13365,9 +13365,10 @@ depset::hash::is_tu_local_entity (tree decl, bool explain/*=false*/)
tree main_decl = TYPE_MAIN_DECL (type);
if (!DECL_CLASS_SCOPE_P (main_decl)
&& !decl_function_context (main_decl)
- /* FIXME: Lambdas defined outside initializers. We'll need to more
- thoroughly set LAMBDA_TYPE_EXTRA_SCOPE to check this. */
- && !LAMBDA_TYPE_P (type))
+ /* LAMBDA_EXPR_EXTRA_SCOPE will be set for lambdas defined in
+ contexts where they would not be TU-local. */
+ && !(LAMBDA_TYPE_P (type)
+ && LAMBDA_TYPE_EXTRA_SCOPE (type)))
{
if (explain)
inform (loc, "%qT has no name and is not defined within a class, "
@@ -31680,8 +31680,20 @@ cp_parser_concept_definition (cp_parser *parser)
return error_mark_node;
}
+ tree decl = start_concept_definition (id);
+ if (decl == error_mark_node)
+ {
+ cp_parser_skip_to_end_of_statement (parser);
+ cp_parser_consume_semicolon_at_end_of_statement (parser);
+ return error_mark_node;
+ }
+
processing_constraint_expression_sentinel parsing_constraint;
+
+ start_lambda_scope (decl);
tree init = cp_parser_constraint_expression (parser);
+ finish_lambda_scope ();
+
if (init == error_mark_node)
cp_parser_skip_to_end_of_statement (parser);
@@ -31689,7 +31701,7 @@ cp_parser_concept_definition (cp_parser *parser)
but continue as if it were. */
cp_parser_consume_semicolon_at_end_of_statement (parser);
- return finish_concept_definition (id, init, attrs);
+ return finish_concept_definition (decl, init, attrs);
}
// -------------------------------------------------------------------------- //
@@ -20098,7 +20098,7 @@ tsubst_lambda_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl)
if (LAMBDA_EXPR_EXTRA_SCOPE (t))
record_lambda_scope (r);
- else if (TYPE_NAMESPACE_SCOPE_P (TREE_TYPE (t)))
+ if (TYPE_NAMESPACE_SCOPE_P (TREE_TYPE (t)))
/* If we're pushed into another scope (PR105652), fix it. */
TYPE_CONTEXT (type) = DECL_CONTEXT (TYPE_NAME (type))
= TYPE_CONTEXT (TREE_TYPE (t));
@@ -29886,12 +29886,10 @@ placeholder_type_constraint_dependent_p (tree t)
return false;
}
-/* Build and return a concept definition. Like other templates, the
- CONCEPT_DECL node is wrapped by a TEMPLATE_DECL. This returns the
- the TEMPLATE_DECL. */
+/* Prepare and return a concept definition. */
tree
-finish_concept_definition (cp_expr id, tree init, tree attrs)
+start_concept_definition (cp_expr id)
{
gcc_assert (identifier_p (id));
gcc_assert (processing_template_decl);
@@ -29923,9 +29921,20 @@ finish_concept_definition (cp_expr id, tree init, tree attrs)
/* Initially build the concept declaration; its type is bool. */
tree decl = build_lang_decl_loc (loc, CONCEPT_DECL, *id, boolean_type_node);
DECL_CONTEXT (decl) = current_scope ();
- DECL_INITIAL (decl) = init;
TREE_PUBLIC (decl) = true;
+ return decl;
+}
+
+/* Finish building a concept definition. Like other templates, the
+ CONCEPT_DECL node is wrapped by a TEMPLATE_DECL. This returns the
+ the TEMPLATE_DECL. */
+
+tree
+finish_concept_definition (tree decl, tree init, tree attrs)
+{
+ DECL_INITIAL (decl) = init;
+
if (attrs)
cplus_decl_attributes (&decl, attrs, 0);
@@ -80,7 +80,7 @@ void in_function_body() { struct {} x; } // OK
auto in_initializer = []{}; // OK
#if __cplusplus >= 202002L
-decltype([]{}) d_lambda; // { dg-error "exposes TU-local entity" "" { xfail *-*-* } }
+decltype([]{}) d_lambda; // { dg-error "exposes TU-local entity" "" { target c++20 } }
template <typename T>
concept in_constraint_expression = requires {
@@ -89,6 +89,10 @@ concept in_constraint_expression = requires {
// but I don't think that is intended.
[]{}; // { dg-bogus "exposes TU-local entity" }
};
+
+// Again, by the standard this should probably be legal, but currently
+// we cannot properly mangle lambdas in namespace-scope aliases.
+using alias_lambda = decltype([]{}); // { dg-bogus "exposes TU-local entity" "" { xfail c++20 } }
#endif
// (But consider unnamed types with names for linkage purposes as having names)
new file mode 100644
@@ -0,0 +1,2 @@
+template <typename T>
+concept C = requires { []{}; };
new file mode 100644
@@ -0,0 +1,4 @@
+// { dg-additional-options "-fmodule-header -std=c++20" }
+// { dg-module-cmi {} }
+
+#include "lambda-9.h"
new file mode 100644
@@ -0,0 +1,6 @@
+// { dg-additional-options "-fmodules -std=c++20 -fno-module-lazy" }
+
+#include "lambda-9.h"
+import "lambda-9_a.H";
+
+static_assert(C<int>);