Enable AST mutation in the constant evaluator

[Endilll]

No description provided.

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 05b6c2e4b933e7a3606899c72067c92b6077287b 0b2d92e0fed41ee2428c3ef8b8369790a1279a21 --extensions h,cpp -- clang/test/SemaCXX/constexpr-function-instantiation.cpp clang/include/clang/AST/ASTContext.h clang/include/clang/Sema/Sema.h clang/lib/AST/ASTContext.cpp clang/lib/AST/ByteCode/ByteCodeEmitter.cpp clang/lib/AST/ByteCode/ByteCodeEmitter.h clang/lib/AST/ByteCode/Compiler.cpp clang/lib/AST/ByteCode/Compiler.h clang/lib/AST/ByteCode/Context.cpp clang/lib/AST/ByteCode/Context.h clang/lib/AST/ExprConstant.cpp clang/lib/Sema/Sema.cpp

View the diff from clang-format here.

diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index c02f3b5389..5aa025fdc3 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -904,8 +904,8 @@ ASTContext::ASTContext(LangOptions &LOpts, SourceManager &SM,
                                         LangOpts.XRayAttrListFiles, SM)),
       ProfList(new ProfileList(LangOpts.ProfileListFiles, SM)),
       PrintingPolicy(LOpts),
-      SemaProxyPtr(std::make_unique<UnimplementedSemaProxy>()),
-      Idents(idents), Selectors(sels), BuiltinInfo(builtins), TUKind(TUKind),
+      SemaProxyPtr(std::make_unique<UnimplementedSemaProxy>()), Idents(idents),
+      Selectors(sels), BuiltinInfo(builtins), TUKind(TUKind),
       DeclarationNames(*this), Comments(SM),
       CommentCommandTraits(BumpAlloc, LOpts.CommentOpts),
       CompCategories(this_()), LastSDM(nullptr, 0) {

[cor3ntin]

@zygoloid @katzdm for awareness

[Endilll]

@zygoloid @katzdm for awareness

This PR is still in flux. Discussions are lengthy and typically happen in meetings.
My plan is to get to a stable point, write down how we got there, alternatives rejected (with reasoning), and get the PR out of the draft status. This would be a good point to engage for people who are interested, but not interested enough to attend a lot of meetings.

[Endilll]

I added Sema callback into the new interpreter in 0b2d92e. I think it went smoothly, save for the fact that source locations are not always readily available there, but they are need to correctly emit diagnostics from Sema.

I invite @tbaederr to provide early feedback on my changes to the new constant interpreter.

[zygoloid]

If we do end up needing this (and it's looking increasingly likely that we will), I think the general approach of having a set of callbacks that gets passed into the constant evaluator is the right approach.

I think the older approach in this PR (a callback object that is explicitly passed to each AST operation that needs it) is significantly preferable to the newer approach (caching the callback in the ASTContext). The reason is that I think it's very important that we have a bright line visible at every call to the evaluator distinguishing

• incidental evaluations that we perform during various checking / diagnostic / code generation stages and that absolutely must not trigger AST mutations or any kind of visible behavior and
• the special "the language rules say this is manifestly constant evaluated" cases that should be able to perform AST mutations, that we need to be extremely careful to invoke at exactly the right times and in exactly the right cases and to invoke only once

If we're going to allow AST mutation from constant evaluation, we need very strong guard rails around it to make sure it doesn't happen accidentally. We're already passing in information to the evaluator for this distinction (to determine the value of __builtin_is_constant_evaluated), so switching to passing a pointer instead of an enumerator seems reasonable, and constructively guarantees we don't get this wrong inside the implementation of the evaluator.

[Endilll]

@zygoloid I definitely don't disagree with your points. After extensive discussions with other maintainers (Shafik, Corentin, Erich, Aaron), I went from passing the callback explicitly to very implicit approach, because of reasons that lie outside of usage of AST in Clang itself, and revolve around tools like clang-tidy:

1. Asking users to pass additional parameter to every Eval*() function makes a bad transition story for users that wish to upgrade to a version of Clang that has changed the interface in this way.
2. The transition story becomes even worse when we consider the fact that Sema is the only reasonable implementation of this callback, whereas clang-tidy doesn't typically have access to Sema. So we're asking users to pass an implementation of interface they have barely any chance to implement.
3. The transition story could be improved by specifying a default for callback parameter. It eases the transition, but completely murders the intent that enabling AST mutation during constant evaluation has to be visible.

So the approach we took is like (3) above, but maxing out on hiding this new callback from the API. If constant evaluation is triggered without Sema available, nothing changes. If Sema is created, then you get full C++26 constant evaluation capabilities, including AST mutation. This also means that future additions to constant evaluator that need AST mutation cannot blindly assume that AST mutation is available.

We also identified that issues like #73232 can leverage this new callback. To my understanding, this was one of the reasons why idea to limit calls to callback to C++26 and newer language modes didn't have traction among maintainers.

I think there's also understanding among maintainers that this PR is strictly worse that status quo whichever approach is taken. No one, including me, enjoys storing a callback in effectively a global variable (ASTContext is available almost everywhere). As long as P2996 comes with sides effects in constant evaluation, we're just picking our poison.

CC @AaronBallman in case I forgot something or misrepresented your opinion on situation about external users of Clang.

[Endilll]

the special "the language rules say this is manifestly constant evaluated" cases that should be able to perform AST mutations, that we need to be extremely careful to invoke at exactly the right times and in exactly the right cases and to invoke only once

Can you expand on the "to invoke only once" bit? As I understand the position of authors of P2996, calls to define_aggregate with the same arguments are supposed to be idempotent. Do you see reasons why we wouldn't be able to achieve that, meaning that we need to prevent define_aggregate to be called multiple times (with the same arguments) on the language level?

[zygoloid]

1. Asking users to pass additional parameter to every Eval*() function makes a bad transition story for users that wish to upgrade to a version of Clang that has changed the interface in this way.

External callers of the Eval* functions in almost all cases should not be passing in this extra information -- existing callers of evaluation functions do not expect those functions to mutate the AST. Making the default behavior be a "pure" evaluation with a guarantee of no side effects seems like it's going to be the right default in almost all cases. We absolutely don't want existing users of Clang's API to suddenly start triggering AST mutation in calls that previously were performing pure evaluation of an expression for its value.

2. The transition story becomes even worse when we consider the fact that Sema is the only reasonable implementation of this callback, whereas clang-tidy doesn't typically have access to Sema. So we're asking users to pass an implementation of interface they have barely any chance to implement.

I don't think so, because callers that don't have access to Sema should never be doing an evaluation that performs AST mutation.

3. The transition story could be improved by specifying a default for callback parameter. It eases the transition, but completely murders the intent that enabling AST mutation during constant evaluation has to be visible.

I'm not following the argument here. If the default is that you don't get AST mutation, then it seems to me that you would need to explicitly and visibly opt into getting AST mutation by passing the callbacks to the evaluator.

[zygoloid]

the special "the language rules say this is manifestly constant evaluated" cases that should be able to perform AST mutations, that we need to be extremely careful to invoke at exactly the right times and in exactly the right cases and to invoke only once

Can you expand on the "to invoke only once" bit? As I understand the position of authors of P2996, calls to define_aggregate with the same arguments are supposed to be idempotent. Do you see reasons why we wouldn't be able to achieve that, meaning that we need to prevent define_aggregate to be called multiple times (with the same arguments) on the language level?

I think it's short-sighted to imagine that will work for the breadth of compile-time constructs we'll end up providing. People have already asked for things like a consteval function that produces a warning or an error -- is that also going to be idempotent? What about consteval facilities that generate random numbers, or produce a log file on the build machine? What about a consteval utility that generates a new anonymous global? What do you do if you want each call to your "make a new type" facility to guarantee to produce a new, distinct type?

Maybe we can say no to some of those things, but I don't think we'll be able to say no to all of them forever. It seems more forward-looking to me to instead guarantee that we will evaluate exactly the things we're required to evaluate in this new mode, exactly once each.

[Endilll]

the special "the language rules say this is manifestly constant evaluated" cases that should be able to perform AST mutations, that we need to be extremely careful to invoke at exactly the right times and in exactly the right cases and to invoke only once

Can you expand on the "to invoke only once" bit? As I understand the position of authors of P2996, calls to define_aggregate with the same arguments are supposed to be idempotent. Do you see reasons why we wouldn't be able to achieve that, meaning that we need to prevent define_aggregate to be called multiple times (with the same arguments) on the language level?

I think it's short-sighted to imagine that will work for the breadth of compile-time constructs we'll end up providing. People have already asked for things like a consteval function that produces a warning or an error -- is that also going to be idempotent? What about consteval facilities that generate random numbers, or produce a log file on the build machine? What about a consteval utility that generates a new anonymous global? What do you do if you want each call to your "make a new type" facility to guarantee to produce a new, distinct type?

Maybe we can say no to some of those things, but I don't think we'll be able to say no to all of them forever. It seems more forward-looking to me to instead guarantee that we will evaluate exactly the things we're required to evaluate in this new mode, exactly once each.

Consider the following example:

struct S;
consteval int f() {
  define_aggregate(^^S, {}); // we say this function is not idempotent,
                             // so we want guarantees that it can't be evaluated more than once
  write_char_to_file("output.txt", 'a'); // presumably we want this to evaluated as many times as we get here
  return 0;
}

constexpr int a = f();
constexpr int b = f();

I think there's a contradiction between what define_aggregate needs and that hypothetical write_char_to_file needs from the same language construct (which is currently called plainly constant-evaluated expression). If we say that we're not going to guarantee idempotency for any function that can have side effects during constant evaluation, then it seems that it just can't work. What do you think of this?

I'm collecting material to present to WG21 (and for my own understanding, too), because I don't feel we have a strong position there.

[zygoloid]

I think there's a contradiction between what define_aggregate needs and that hypothetical write_char_to_file needs from the same language construct (which is currently called plainly constant-evaluated expression). If we say that we're not going to guarantee idempotency for any function that can have side effects during constant evaluation, then it seems that it just can't work. What do you think of this?

This behavior of define_aggregate seems strange to me -- I wonder if it's trying to work around a "the same constant expression may be evaluated more than once" concern, and if we instead guaranteed that doesn't happen, they could avoid the complexity and instead say that it's an error to define a type more than once.

As an alternative to define_aggregate being idempotent, something like:

consteval int f() {
  if (!is_defined(^^S)) {
    define_aggregate(^^S, {}); 
  }
  write_char_to_file("output.txt", 'a');
  return 0;
}

seems like it could be workable if that's the behavior you want. (Though that doesn't have the same "check the definition is the same each time" property.)

Note that idempotency is not composable if you can observe whether the idempotent action has already happened. For example:

struct A;
struct B;
consteval void f() {
  if (!is_defined(^^A)) { define_aggregate(^^A, {}); }
  else { define_aggregate(^^B, {}); }
}

Here, f() is not idempotent, even though all the pieces that it's constructed from are. Evaluating a call to f() twice is not the same as evaluating a call to it once.

[katzdm]

I think there's a contradiction between what define_aggregate needs and that hypothetical write_char_to_file needs from the same language construct (which is currently called plainly constant-evaluated expression). If we say that we're not going to guarantee idempotency for any function that can have side effects during constant evaluation, then it seems that it just can't work. What do you think of this?

This behavior of define_aggregate seems strange to me -- I wonder if it's trying to work around a "the same constant expression may be evaluated more than once" concern, and if we instead guaranteed that doesn't happen, they could avoid the complexity and instead say that it's an error to define a type more than once.

As an alternative to define_aggregate being idempotent, something like:

consteval int f() {
  if (!is_defined(^^S)) {
    define_aggregate(^^S, {}); 
  }
  write_char_to_file("output.txt", 'a');
  return 0;
}

seems like it could be workable if that's the behavior you want. (Though that doesn't have the same "check the definition is the same each time" property.)

