Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ClangPlugins: Invert lambda capture mechanism; Emit errors instead of warning #24361

Merged

Conversation

mattco98
Copy link
Collaborator

@mattco98 mattco98 commented May 18, 2024

This also removes the IGNORE_USE_IN_ESCAPING_LAMBDA annotation in favor of a DOES_NOT_OUTLIVE_CAPTURES annotation that is applied directly to the lambda instead of to each individual capture declaration.

I also added a FixIt hint for one of the errors:
image

Soft depends on #24350, as otherwise those issues will be flagged as errors and fail the build, but that technically doesn't matter until this is integrated in CI

@mattco98 mattco98 requested a review from AtkinsSJ as a code owner May 18, 2024 14:28
@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label May 18, 2024
@mattco98
Copy link
Collaborator Author

mattco98 commented May 18, 2024

Looks like clang-format can't handle the C++23 lambda attributes if they're macro identifiers unfortunately, so that change will have to wait

@mattco98 mattco98 marked this pull request as draft May 18, 2024 16:34
@nico
Copy link
Collaborator

nico commented May 18, 2024

Looks like clang-format can't handle the C++23 lambda attributes if they're macro identifiers unfortunately, so that change will have to wait

I just filed llvm/llvm-project#92657 -- is this that same bug?

@mattco98
Copy link
Collaborator Author

mattco98 commented May 18, 2024

I just filed llvm/llvm-project#92657 -- is this that same bug?

Maybe? Depends on what that annotation is applied to. This bug is related to the annotations introduced in P2173R1, which apply directly to the operator() of the lambda, like in this snippet:

#define ATTR [[clang::annotate("attr")]]
void foo()
{
    auto lambda = [] ATTR (int& arg) {};
}

I know annotations that appear after the return type apply to that type, but I'm not sure about annotations that are applied before the return type. If this is the same thing then I'll just use the style you used instead (though what does the annotation apply to if there is no return clause at all?)

e: Nope, different issue:

❯ clang test.cpp -std=c++23
test.cpp:4:33: error: 'annotate' attribute cannot be applied to types
    4 |     auto lambda = [] (int& arg) ATTR {};
      |                                 ^
test.cpp:1:16: note: expanded from macro 'ATTR'
    1 | #define ATTR [[clang::annotate("attr")]]
      |                ^
1 error generated.

@github-actions github-actions bot removed the 👀 pr-needs-review PR needs review from a maintainer or community member label May 18, 2024
@mattco98
Copy link
Collaborator Author

ref: llvm/llvm-project#92661

@mattco98 mattco98 force-pushed the gc-verifier-invert-lambda-capture branch from 3dad58f to d005c3b Compare May 19, 2024 17:47
@mattco98 mattco98 marked this pull request as ready for review May 19, 2024 17:47
@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label May 19, 2024
@mattco98 mattco98 requested a review from ADKaster May 20, 2024 23:50
@mattco98 mattco98 force-pushed the gc-verifier-invert-lambda-capture branch 2 times, most recently from 9db16d5 to 5e50f31 Compare May 21, 2024 00:22
@BuggieBot
Copy link
Member

Hello!

One or more of the commit messages in this PR do not match the SerenityOS code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why.
Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.

@mattco98 mattco98 force-pushed the gc-verifier-invert-lambda-capture branch 2 times, most recently from 5e50f31 to 97d73eb Compare May 23, 2024 00:15
See the next commit for an explanation
Instead of being opt-out with NOESCAPE, it is now opt-in with ESCAPING.
Opt-out is ideal, but unfortunately this was extremely noisy when
compiling the entire codebase. Escaping functions are rarer than non-
escaping ones, so let's just go with that for now.

This also allows us to gradually add heuristics for detecting missing
ESCAPING annotations and emitting them as errors. It also nicely matches
the spelling that Swift uses (@escaping), which is where this idea
originally came from.
This isn't comprehensive; just a result of a simple grep search.
Now that the lambda capture plugin isn't full of false-positives, we can
make the jump and start halting builds for these errors. It also allows
these plugins to be useful in CI.
@mattco98 mattco98 force-pushed the gc-verifier-invert-lambda-capture branch from 97d73eb to 2f85118 Compare May 23, 2024 00:32
@ADKaster ADKaster merged commit 6a4938a into SerenityOS:master May 23, 2024
12 checks passed
@github-actions github-actions bot removed the 👀 pr-needs-review PR needs review from a maintainer or community member label May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants