Page MenuHomePhabricator

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

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

Tyker created this revision.Jun 28 2019, 3:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 28 2019, 3:24 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Tyker updated this revision to Diff 217615.Aug 28 2019, 5:44 AM

Rebased

@rsmith Ping

rsmith added inline comments.Aug 29 2019, 11:38 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
2361

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

2362–2364

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.

2368

"consteval specified" -> "declared consteval"

3852–3853

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
2200

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
2302

Please keep lines to 80 columns or fewer.

clang/lib/Sema/SemaExpr.cpp
5561

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).

5582–5583

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

5600–5605

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.)

5613–5617

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

5699–5700

What's the purpose of this change?

clang/lib/Sema/SemaOverload.cpp
9656–9667

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
5699–5700

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
9656–9667

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
2366

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
807–809

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.

1066

ImmediateInvocationsCandidates -> ImmediateInvocationCandidates

1070

ReferenceOnConsteval -> ReferencesToConsteval or similar.

clang/lib/AST/ExprConstant.cpp
2012

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
11264–11265

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
15099

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

15099–15102

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."

15103–15106

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.

15138

< 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.

15169

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.)

15185–15186

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.EditedThu, Oct 3, 8:01 AM
Tyker marked 16 inline comments as done.

almost all comments were addressed.

clang/lib/Sema/SemaDeclCXX.cpp
11264–11265

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
15138

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.

15169

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.Mon, Oct 7, 10:03 AM
Tyker updated this revision to Diff 224588.Fri, Oct 11, 7:09 AM

improve performance in a bad case

Tyker added a comment.Thu, Oct 17, 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 ?