Page MenuHomePhabricator

[coroutines][PR40978] Emit error for co_yield within catch block
ClosedPublic

Authored by modocache on Mar 6 2019, 10:17 PM.

Details

Summary

As reported in https://bugs.llvm.org/show_bug.cgi?id=40978, it's an
error to use the co_yield or co_await keywords outside of a valid
"suspension context" as defined by [expr.await]p2 of
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/n4775.pdf.

Whether or not the current scope was in a function-try-block's
(https://en.cppreference.com/w/cpp/language/function-try-block) handler
could be determined using scope flag Scope::FnTryCatchScope. No
such flag existed for a simple C++ catch statement, so this commit adds
one.

Diff Detail

Repository
rC Clang

Event Timeline

modocache created this revision.Mar 6 2019, 10:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2019, 10:17 PM
EricWF added inline comments.Mar 6 2019, 11:02 PM
lib/Sema/SemaCoroutine.cpp
675

We can still build a valid AST after encountering this error, no?

modocache added inline comments.Mar 6 2019, 11:14 PM
lib/Sema/SemaCoroutine.cpp
675

I believe so. Just to be clear: you'd like me to continue building the AST even after emitting this error diagnostic? My understanding is that most of this file bails soon after any error is encountered (correct me if that's wrong). I'm happy to change that, but I wonder if it'd be better to do that in a separate diff...?

lewissbaker added inline comments.Mar 7 2019, 10:43 AM
lib/Sema/SemaCoroutine.cpp
197–200

Does removing this check now mean that we're not checking that co_return statements don't appear in unevaluated contexts?

Or is that already handled elsewhere by the fact that co_return statements are not expressions and are therefore detected earlier as a grammar violation when parsing sizeof() expression?

675

Do the scope flags reset when a lambda occurs within a catch-block?

eg. The following should still be valid.

try { ... }
catch (...) {
  []() -> task { co_await foo(); }();
}
test/SemaCXX/coroutines.cpp
299–311

Not related to this diff, but...

I don't think that these should be ill-formed.

According to N4775 there are only exclusions added for [class.ctor] and [class.dtor].
I can't see anything in the spec that says that assignment special member functions cannot be coroutines.

modocache added inline comments.Mar 7 2019, 11:46 AM
lib/Sema/SemaCoroutine.cpp
197–200

That's right! If one were to attempt to use a co_return within a subexpression of sizeof, they'd see error: expected expression before they'd ever have seen this error message. So I believe there's no need to perform this check for co_return, and I believe that's why the revisions to the standard included in the Coroutines TS don't make any special mention of disallowing co_return in those contexts.

675

I believe they're reset, the example you posted compiles fine with this patch. I'll add a test case specifically for this and confirm exactly where the scope flags are reset or ignored in the lambda case.

test/SemaCXX/coroutines.cpp
299–311

That's a great point. Could you create a Bugzilla for this work? And maybe we can get @GorNishanov's opinion?

modocache updated this revision to Diff 189763.Mar 7 2019, 11:51 AM

Add test case for executing a lambda using co_yield within a catch block.

modocache marked an inline comment as done.Mar 7 2019, 12:02 PM
modocache added inline comments.
lib/Sema/SemaCoroutine.cpp
675

When the parser encounters a lambda, it takes the path Parser::ParseLambdaExpression -> Parser::ParseLambdaExpressionAfterIntroducer -> Parser::ParseCompoundStatementBody, which creates an inner scope for the body of the lambda. This inner scope does not have the Scope::CatchScope flag, so it doesn't result in the error.

Although, your question did make me realize that this compiles just fine, even with this patch:

void example() {
  try {
    throw;
  } catch (...) {
    try {
      co_await a;
    }
  }
}

But I believe this above case should also be treated as an error, right?

modocache marked an inline comment as not done.Mar 7 2019, 12:02 PM
GorNishanov added inline comments.Mar 7 2019, 1:05 PM
test/SemaCXX/coroutines.cpp
299–311

In 2015, such as https://wg21.link/N4499 there was a blank prohibition:
"A special member function shall not be a coroutine."

Later, at @rsmith suggestion, we relaxed it a little and banned only constructors and destructors.

I am not yet aware of any use cases where such coroutines would be useful.

lewissbaker added inline comments.Mar 7 2019, 1:07 PM
lewissbaker added inline comments.Mar 7 2019, 1:08 PM
lib/Sema/SemaCoroutine.cpp
675

Yes, I believe that co_await within a try-block within a catch-block should not be allowed.

GorNishanov requested changes to this revision.Mar 7 2019, 1:11 PM
GorNishanov added inline comments.
lib/Sema/SemaCoroutine.cpp
675

Yes. That will result in suspension of the coroutine and we don't yet know how to suspend in catch blocks.

Also, I agree with @EricWF, this error should not prevent us from continuing semantic analysis with the rest of the function body.

This revision now requires changes to proceed.Mar 7 2019, 1:11 PM
modocache updated this revision to Diff 190718.Mar 14 2019, 1:48 PM

Thank you for the reviews! This revision fixes the nested try/catch behavior. I still need to modify this such that semantic analysis continues for the rest of the function body.

modocache updated this revision to Diff 190727.Mar 14 2019, 2:19 PM

Remove unused function parameter.

modocache updated this revision to Diff 190740.Mar 14 2019, 3:12 PM

OK, I've responded to all your review comments -- thank you! Please give this another look when you get a chance. I would like to emit note diagnostics pointing to the catch blocks, but I've left that as a FIXME for now.

modocache marked 5 inline comments as done.Mar 14 2019, 3:13 PM
This revision is now accepted and ready to land.Mar 15 2019, 9:58 AM

Great, thanks for the reviews, everyone!

This revision was automatically updated to reflect the committed changes.

@modocache Could you take a look please ? Nested scopes in a catch statement are completely broken because of this patch.

Sorry for the late response, I'm looking now. Should I revert this for now?

I'm pushing a revert. Sorry for the trouble!

modocache added a comment.EditedMar 22 2019, 9:08 AM

Reverted in rC356774. I'll rework this change along with a test confirming https://bugs.llvm.org/show_bug.cgi?id=41171 doesn't occur. Apologies!