Note that idempotency is not composable if you can observe whether the idempotent action has already happened. For example:

struct A;
struct B;
consteval void f() {
  if (!is_defined(^^A)) { define_aggregate(^^A, {}); }
  else { define_aggregate(^^B, {}); }
}

Here, f() is not idempotent, even though all the pieces that it's constructed from are. Evaluating a call to f() twice is not the same as evaluating a call to it once.

Hey Richard - We added idempotency to define_aggregate in response to concern from Clang maintainers that it could interact poorly with clang-repl. The argument was that idempotency would provide a better experience for a user that wanted to re-evaluate an expression that was previously submitted for evaluation. I think, for define_aggregate, it's a reasonable choice (but no, this is not a case of P2996 authors trying to work around our own model - the original behavior was indeed non-idempotent).

@Endilll re: the write_char_to_file example:

struct S;
consteval int f() {
  define_aggregate(^^S, {});
  write_char_to_file("output.txt", 'a');
  return 0;
}

constexpr int a = f();  // <- this is a plainly constant-evaluated expression.
constexpr int b = f();  // <- so is this one.

There is nothing about a call to define_aggregate that prevents it from being evaluated more than once. The "special expressions" (i.e., the ones "plainly constant-evaluated") are the initializers of a and b; those are guaranteed to be evaluated once and only once. Thus is it only within such evaluations that a declaration is allowed to be "produced"; producing a declaration from any other manifestly constant-evaluated expression makes the program ill-formed.

size_t runtime_fn() {
  struct S;
  return size_of(define_aggreate(^^S, {}));
    // 'size_of(...)' is a manifestly constant-evaluated immediate invocation, but
    // not plainly constant-evaluated. Because it produces a declaration, the program
    // is ill-formed.
}

It's true that if define_aggregate lacks idempotency, the write_char_to_file example becomes ill-formed - but that's easily fixed. You would embed the run-once-only part in a consteval-block:

struct S;
consteval int f() {
  consteval {
    define_aggregate(^^S, {});
  }  // <- 'define_aggregate' call is evaluated *here*.
  write_char_to_file("output.txt", 'a');
  return 0;
}

constexpr int a = f();  // initializer is still evaluated only once, but conteval-block is not reevaluated.
constexpr int b = f();  // same for 'b'.

[cor3ntin]

@zygoloid I think we ended up concluding that

• The tooling situation will not change (When Sema is not available mutation can't happen and we provide a default no-op implementation of the mutation function)
• It's easy to check in SemaProxyImpl (or wherever it's called from ) that define class etc only happen in what P2996 calls a "plainly evaluated constant evaluated context" (constexpr initialization + consteval block as of the Poland meeting)

I initially was of the same opinion as you, but I think we would have to modify most call sites, and it's unclear that it would buy us anything (On the other hand, I'm not thrilled about a pointer to Sema being stashed in ASTContext, just because it feels weird, but it's not an issue in practice)

