Page MenuHomePhabricator

[C++20] Add consteval-specific semantic for functions
ClosedPublic

Authored by Tyker on Jun 28 2019, 3:24 PM.

Details

Summary

Changes:

  • Calls to consteval function and constructors are not evaluated as soon as they are reached.
  • Add diagnostic for taking address of a consteval function in non-constexpr context.
  • Add diagnostic for address of consteval function accessible at runtime.
  • Add tests

Serialization and importing depends on https://reviews.llvm.org/D63640

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
rsmith added inline comments.Aug 29 2019, 11:38 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
2413

Let's use the proper terminology here so people can search for it: "outside of an immediate invocation"

2414–2416

This diagnostic should be moved to DiagnosticASTKinds (and produced by CheckConstantExpression; see my other comment on this). Please rephrase this to follow the same pattern as note_constexpr_non_global.

2420

"consteval specified" -> "declared consteval"

4012–4013

consteval should not affect overload resolution. I don't think this diagnostic makes sense. (We should first pick the function and *then* check whether it's usable.)

clang/include/clang/Sema/Sema.h
2335

This should be called CheckImmediateInvocation or similar. ActOn* functions should only be called by the parser in response to source constructs, not internally by Sema.

clang/lib/Sema/SemaDeclCXX.cpp
2309

Please keep lines to 80 columns or fewer.

clang/lib/Sema/SemaExpr.cpp
5707

This is the wrong place to call ActOnConstevalCall from: that needs to happen when we actually build the resolved call expression (which isn't always done by this function).

5728–5729

!isConstantEvaluated() should be checked in the CXXConstructExpr case too; consider moving that check into ActOnConstevalCall.

5746–5751

I don't think this approach can work as-is. You need to know the complete bounds of the immediate invocation before you can check it. For example:

enum E {};
consteval int operator+(int (*f)(), E) { return f(); }
consteval int fn() { return 42; }
consteval auto get() -> int(*)() { return fn; }

int k = get() + E();

This is (or at least should be) valid code: the immediate invocation here is the entire get() + E() expression, and evaluating that produces 42. But this approach will reject the code, because get() in isolation is a constant expression. (The current wording suggests that this code is invalid, but I'm trying to get that fixed; I'm confident that was not the intent.)

I think we need a different approach. Perhaps:

  • when we see a candidate immediate invocation, immediately create a ConstantExpr wrapping it, but don't evaluate it yet
  • add that ConstantExpr to a list on the expression evaluation context record
  • if there already were any expressions listed there, check to see if any of them is a subexpression of the current expression, and if so, perform a TreeTransform to remove the ConstantExpr wrappers from those subexpressions and remove them from the list on the evaluation context record
  • when we pop an expression evaluation context record, *then* evaluate all remaining ConstantExprs for immediate invocations and check they're valid

(Or some other approach that handles this wrinkle; the above is just a suggestion.)

5759–5763

This should be checked when determining whether the core constant expression is a constant expression, which happens in CheckConstantExpression in ExprConstant.cpp, not here.

5845–5846

What's the purpose of this change?

clang/lib/Sema/SemaOverload.cpp
9961–9972

This isn't the right way to deal with this, in part because we don't know whether we're in an immediate invocation or not at this point. Instead, we should track that we saw a use of a consteval function from Sema::DiagnoseUseOfDecl, and issue a diagnostic if that use is not within an immediate invocation / within a consteval function.

Tyker marked 2 inline comments as done.Aug 31 2019, 11:47 AM

sorry i didn't realize the full complexity of immediate invocations.
i am working on a patch fixing issues.

clang/lib/Sema/SemaExpr.cpp
5845–5846

this change was to prevent a double error message for code like:

consteval f() { return 0; }
auto* p = __builtin_addressof(f);

it is going to disappear because the error reporting locations are going to move.

clang/lib/Sema/SemaOverload.cpp
9961–9972

from what i understood Sema::DiagnoseUseOfDecl is called as soon as the Decl is used. but there is some cases similar to the one you gave in an other comment where we can't know yet if the use is during an immediate invokation. like the following

enum E {};
consteval int operator+(int (*f)(), E) { return f(); }
consteval int fn() { return 42; }

int k = &fn + E();

this should be valid i think. but at the point of call of Sema::DiagnoseUseOfDecl for the use of fn we don't know yet that we are in an immediate invokation. so i think this check should be delayed until we know the bounds of the immediate invokation.

Tyker updated this revision to Diff 219205.EditedSep 6 2019, 5:11 PM
Tyker retitled this revision from [C++20] Add consteval-specifique semantic to [C++20] Add consteval-specifique semantic for functions.

I narrowed the patch because it was getting quite big. this patch only handle consteval function, not constructors.

Changes:

  • keep track of DeclRefExpr on consteval decaltions.
  • keep track of candidates for immediate invocation.
  • detect and remove nested immediate invocation candidates
  • detect and remove DeclRefExpr on consteval declarations inside immediate invocations.
  • diagnose non removed DeclRefExpr on consteval declarations and candidates for immediate invocation.
  • fixe existing bug where defaulted special members declared consteval where marked constexpr instead.
  • improve tests.
Tyker marked 12 inline comments as done.Sep 6 2019, 5:12 PM
Tyker updated this revision to Diff 219240.Sep 7 2019, 7:36 AM

Changes:

  • Rebased
  • Fixed typos
Tyker retitled this revision from [C++20] Add consteval-specifique semantic for functions to [C++20] Add consteval-specific semantic for functions.Sep 7 2019, 7:36 AM

Very cool, this is an elegant approach.

clang/include/clang/Basic/DiagnosticASTKinds.td
58

on -> to

clang/include/clang/Basic/DiagnosticSemaKinds.td
2418

Please add 's around the function name. Instead of encoding the name as an integer, you can stream the declaration itself into the diagnostic and just use "%0 cannot be declared consteval" here.

clang/include/clang/Sema/Sema.h
840–842

The comment should first describe what this field means, before saying how to use it (if necessary). So:

/// Whether immediate invocation candidates and references to consteval functions should be tracked.

or something like that.

That said, consider renaming this to RebuildingImmediateInvocation to avoid suggesting it's a general mechanism to turn off immediate invocation tracking.

1101

ImmediateInvocationsCandidates -> ImmediateInvocationCandidates

1105

ReferenceOnConsteval -> ReferencesToConsteval or similar.

clang/lib/AST/ExprConstant.cpp
2011

It would be useful for the diagnostic to say what consteval function we have a pointer to, and maybe include a note pointing at its declaration.

clang/lib/Sema/SemaDeclCXX.cpp
12634–12635

I don't think this is necessary: implicit special members are never consteval. (We use DeclareImplicitDefaultConstructor when there is no user-declared default constructor, in which case there can't possibly be a defaulted consteval default constructor.) I don't think you need any of the DetermineSpecialMemberConstexprKind machinery, nor the tracking of DefaultedSpecialMemberIsConsteval in CXXRecordDecl.

