From e0d6afbabeb8cc0567500edc435377b9ac094ee2 Mon Sep 17 00:00:00 2001 From: Matthew Olsson Date: Fri, 17 May 2024 17:09:52 -0700 Subject: [PATCH] ClangPlugins: Invert the lambda detection escape mechanism 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. --- AK/Function.h | 2 ++ .../LambdaCapturePluginAction.cpp | 9 +++------ .../lambda_capture_local_by_ref.cpp | 8 ++++++-- ...bda_capture_local_marked_ignore_by_ref.cpp | 3 ++- ...a_marked_noescape_capture_local_by_ref.cpp | 20 ------------------- 5 files changed, 13 insertions(+), 29 deletions(-) delete mode 100644 Tests/ClangPlugins/LambdaTests/lambda_marked_noescape_capture_local_by_ref.cpp diff --git a/AK/Function.h b/AK/Function.h index b8f4b4ef13..d3732cdb2a 100644 --- a/AK/Function.h +++ b/AK/Function.h @@ -40,9 +40,11 @@ namespace AK { // These annotations are used to avoid capturing a variable with local storage in a lambda that outlives it #if defined(AK_COMPILER_CLANG) +# define ESCAPING [[clang::annotate("serenity::escaping")]] // FIXME: When we get C++23, change this to be applied to the lambda directly instead of to the types of its captures # define IGNORE_USE_IN_ESCAPING_LAMBDA [[clang::annotate("serenity::ignore_use_in_escaping_lambda")]] #else +# define ESCAPING # define IGNORE_USE_IN_ESCAPING_LAMBDA #endif diff --git a/Meta/Lagom/ClangPlugins/LambdaCapturePluginAction.cpp b/Meta/Lagom/ClangPlugins/LambdaCapturePluginAction.cpp index 64db3aa01e..6ca31fdb70 100644 --- a/Meta/Lagom/ClangPlugins/LambdaCapturePluginAction.cpp +++ b/Meta/Lagom/ClangPlugins/LambdaCapturePluginAction.cpp @@ -22,6 +22,7 @@ AST_MATCHER_P(clang::Decl, hasAnnotation, std::string, name) return false; } +// FIXME: Detect simple lambda escape patterns so we can enforce ESCAPING annotations in the most common cases class Consumer : public clang::ASTConsumer , public clang::ast_matchers::internal::CollectMatchesCallback { @@ -87,7 +88,7 @@ public: allOf( // It's important that the parameter has a RecordType, as a templated type can never escape its function hasType(cxxRecordDecl()), - unless(hasAnnotation("serenity::noescape")))) + hasAnnotation("serenity::escaping"))) .bind("lambda-param-ref")))), this); } @@ -105,7 +106,7 @@ public: if (capture->capturesThis() || capture->getCaptureKind() != clang::LCK_ByRef) return; - auto diag_id = diag_engine.getCustomDiagID(clang::DiagnosticsEngine::Warning, "Variable with local storage is captured by reference in a lambda that may be asynchronously executed"); + auto diag_id = diag_engine.getCustomDiagID(clang::DiagnosticsEngine::Warning, "Variable with local storage is captured by reference in a lambda marked ESCAPING"); diag_engine.Report(capture->getLocation(), diag_id); clang::SourceLocation captured_var_location; @@ -116,10 +117,6 @@ public: } diag_id = diag_engine.getCustomDiagID(clang::DiagnosticsEngine::Note, "Annotate the variable declaration with IGNORE_USE_IN_ESCAPING_LAMBDA if it outlives the lambda"); diag_engine.Report(captured_var_location, diag_id); - - auto const* param = result.Nodes.getNodeAs("lambda-param-ref"); - diag_id = diag_engine.getCustomDiagID(clang::DiagnosticsEngine::Note, "Annotate the parameter with NOESCAPE if the lambda will not outlive the function call"); - diag_engine.Report(param->getTypeSourceInfo()->getTypeLoc().getBeginLoc(), diag_id); } } diff --git a/Tests/ClangPlugins/LambdaTests/lambda_capture_local_by_ref.cpp b/Tests/ClangPlugins/LambdaTests/lambda_capture_local_by_ref.cpp index 2fbcb7a9cf..5c1c29ff68 100644 --- a/Tests/ClangPlugins/LambdaTests/lambda_capture_local_by_ref.cpp +++ b/Tests/ClangPlugins/LambdaTests/lambda_capture_local_by_ref.cpp @@ -8,16 +8,20 @@ #include -// expected-note@+1 {{Annotate the parameter with NOESCAPE if the lambda will not outlive the function call}} void take_fn(Function) { } +void take_fn_escaping(ESCAPING Function) { } void test() { // expected-note@+1 {{Annotate the variable declaration with IGNORE_USE_IN_ESCAPING_LAMBDA if it outlives the lambda}} int a = 0; - // expected-warning@+1 {{Variable with local storage is captured by reference in a lambda that may be asynchronously executed}} take_fn([&a] { (void)a; }); + + // expected-warning@+1 {{Variable with local storage is captured by reference in a lambda marked ESCAPING}} + take_fn_escaping([&a] { + (void)a; + }); } diff --git a/Tests/ClangPlugins/LambdaTests/lambda_capture_local_marked_ignore_by_ref.cpp b/Tests/ClangPlugins/LambdaTests/lambda_capture_local_marked_ignore_by_ref.cpp index 496ccdc619..c6c7467490 100644 --- a/Tests/ClangPlugins/LambdaTests/lambda_capture_local_marked_ignore_by_ref.cpp +++ b/Tests/ClangPlugins/LambdaTests/lambda_capture_local_marked_ignore_by_ref.cpp @@ -9,11 +9,12 @@ #include -void take_fn(Function) { } +void take_fn(ESCAPING Function) { } void test() { IGNORE_USE_IN_ESCAPING_LAMBDA int a = 0; + take_fn([&a] { (void)a; }); diff --git a/Tests/ClangPlugins/LambdaTests/lambda_marked_noescape_capture_local_by_ref.cpp b/Tests/ClangPlugins/LambdaTests/lambda_marked_noescape_capture_local_by_ref.cpp deleted file mode 100644 index 5d52e310b6..0000000000 --- a/Tests/ClangPlugins/LambdaTests/lambda_marked_noescape_capture_local_by_ref.cpp +++ /dev/null @@ -1,20 +0,0 @@ -/* - * Copyright (c) 2024, Matthew Olsson - * - * SPDX-License-Identifier: BSD-2-Clause - */ - -// RUN: %clang++ -cc1 -verify %plugin_opts% %s 2>&1 -// expected-no-diagnostics - -#include - -void take_fn(NOESCAPE Function) { } - -void test() -{ - int a = 0; - take_fn([&a] { - (void)a; - }); -}