PS: Note that it seems to still be unclear how we should handle try evaluation (destructors, vlas), when the evaluation fails after side effects are produced

[AaronBallman]

@zygoloid I think we ended up concluding that

* The tooling situation will not change (When Sema is not available mutation can't happen and we provide a default no-op implementation of the mutation function)

* It's easy to check in `SemaProxyImpl` (or wherever it's called from ) that define class etc only happen in what P2996 calls a "plainly evaluated constant evaluated context" (constexpr initialization + consteval block as of the Poland meeting)

I initially was of the same opinion as you, but I think we would have to modify most call sites, and it's unclear that it would buy us anything (On the other hand, I'm not thrilled about a pointer to Sema being stashed in ASTContext, just because it feels weird, but it's not an issue in practice)

I'm of the same opinion. My logic when I proposed this design to Vlad was:

1. We need a callback somewhere; it can be explicitly part of the interfaces or it can be hidden from view.
2. Normally, explicit interfaces are better; however, this interface can only realistically be implemented by one thing: Sema. No user of (e.g.) clang-tidy is going to implement their own semantic layering just so constant evaluation of side effecting reflection works.
3. We want to cause as little disruption as possible for downstream consumers of the AST library. That means: no requirement to link clangSema in to clangAST (layering violations) and it means that interface changes should come with enough benefit to warrant breaking every downstream interface trying to evaluate constant expressions (of which there are plenty).
4. Given that Sema is the only viable way to implement the callback and that we don't want to change the interfaces without benefit, it didn't make sense to explicitly modify all the constant expression evaluating APIs to thread that through them. Instead, it seemed less invasive to have a hidden pointer to the callback somewhere and if that pointer is null, we react as though the side effect simply never took place (or we could diagnose more explicitly if we wanted).