clang/lib/Sema/SemaExpr.cpp
15264

Consider using E.get()->IgnoreImplicit() here; we want to step over (at least) any CXXBindTemporaryExprs inserted by MaybeBindToTemporary.

15264–15267

Please add a comment to this to point out that it's just an optimization, for example:

"Opportunistically remove the callee from ReferencesToConsteval if we can. It's OK if this fails; we'll also remove this in HandleImmediateInvocations, but catching it here allows us to avoid walking the AST looking for it in simple cases."

15268–15271

We will need an ExprWithCleanups wrapping E.get() if there might be any temporaries within it.

We don't actually know whether there definitely are any such temporaries, but we can conservatively assume we might need an ExprWithCleanups if Cleanup.exprNeedsCleanups() returns true.

You can test this with something like:

struct A { int *p = new int(42); consteval int get() { return *p; } constexpr ~A() { delete p; } };
int k = A().get();

... which should be valid, but will be rejected (or might assert) if we don't destroy the A() temporary as part of evaluating the consteval call to get.

15303

< on SourceLocation doesn't really mean anything. It's not correct to use it here. Also, using source locations will not give you outer expressions before inner ones. Consider:

enum E {};
consteval E f();
consteval int operator+(E, int);
void g() {
  f() + 1;
}

Here, the begin location of both the call to f() and the call to operator+ are in the same place: at the f token.

I don't think you need this sort at all: instead, you can process immediate invocations in ImmediateInvocationCandidates in reverse order: subexpressions must have been built before their enclosing expressions, so walking in reverse order will guarantee that you process an outer candidate before any inner ones.

15334

We should look for an exact match here, not merely for something nearby. (Maybe: form a pointer set of immediate invocation pointer expressions, and remove things from the set when you encounter them in this walk; then when iterating over the immediate invocations, skip ones that have already been erased from the set.)

15350–15351

I don't imagine this will ever actually happen, because immutability of the AST is so important to other parts of Clang. This should be a rare case, so it likely doesn't matter; I'd just remove this FIXME.

Tyker updated this revision to Diff 223021.EditedOct 3 2019, 8:01 AM
Tyker marked 16 inline comments as done.

almost all comments were addressed.

clang/lib/Sema/SemaDeclCXX.cpp
12634–12635

