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.
This commit is contained in:
parent
a5f4c9a632
commit
e0d6afbabe
Notes:
sideshowbarker
2024-07-17 03:25:24 +09:00
Author: https://github.com/mattco98 Commit: https://github.com/SerenityOS/serenity/commit/e0d6afbabe Pull-request: https://github.com/SerenityOS/serenity/pull/24361 Reviewed-by: https://github.com/ADKaster ✅
5 changed files with 13 additions and 29 deletions
|
@ -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
|
||||
|
||||
|
|
|
@ -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<clang::ParmVarDecl>("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);
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
@ -8,16 +8,20 @@
|
|||
|
||||
#include <AK/Function.h>
|
||||
|
||||
// expected-note@+1 {{Annotate the parameter with NOESCAPE if the lambda will not outlive the function call}}
|
||||
void take_fn(Function<void()>) { }
|
||||
void take_fn_escaping(ESCAPING Function<void()>) { }
|
||||
|
||||
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;
|
||||
});
|
||||
}
|
||||
|
|
|
@ -9,11 +9,12 @@
|
|||
|
||||
#include <AK/Function.h>
|
||||
|
||||
void take_fn(Function<void()>) { }
|
||||
void take_fn(ESCAPING Function<void()>) { }
|
||||
|
||||
void test()
|
||||
{
|
||||
IGNORE_USE_IN_ESCAPING_LAMBDA int a = 0;
|
||||
|
||||
take_fn([&a] {
|
||||
(void)a;
|
||||
});
|
||||
|
|
|
@ -1,20 +0,0 @@
|
|||
/*
|
||||
* Copyright (c) 2024, Matthew Olsson <mattco@serenityos.org>
|
||||
*
|
||||
* SPDX-License-Identifier: BSD-2-Clause
|
||||
*/
|
||||
|
||||
// RUN: %clang++ -cc1 -verify %plugin_opts% %s 2>&1
|
||||
// expected-no-diagnostics
|
||||
|
||||
#include <AK/Function.h>
|
||||
|
||||
void take_fn(NOESCAPE Function<void()>) { }
|
||||
|
||||
void test()
|
||||
{
|
||||
int a = 0;
|
||||
take_fn([&a] {
|
||||
(void)a;
|
||||
});
|
||||
}
|
Loading…
Add table
Reference in a new issue