This approach should have the least impact on downstream consumers because the AST should be fully formed before tools like clang-tidy, etc are evaluating the AST (so side effects should have already taken place), so the evaluation results should Just Work™ in most cases, so long as the execution path was exercised by the compiler.

It's not an ideal design, but I think it strikes the right balance. I wonder if there's a way we can have more of a bright line between the APIs which can mutate state and ones which can't. For example, could we come up with an attribute we require to be written on the API and is checked via a clang-tidy check?

PS: Note that it seems to still be unclear how we should handle try evaluation (destructors, vlas), when the evaluation fails after side effects are produced

+1, though my understanding was that if evaluation fails, then the program should be ill-formed, and so what happens with destructors, etc is not really important. Did I understand wrong though?

[zygoloid]

I initially was of the same opinion as you, but I think we would have to modify most call sites

It seems to me that we'd need to modify those call sites that want to perform an evaluation of a "plainly constant-evaluated expression". Aren't those exactly the call sites we need to modify anyway, in order to enable the "plainly constant-evaluated" mode, as you mention:

It's easy to check in SemaProxyImpl (or wherever it's called from ) that define class etc only happen in what P2996 calls a "plainly evaluated constant evaluated context" (constexpr initialization + consteval block as of the Poland meeting)

Yeah. So it seems like our options are:

1. All calls to the evaluator in this new mode are passed a new flag to say "this is a plainly constant-evaluated evaluation", and the code is broken if there's not a SemaProxyImpl stashed away, but we have no static checks for that. All SemaProxyImpl methods must check the corresponding state and we'll have subtle bugs if they don't, but we have no static checks for that either.
2. All calls to the evaluator in this new mode are passed a SemaProxyImpl which indicates that "this is a plainly constant-evaluated evaluation". All code that uses the proxy has to check that it's present.

In terms of changes to the caller, these seem the same to me. (The few "plainly constant-evaluated" cases -- all of which should be within Sema -- change. No other callers change.) But the latter seems to give us more static safety than the former. What am I missing?

[zygoloid]

Hey Richard - We added idempotency to define_aggregate in response to concern from Clang maintainers that it could interact poorly with clang-repl. The argument was that idempotency would provide a better experience for a user that wanted to re-evaluate an expression that was previously submitted for evaluation.

I see. I've given it some thought, and I don't agree that this argument should govern the behavior of define_aggregate. I would expect

struct S;
struct S { ... };

to behave exactly the same in a C++ interpreter / REPL as

struct S;
consteval { define_aggregate(^^S, {}); }

I think not following that principle would create unnecessary work for the implementation and confusion for users, as well as harming the general idea that code generated by metaprogramming works the same way as code generated by source declarations.

If, for a syntactic struct definition, an implementation can "undo" defining the struct and rewind back to before the definition, then I'd expect the same behavior for a metaprogramming struct definition (and your proposal shouldn't try to get in the way of that or dictate the behavior in that case, because it's outside the scope of the C++ standard). Conversely, if the implementation can't undo it in one case, I'd expect it to undo it in neither case. More broadly, if this is a real problem for an interpreter, then it's not unique to define_aggregate and applies to all syntactic and metaprogramming-introduced declarations and definitions, and such an interpreter should endeavor to use the same solution in all the places where the problem appears -- but, again, that situation and its chosen solution are outside the scope of the C++ standard and we don't need rules in the standard to enable the implementation to solve this problem.