all the code you mention is to fixe an issue i saw while working on consteval.
the AST of

struct A {
    consteval A() = default;
    consteval A(int) { }
};

is

TranslationUnitDecl
`-CXXRecordDecl <line:1:1, line:4:1> line:1:8 struct A definition
  |-DefinitionData pass_in_registers empty standard_layout trivially_copyable trivial literal has_user_declared_ctor has_constexpr_non_copy_move_ctor can_const_default_init
  | |-DefaultConstructor exists trivial constexpr defaulted_is_constexpr
  | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
  | |-MoveConstructor exists simple trivial needs_implicit
  | |-CopyAssignment trivial has_const_param needs_implicit implicit_has_const_param
  | |-MoveAssignment exists simple trivial needs_implicit
  | `-Destructor simple irrelevant trivial needs_implicit
  |-CXXRecordDecl <col:1, col:8> col:8 implicit referenced struct A
  |-CXXConstructorDecl <line:2:5, col:27> col:15 constexpr A 'void ()' default trivial noexcept-unevaluated 0x55585ae25160
  `-CXXConstructorDecl <line:3:5, col:24> col:15 consteval A 'void (int)'
    |-ParmVarDecl <col:17> col:20 'int'
    `-CompoundStmt <col:22, col:24>

godbolt
notice that A::A() is marked as constexpr instead of consteval.
and A::A(int) is marked correctly.

this is because consteval defaulted special members are not marked as consteval.
those code changes are intended to fix that. with this patch both constructor are marked as consteval

clang/lib/Sema/SemaExpr.cpp
15303

the sort is used in combination with a lower_bounds in ComplexRemove::TransformConstantExpr to lookup in ImmediateInvocationsCandidates using binary search.
but i think your proposition is more efficient except in some very rare cases.

15334

the issue i had with a set is that we can't remove element during iteration as removing element invalidate iterators.
but to get an exact match we can use the pointer as "key", they are unique.

Tyker updated this revision to Diff 223624.Oct 7 2019, 10:03 AM
Tyker updated this revision to Diff 224588.Oct 11 2019, 7:09 AM

improve performance in a bad case

Tyker added a comment.Oct 17 2019, 1:12 AM

The now that constexpr destructors are legal. The code in this patch need to be adapted, I have question about the following code.

struct A {
  constexpr ~A() {}
};

consteval A f() {
    return A{};
}

void test() {
    A a;
    a = f(); // <-- here
}

