Linting: reject more use-after-moves - #7451
Merged
Merged
Conversation
Contributor
|
@jtolio Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
jasnell
approved these changes
Sep 21, 2026
Contributor
|
Out of curiosity, which false positives did you get when adding kj::mv to use-after-move.InvalidationFunctions? Wonder if this is something we could upstream |
jtolio
force-pushed
the
jolio/linting-reje-yw7unu
branch
2 times, most recently
from
September 21, 2026 18:50
bf4dd99 to
1a65b64
Compare
Member
Author
I'm still working through it, but the main problem is KJ_CASE_ONEOF, which expands to a loop, which executes at most once, but the linter doesn't understand there won't be another iteration. There's hundreds of issues due to that. I think I can fix everything else. |
Member
Author
|
This change would benefit from capnproto/capnproto#2833, but does not need it right now. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7451 +/- ##
=======================================
Coverage 38.42% 38.43%
=======================================
Files 858 858
Lines 264343 264389 +46
Branches 24596 24602 +6
=======================================
+ Hits 101577 101619 +42
+ Misses 148948 148947 -1
- Partials 13818 13823 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
harrishancock
approved these changes
Sep 22, 2026
jtolio
force-pushed
the
jolio/linting-reje-yw7unu
branch
2 times, most recently
from
September 22, 2026 18:32
f16e110 to
0a94273
Compare
jtolio
force-pushed
the
jolio/linting-reje-yw7unu
branch
from
September 25, 2026 15:06
102a04a to
ca56b7e
Compare
Member
Author
|
Rebase (some conflicts) ^ |
jtolio
force-pushed
the
jolio/linting-reje-yw7unu
branch
from
September 25, 2026 15:07
ca56b7e to
3ef9816
Compare
Member
Author
|
Add support for kj::Own, kj::Rc, kj::Arc as standard smart pointers ^ |
jtolio
force-pushed
the
jolio/linting-reje-yw7unu
branch
from
September 25, 2026 15:08
3ef9816 to
3e63510
Compare
Member
Author
|
Trivial rebase ^ |
Argument evaluation order is actually undefined, so, this could not end up working out right. Force the order instead.
This was essentially:
return context.attachSpans(
js,
context.awaitIo(...,
[traceContext = kj::mv(traceContext)](...) { ... }),
kj::mv(traceContext));
so, let's remove the second traceContext attachment since it is
already inside the continuation.
The upstream "bugprone-use-after-move" linter has two problems:
1) First, it has not been configured to recognize kj::mv!
2) but, we can't just tell it to recognize kj::mv without causing
a huge amount of false positives.
So, we fork it to be more kj friendly and tell it about kj::mv.
Confirmed that this identified a few existing use after moves.
RemotePromise stores its promise and pipeline as separate base subobjects. Consuming both by moving the same RemotePromise twice is correct but trips use-after-move linting; releaseParts() (from capnproto/capnproto#2833) releases both at once instead.
Keep the upstream source and header verbatim so the adaptation can be reviewed separately. They are deliberately outside the build until the following commit supplies the local interface and CFG adjustment hook. Source: llvmorg-22.1.5, clang-tools-extra/clang-tidy/bugprone. https://github.com/llvm/llvm-project/tree/llvmorg-22.1.5 The original copyright notices and LLVM license are retained. Assisted-by: Codex:gpt-6-astra
KJ_CASE_ONEOF clears its loop sentinel after the selected body, but Clang's CFG still permits another iteration. Suppressing moves inside the macro hides real uses; deciding from later references also admits false positives for unevaluated expressions. Redirect the generated back edge to the loop exit before sequencing and reachability analysis. LLVM 22 keeps this CFG private to its checker implementation. Preserve its analysis in an attributed local snapshot with a CFG adjustment hook, leaving the shared AST unchanged. Exercise the actual dependency macros with uses inside and after cases, reinitialization, unevaluated uses, continue, nested cases, lambdas, and genuine loops and goto edges. Assisted-by: Codex:gpt-6-astra
Exercise coroutine move-captures with the real KJ_CASE_ONEOF macro. Accept a safe capture while retaining diagnostics for use after the capture and move-then-use inside its lambda.
Consuming each slot with `kj::mv(unwrapped).template take<I>()...` repeats an rvalue cast of the same object, which the use-after-move check reports. Add an rvalue-qualified apply() that forwards every slot to a callback in one consuming call and returns its result. Convert the extraction tests to apply(), preserving value, rvalue- and lvalue-reference coverage, and add tests for return forwarding, void callbacks and an empty argument pack.
Replace the repeated `kj::mv(unwrapped).template take<indexes>()...` expansions in constructor, method, static-method and functor callbacks with a single `kj::mv(unwrapped).apply(...)` call. Behavior, including void results and return-value wrapping, is unchanged.
Only `prefix` is consumed; `reverse` and `limit` are read afterwards. Write `kj::mv(options.prefix)` so the partial consumption is explicit rather than moving the whole options struct.
makeWorkerImpl() read `containerEngineConf`, `isDynamic` and `localActorConfigs` from `def` after moving it into the link callback. Save them beforehand, keeping a reference to the existing actor-config map rather than copying it.
kj::Own, kj::Rc and kj::Arc null out their source on move, just as std::unique_ptr, std::shared_ptr and std::weak_ptr are specified to. Extend the use-after-move check's smart-pointer exemption to them, so only dereferencing a moved-from KJ pointer is flagged.
reportImpl() clones the event for earlier recipients and moves it only for the last one, but the checker assumes another iteration can follow the move. Annotate the read with NOLINT and explain the invariant, preserving the no-copy path for a single tail worker.
The WeakRef and ReverseIoOwn tests inspect moved-from objects to verify ownership transfer and invalid-access behavior. Annotate those assertions with NOLINT.
jtolio
force-pushed
the
jolio/linting-reje-yw7unu
branch
from
September 25, 2026 17:26
3e63510 to
13aea4b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The upstream "bugprone-use-after-move" linter has two problems:
First, it has not been configured to recognize kj::mv!
but, we can't just tell it to recognize kj::mv without causing a huge amount of false positives.
So, we fork it to be more kj friendly and tell it about kj::mv.
Confirmed that this identified a few existing use after moves.