Instead of supporting some kind of an "undo" mechanism, it might make sense for an interpreter to treat definitions as idempotent in general (perhaps with a special rule that we use the most recent body for a function definition). For such an implementation that makes that choice, it would seem appropriate for define_aggregate to also be idempotent. But that should be a specific rule for that implementation, not a rule that appears in the standard. And we certainly shouldn't change the standard to require this unusual implementation-specific rule to be implemented by all C++ implementations, because it might better fit a non-conforming model that clang-repl might adopt (and as far as I'm aware, we haven't chosen to make definitions idempotent in clang-repl).

[Endilll]

For such an implementation that makes that choice, it would seem appropriate for define_aggregate to also be idempotent. But that should be a specific rule for that implementation, not a rule that appears in the standard. And we certainly shouldn't change the standard to require this unusual implementation-specific rule to be implemented by all C++ implementations

@zygoloid would it be correct to say that you want the Standard to leave idempotency of functions with side effects up to implementations, and, consequently, you want the model of how side effects are integrated into constant evaluation to work for both idempotent and non-idempotent functions?

[zygoloid]

@zygoloid would it be correct to say that you want the Standard to leave idempotency of functions with side effects up to implementations, and, consequently, you want the model of how side effects are integrated into constant evaluation to work for both idempotent and non-idempotent functions?

No, I think the standard should specify the exact semantics of its metaprogramming primitives. And for metaprogramming actions that generate code in particular, I think they should behave the same as the source code they are intended to be equivalent to -- and thus should produce redefinition errors as usual if the same definition is created more than once, regardless of whether one or both of the definitions came from a metaprogram. For other side effects, the rules for that primitive should specify what the behavior is -- and for example, whether we want some kind of deduplication or not.

Separately, I think if an interpreter wants to apply some level of idempotency to redefinitions that the standard says are invalid, in order to better support things like re-parsing the same code (but with, say, a function body changed), it should do so consistently across different kinds of entity and regardless of whether they come directly from parsing source code or from metaprogramming. But that's out of scope for current standardization efforts (one could imagine explicit support for REPL-style interpreters in the C++ standard, but we don't have any such thing right now).

[AaronBallman]

No, I think the standard should specify the exact semantics of its metaprogramming primitives. And for metaprogramming actions that generate code in particular, I think they should behave the same as the source code they are intended to be equivalent to -- and thus should produce redefinition errors as usual if the same definition is created more than once, regardless of whether one or both of the definitions came from a metaprogram. For other side effects, the rules for that primitive should specify what the behavior is -- and for example, whether we want some kind of deduplication or not.

There's a part of me that wonders whether this should actually be part of the function interface (in a viral manner) so that the control is left to the interface designer. e.g., if you have a side effecting operation, the interface designer could have a way to say "side effects happen once regardless of how many calls to this function are made" or "these effects happen each time the call is made". (There could be a default behavior, I would learn towards "these effects happen each time the call is made".) This would allow for both idempotency and not, depending on the programmer's needs. The downside is, it's more complicated and viral annotations are sometimes not appreciated by everyone.

[AaronBallman]

Hey Richard - We added idempotency to define_aggregate in response to concern from Clang maintainers that it could interact poorly with clang-repl. The argument was that idempotency would provide a better experience for a user that wanted to re-evaluate an expression that was previously submitted for evaluation. I think, for define_aggregate, it's a reasonable choice (but no, this is not a case of P2996 authors trying to work around our own model - the original behavior was indeed non-idempotent).

I think I was the maintainer who mentioned that. :-)

I think we want consteval function calls to be idempotent because it means we can memoize calls for improved performance and it matches existing implementation/mental models of what constant evaluation means. Certainly that helps with things like clang-repl too, but clang-repl isn't standards conforming anyway so I don't think it should push the design of the standards feature too much.

I think there's no way we can make consteval function calls themselves idempotent once C++ has reflection with side effects because that introduces observable effects outside of the value computations. Same inputs can lead to different outputs.

So whether define_aggregate itself is idempotent is kind of moot. What we want is for consteval functions to produce the same outputs when given the same inputs, but I believe there's no reasonable way to accomplish that. So I think we can make it non-idempotent without really losing anything more.