At the point i marked.
The invocation of f causes an immediate invocation (http://eel.is/c++draft/expr.const#12).
Immediate invocation are full expression (http://eel.is/c++draft/intro.execution#5).
Full expression resolve all there side-effects before evaluating the next full expression (http://eel.is/c++draft/intro.execution#9).
The return value of f() is created inside the immediate invocation.
So the destructor of the value returned by f() should be destroyed within the immediate evaluation.
But that value is needed for the assignment operator.
This seem contradictory. What have i misunderstood ? What should happen here ?

The now that constexpr destructors are legal. The code in this patch need to be adapted, I have question about the following code.

struct A {
  constexpr ~A() {}
};

consteval A f() {
    return A{};
}

void test() {
    A a;
    a = f(); // <-- here
}

At the point i marked.
The invocation of f causes an immediate invocation (http://eel.is/c++draft/expr.const#12).
Immediate invocation are full expression (http://eel.is/c++draft/intro.execution#5).
Full expression resolve all there side-effects before evaluating the next full expression (http://eel.is/c++draft/intro.execution#9).
The return value of f() is created inside the immediate invocation.
So the destructor of the value returned by f() should be destroyed within the immediate evaluation.
But that value is needed for the assignment operator.
This seem contradictory. What have i misunderstood ? What should happen here ?

After desugaring, the above example looks like this:

a.operator=( <temporary materialization conversion>( <consteval full-expression>( f() ) ) );

That is: the temporary is materialized outside the immediate evaluation, so no A destructor should be run as part of evaluating that full-expression (the destructor is run in the enclosing full-expression). If the example were instead:

constexpr A g() {
  return A {};
}
consteval A &id (A &&a) { return a; }

void test() {
  A a;
  a = id(g());
}

then we would have

a.operator=( <consteval full-expression>( id( <temporary materialization conversion>( g() ) ) ) );

... and now the A temporary should be destroyed inside the full-expression. This example is ill-formed because the id call is not a constant expression (because its evaluated value refers to an object with automatic storage duration).

Tyker updated this revision to Diff 227909.Nov 5 2019, 10:06 AM

@rsmith

Changes:

  • Rebased on recent master.
  • Adapted this patch to constexpr destructors.
  • Fixed issues with handling of temporaries.
  • Improve Tests.
Tyker updated this revision to Diff 228197.Nov 7 2019, 2:34 AM

minor fixes
improved tests

rsmith added inline comments.Nov 26 2019, 2:30 PM
clang/include/clang/AST/Expr.h
1058–1060

I'd remove the "CausedBy" here -- the ConstantExpr is our representation of the immediate invocation.

Also, please start this function name with a lowercase i for local consistency.

clang/include/clang/Sema/Sema.h
1056

II doesn't mean anything to a reader here; please expand the abbreviation.

5539–5540

potention -> potential
Missing period at end of sentence.

clang/lib/AST/ExprConstant.cpp
2008–2009

Please add braces to these if statements (their controlled statement is too long to omit the braces). As a general rule: if an inner construct needs braces, put braces on the outer constructs too.

13863

This isn't sufficient: the evaluation process can refer back to the object under construction (eg, via this), and we need an lvalue that actually names the result in order for this to work.

I think you'll need to do something like creating a suitable object (perhaps a LifetimeExtendedTemporaryDecl) in the caller to represent the result of the immediate invocation, and passing that into here.

clang/lib/Sema/Sema.cpp
164 ↗(On Diff #228197)

Consider using a default member initializer instead.

clang/lib/Sema/SemaDecl.cpp
8955–8956

Please add braces here.

8956

This rule also applies to non-member allocator functions. (Don't check for a CXXMethodDecl here.)

clang/lib/Sema/SemaDeclCXX.cpp
12634–12635

This change is incorrect: an implicit special member is never consteval. (Note that implicit special members are the ones the compiler declares for you, not the ones you get from = default.) We don't need this, and as a consequence we don't need DefaultedSpecialMemberIsConsteval on DefinitionData.

clang/lib/Sema/SemaExpr.cpp
15318

This is unreachable; a CXXMemberCallExpr is a CallExpr so this was already handled above. Can you just delete this case?

15323

Typo unhandeld -> unhandled

15374

Transfromer -> Transformer

clang/lib/Sema/TreeTransform.h
3451–3452 ↗(On Diff #228197)

I'm surprised that we can remove this; I'd expect this to be necessary to properly handle transforming cases such as

struct X { X(int); ~X(); int n; };
struct Y { explicit Y(int); };
template<typename T> void f() {
  Y y(X(sizeof(sizeof(T))).n);
}
template void f<int>();

... where we need to strip off an ExprWithCleanups to find the syntactic initializer inside it.

If you want to special-case this from ComplexRemove, to make sure you see all ConstantExprs, can you override TransformInitializer there instead of changing it globally?

Tyker updated this revision to Diff 232450.Dec 5 2019, 2:51 PM
Tyker marked 14 inline comments as done.

comments were fixed.

clang/lib/AST/ExprConstant.cpp
13863

i believe this solution should work. without LifetimeExtendedTemporaryDecl because reference/pointer on temporaries are not valid results of constant evaluation. so the AST should never store an APValue whose LValue is a ConstantExpr.

Tyker marked an inline comment as done.Dec 5 2019, 2:53 PM

@rsmith friendly reminder

Tyker added a comment.Feb 3 2020, 2:47 PM

@aaron.ballman do you think you can review this ?

rsmith accepted this revision.Feb 3 2020, 6:16 PM

Some nits and very minor things, but I think this is generally good to go with those fixed. Sorry for the long review delay, and thanks for working on it!

clang/include/clang/Basic/DiagnosticSemaKinds.td
2416

could not be evaluated -> is not a constant expression

clang/include/clang/Sema/Sema.h
840

rebuild -> rebuilt

1104

candidate -> candidates

1107

DeclRefExpr -> DeclRefExprs

1108

know -> known

clang/lib/AST/ExprConstant.cpp
13863

OK, I think that should be fine. Please update the comment above class LValueExprEvaluator to describe this additional case.

clang/lib/Sema/SemaDecl.cpp
8937

In C++20, we should probably reset to constexpr rather than unspecified. Before C++20, it's better for error recovery to leave the function as constexpr. (So unconditionally using CSK_constexpr here would be OK.)

clang/lib/Sema/SemaExpr.cpp
15325

You can use `"%q0" to get a qualified name in a diagnostic; you don't need to turn it into a string yourself.

This revision is now accepted and ready to land.Feb 3 2020, 6:16 PM
Tyker marked 8 inline comments as done.Feb 4 2020, 11:39 AM

thank you for the review.
i am sorry there were so many back and forth.

This revision was automatically updated to reflect the committed changes.