In D146535#4210198, @eandrews wrote:

Attributes of lambda call operator were evaluated in the context of the closure object type rather than its operator,

Just for my understanding, what did this affect with regards to the if guard for assert? CurContext or something else? I suspected it had something to do with contexts being incorrect but I wasn't familiar enough with the code to pinpoint where/what exactly.

Whether the assertion itself in getCurLambda is still pertinent,
despite no test depending on it does require more analysis.

In D124351#4207501, @eandrews wrote:

This patch causes an assertion when the attribute argument is an integer constant expression - https://godbolt.org/z/osKx5ejMb and has resulted in test fails downstream since any attribute which uses VerifyIntegerConstantExpression now hits this assert if used with lambdas.

The assert hit is in getCurLambda() :

auto *CurLSI = dyn_cast<LambdaScopeInfo>(*I);
 if (CurLSI && CurLSI->Lambda && CurLSI->CallOperator &&
     !CurLSI->Lambda->Encloses(CurContext) && CurLSI->AfterParameterList) {
   // We have switched contexts due to template instantiation.
   assert(!CodeSynthesisContexts.empty()); <----- Hits this
   return nullptr;
 }

Prior to this patch, Lambda Scope Information was not populated till after semantic analysis of attributes. This meant CurLSI->Lambda returned nullptr and we never entered the if. This patch however populates LSI during semantic analysis of lambda expression after introducer, which means we now enter the if during semantic analysis of the attribute` (AfterParameterList will be true at this point since this assert is hit way past parsing the parameters. CurContext is foo. I don't know if CurContext is right/wrong without further debugging.)

For the godbolt test pasted above, neither before nor after this patch do we hit the code where CodeSynthesisContexts is populated. I wanted to see what code actually enters that block and so I tried deleting the code to see what tests fails. What is interesting is that nothing does :/ So I don't know if that means our tests are incomplete or if this code is not required.

In D146187#4201905, @aaron.ballman wrote:

In D146187#4201223, @ChuanqiXu wrote:

but we don't define __cpp_­impl_­coroutine: http://eel.is/c++draft/tab:cpp.predefined.ft

We defined __cpp_­impl_­coroutine. And __cpp_coroutines is for Coroutines TS. I forgot to remove this too. I'll handle it in a separate patch.

Thanks for handling the TS bits, but what am I misunderstanding here: https://godbolt.org/z/KdrM713r5 ?

Do we need a release note for this?

LGTM. Triple checking with @hubert.reinterpretcast for conformance but I'm pretty sure that's correct.

In terms of the encoding question I was asking, that's information we'll have to figure out (CC @tahonermann and @cor3ntin for text encoding question). My guess (which needs verification) is that we convert the file name from the system encoding to UTF-8 internally, and this builtin will likely return UTF-8 as a result. If that's correct, I think that behavior is reasonable, but I've CCed some experts who can tell me all the things I forgot to consider.

In D129951#4179596, @cjdb wrote:

In D129951#4179481, @ldionne wrote:

In D129951#4179467, @cjdb wrote:

I'm deeply disappointed that libc++ moved away from using __function_like: that was an important part of preventing niebloid misuse. It isn't conforming to treat niebloids as function objects, which is what __function_like prevented (I just checked std::ranges::next and it seems that __function_like was completely removed).

Oops, it seems like my reply got into a race condition with yours. I am curious though, why is it non-conforming to treat niebloids as function objects? It is certainly more lax than required by the Standard, however we are allowed to make those copyable, aren't we? Does the Standard say that we *have* to make them non-usable as objects? That would change things for sure (not with this patch, but we'd probably want to try re-introducing __function_like, and we'd file a bug report against libstdc++ to do the same).

"isn't conforming" is a bit too harsh in retrospect (this applies to every conversation I've brought this up in, although I meant that it allows users to write "non-conforming code"). A more accurate way to put this is: it is conforming on a per-implementation basis, but is non-portable due to namespace.std/p6. For example, a user who wants to use Microsoft/STL won't ever be able to take advantage of using std::ranges::next as a higher-order function (and could potentially have migration issues if they're porting to Windows), because their niebloids are derived from _Not_quite_object. _Not_quite_object's predecessor in cmcstl2 was the inspiration for __function_like.

As for a refresher on why this is the reason, it's because the functions in std::ranges should be functions, but we lack the technology to make them functions.

In D129951#4179474, @ldionne wrote:

This is a really neat attribute! FWIW, I think we would most likely have used it in <ranges> if it were available back then. Assuming that GCC implemented it, I think we could consider changing to use this attribute. However:

Thanks, glad you like it :-)
(I think the "however" bit got addressed above.)

FWIW, my main thought is that I would like to see this proposed as a WG21 proposal. This might end up an attribute or something else, I'm not sure. But it would address the issue of not all implementations providing the functionality. IMO this patch and the data gathered in it would be a great motivation for WG21 to do something (I also know there are other proposals in this domain).

I don't attend WG21 anymore, but could probably work with someone who is motivated enough to see it from Evolution through plenary. My preference is a specifier, rather than an attribute. For better, or for worse, most attributes are ignorable; this is certainly a property that should never be ignored.

In D124351#4178334, @sammccall wrote:

Sorry for the delay, extracting the repro from the build system seems about as much work as minimizing it :-)

modrepo2.zip2 KBDownload

- running repro.sh hits an assert for me on the main.cpp compile.

(I suspect something much smaller is breaking the expected invariant, I just can't get a smaller crasher)

In D124351#4175717, @sammccall wrote:

We're seeing new clang crashes that bisect to this commit, with modules only.

I have it mostly-reduced and will post shortly, trying to see if I can simplify any further (since modules reproducers are a pain).

Meanwhile, the assert/stack in case it's already useful:

assertion failed at third_party/llvm/llvm-project/clang/lib/Sema/SemaTemplateInstantiate.cpp:4065 in llvm::PointerUnion<Decl *, LocalInstantiationScope::DeclArgumentPack *> *clang::LocalInstantiationScope::findInstantiationOf(const Decl *): isa<LabelDecl>(D) && "declaration not instantiated in this scope"
*** Check failure stack trace: ***
    @     0x5609e4f16f44  __assert_fail
    @     0x5609e1894234  clang::LocalInstantiationScope::findInstantiationOf()
    @     0x5609e18d073c  clang::Sema::FindInstantiatedDecl()
    @     0x5609e18a99d0  clang::TreeTransform<>::TransformLambdaExpr()
    @     0x5609e189dbee  (anonymous namespace)::TemplateInstantiator::TransformLambdaExpr()
    @     0x5609e1892442  clang::TreeTransform<>::TransformExprs()
    @     0x5609e189a71a  clang::TreeTransform<>::TransformCallExpr()
    @     0x5609e189097a  clang::TreeTransform<>::TransformStmt()
    @     0x5609e18afa54  clang::TreeTransform<>::TransformCompoundStmt()
    @     0x5609e1890902  clang::Sema::SubstStmt()
    @     0x5609e18e31df  clang::Sema::InstantiateFunctionDefinition()
    @     0x5609e18e5ed2  clang::Sema::PerformPendingInstantiations()
    @     0x5609e0fcad44  clang::Sema::ActOnEndOfTranslationUnitFragment()
    @     0x5609e0fcbb66  clang::Sema::ActOnEndOfTranslationUnit()
    @     0x5609e0d298e6  clang::Parser::ParseTopLevelDecl()
    @     0x5609e0d2388e  clang::ParseAST()
    @     0x5609e0a647c3  clang::FrontendAction::Execute()
    @     0x5609e09d81ad  clang::CompilerInstance::ExecuteAction()
    @     0x5609dfa05b08  clang::ExecuteCompilerInvocation()
    @     0x5609df9f99f1  cc1_main()
    @     0x5609df9f5d28  ExecuteCC1Tool()
    @     0x5609e0b868be  llvm::function_ref<>::callback_fn<>()
    @     0x5609e4d9ec35  llvm::CrashRecoveryContext::RunSafely()
    @     0x5609e0b86103  clang::driver::CC1Command::Execute()
    @     0x5609e0b44166  clang::driver::Compilation::ExecuteCommand()
    @     0x5609e0b4448f  clang::driver::Compilation::ExecuteJobs()
    @     0x5609e0b63e70  clang::driver::Driver::ExecuteCompilation()
    @     0x5609df9f4ee7  clang_main()
    @     0x5609df9f1bc4  main
    @     0x7fad2cda4633  __libc_start_main
    @     0x5609df9f1b2a  _start

In D144285#4176850, @rupprecht wrote:

In D144285#4163004, @cor3ntin wrote:

If however we find this change to disruptive, we should inform WG21.

Thanks for the explanation, I think I understand the issue now. I got totally thrown off by the title -- it's not about literally writing static_assert(false), it's about deferring static_assert evaluation to template instantiations. Being able to write static_assert(false) (or any falsy constant expression) is just the common use case for this.

So possibly the most trivial example of something that used to break, but now builds:

template <typename>
void Fail() {
    static_assert(false, "my assertion failed");
}

... but will still fail as soon as you invoke Fail<any_type>() somewhere.

It doesn't look like there's a lot of impact from this, and the breakages are corner cases like this. It might be worth mentioning this one case to WG21 but I'm not sure what I would change about the wording.

Thanks, LGTM

I think this make sense.
Maybe we should add a comment quoting [expr.const] p15.1 in both places though.

This expands to

@shafik Can you let me know if you are happy with the changes i made to address your feedback? Thanks!

Remove the #error / #warning tests

Address Aaron's comments

In D144626#4154122, @ChuanqiXu wrote:

In D144626#4150378, @erichkeane wrote:

That looks reasonable to me, though I fear all the libcxx failures are going to be related to this, please make sure to check them out!

I tested locally that the libcxx failures are not related to this patch after I reverted this change.

This has been sitting in the queue for a while, sorry about that.
I think this makes sense in its current iteration, with the warning always on. Have you tried to build a large project like chrome with it?

Given this has not made progress in a while, I think we should try to implement https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2280r4.html instead of this approach, as I'm not sure if that's strictly conforming, and a more holistic approach is likely to lead to better results.
What do people think?

Sorry it took me a while to reply to you. I think you convinced me this is fine as-is! Thanks

@aaron.ballman I think we should close this change as it's superseeded by https://reviews.llvm.org/D144218

@rupprecht Great to hear! Sorry again for the disruption
@shafik Thanks for the review!

Address Shafik's comments

I played with different diagnostics, we already specialize the
literal case, and I didn't find that not saying a static_assert
failed improved things.
Instead, printing, in addition of that diagnostic, an instantiation
stack provides additional useful information.
(The stack is only printed for the literal bool/integer case)

Release notes + formatting

Address Aaron's comments

@aaron.ballman is the plan to backport that to clang 16? I think it probably should.
We ought to try to improve the situation for clang 17

In D143891#4144161, @aaron.ballman wrote:

LGTM but we should add a release note for the change.

This looks good to me.
Comments are allowed anywhere between token, anywhere whitespace would be.

This makes sense to me as is. The check ClassDecl->hasTrivialDefaultConstructor() && !ClassDecl->hasNonTrivialDefaultConstructor() can only be different from status quo in C++20.
Changing isTrivial would be a lot more involved change that should not be in scope of this PR

Thanks for working on this issue.
I don't feel confident commenting on this patch's approach (I think @erichkeane is your best bet), but the small typo does make a pretty big difference here!

Can you add a test to check __has_cpp_attribute(assume) ?

This is an impressive amount of work. I think it makes sense!
Thanks a lot for doing that work.
I only have a few nits after a first review of this.

• Reword Release note
• Restore dependent instantiation tests using Richard's suggestion
• Add additional test to check that diagnostics for static_assert are emitted once per instantiation.

• Simplify how we check we are in a dependent context
• Fix typos

In D144285#4135775, @Endill wrote:

Thank you for the patch.
Any plans to backport this to 16.x branch?

Forgot to run clang-format

Fix GH60518 by not trying to capture parameters declared in the current
context.

I wonder if a better fix would be change ResultChar to be a 64 bits integer, although I think it might be over killed (ie, i'm not aware of a good use case for wanting to store values that large in a 32 bits wide char, so this looks fine.
Thanks for the fix :)

Reopen/Rebase

Thanks Tom, I agree https://github.com/llvm/llvm-project/commit/756395f61b90e30c9087b5efa8b4809ab03aff6e

Thanks for doing this Aaron.
Did you look in libc++ if they used it anywhere? (I assume they don't)

My question is, why do we need to mess up with scopes in that way outside of parsing (there are only a couple places where we do that at the moment, and they are dummy scopes which only exist to balance some push and pop, afaict they serve no other purpose).
As i said, I have no objection to this at all, it clearly fixes the issue and we should land it! But the fact we have to do this in the first place *may* be a sign that there is a deeper issue, one that we clearly should not try to address as part of this.

In D124351#4101611, @rupprecht wrote:

Hi, me again :)

I ran into an interesting build breakage from this, I can't tell if it's a legitimate breakage based on reading P2036R3 and P2579R0 (mostly I'm not a language lawyer).

struct StringLiteral {
  template <int N>
  StringLiteral(const char (&array)[N])
      __attribute__((enable_if(N > 0 && N == __builtin_strlen(array) + 1,
                               "invalid string literal")));
};

struct Message {
  Message(StringLiteral);
};

void Func1() {
  auto x = Message("x");  // Note: this is fine

  // Note: "xx\0" to force a different type, StringLiteral<3>, otherwise this
  // successfully builds.
  auto y = [&](decltype(Message("xx"))) {};

  // ^ fails with: repro.cc:18:13: error: reference to local variable 'array'
  // declared in enclosing function 'StringLiteral::StringLiteral<3>'

  (void)x;
  (void)y;
}

https://godbolt.org/z/M4E6jKxxn

Does this look like an intended breakage from this patch?

LGTM

LGTM except the duplicated comment

In D143109#4097850, @ahatanak wrote:

Doesn't Sema::FunctionScopeRAII pop the lambda scope when it goes out of scope?

In D143109#4097775, @erichkeane wrote:

@cor3ntin : Mind taking a look here? You're my lambda expert these days :)

In D124351#4097224, @aaron.ballman wrote:

In D124351#4097192, @dblaikie wrote:

In D124351#4097110, @cor3ntin wrote:

In D124351#4093840, @nikic wrote:

FYI this causes a minor compile-time regression (around 0.35% on 7zip at O0): http://llvm-compile-time-tracker.com/compare.php?from=cd173cbd7cca69c29df42cd4b42e60433435c29b&to=d708a186b6a9b050d09558163dd353d9f738c82d&stat=instructions%3Au

Just wanted to check whether that's expected.

It's certainly not great but not entirely unexpected as we are doing quite a bit more work, although it should only affect code that constructs a lot of distinct lambda.

Does 7zip meet that criteria? As a gut-check that this only costs in that case, would be nice to know if the 7zip regression is that case, or is this the cost even when that isn't true - so we could expect higher costs when it is true?

I just downloaded their latest sources and didn't spot any lambdas in the C++ code with a simple grep. They also don't use auto so I suspect lambda usage is minimal. (It's possible I missed something, I didn't look exhaustively.)

In D124351#4093840, @nikic wrote:

FYI this causes a minor compile-time regression (around 0.35% on 7zip at O0): http://llvm-compile-time-tracker.com/compare.php?from=cd173cbd7cca69c29df42cd4b42e60433435c29b&to=d708a186b6a9b050d09558163dd353d9f738c82d&stat=instructions%3Au

Just wanted to check whether that's expected.

@aaron.ballman Thanks for the review! I hope we won't find further issue with the paper this time :)

• rebase
• rename ActOnLambdaIntroducer
• Fix a crash in transformedLocalDecl

In D142401#4074959, @shafik wrote:

I could not find any tests that test for this warning.

Yes please! However the warning looks correct to me in that case. A
constructs x which constructs A etc.

• Address Aaron's comments (except renaming ActOnLambdaIntroducer as we pounder on it)
• Delete an aditional outdated comment

Symplify printing a NamedDecl in moved code per Aaron's comment