Fix the inline-function ratchet double-count#2429
Merged
Merged
Conversation
…e helper find_inline_functions walked every FunctionDef in a file (nested ones included) and descended into each one's full subtree, so a function nested two or more levels deep was emitted once per enclosing function. The recorded snapshot counts therefore overstated the real number of inline functions. Nested defs are now collected keyed by source position and counted once. Across the monorepo the only project whose recorded count changes is apps/minds (9 -> 7 with no code change). Separately, hoist the backup-detail endpoint's one remaining inline helper to a module-level _append_backup_service_check invoked via start_new_thread(args=...). This is behavior-preserving and keeps the ratchet honest, rather than trading the inline-function violation for a functools.partial one (that ratchet is at 0 for minds). With both changes the minds inline-function count is 6.
weishi-imbue
temporarily deployed
to
minds-ci-test
GitHub Actions
Inactive
Revert the api_v1 hoist: _run_check_into_results closes over five locals and is used once as a thread target, so the inline closure reads better than a module-level five-argument helper. It stays. The shared-checker double-count fix stands on its own. With the closure kept, the minds inline-function count settles at 7 (down from the buggy 9), not 6.
weishi-imbue
changed the title
weishi-imbue
temporarily deployed
to
minds-ci-test
GitHub Actions
Inactive
Contributor
|
lgtm, thanks for the fix! |
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
2 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.
What
find_inline_functionsinlibs/imbue_commondouble-counts a function nested two or more levels deep. It iterated everyFunctionDefin a file (get_ast_nodes_of_typereturns nested ones too), thenast.walked under each. A function with two enclosing functions is a descendant of both, so it was emitted once per ancestor.snapshot(N)never meant "N inline functions" for any file with 2-deep nesting.Nested defs are now collected keyed by
(lineno, col_offset)and counted once. Two distinctdefs cannot share both coordinates, so the dedup is exact.Blast radius: one project
Measured across all 44 projects that pin this ratchet, using each test's real
_DIR: exactly one recorded count changes —apps/minds, 9 -> 7. Every other project has zero 2-deep nesting (imbue_common's own count, 3, is unchanged). The two counts removed from minds are the same doubly-nested function counted twice.Why this exists
mainbriefly went red onapps/mindstest_prevent_inline_functionsafter a merge race (PR #2409 tightened the snapshot 9->8 against a base that had 8; PR #2413 legitimately added a 9th violation when main still pinned 9; the merge combined them). Main self-healed by bumping the snapshot back to 9. This PR is the honest follow-up: fix the checker so the recorded count is the true count, rather than leaving an inflated snapshot in place.Not in scope (deliberately)
An earlier revision of this PR also hoisted the backup-detail endpoint's one genuine inline closure (
_run_check_into_results) to a module-level helper, which would have taken minds to 6. Reverted: that closure captures five locals and is used once as a thread target, so the inline form reads better than a five-argument module-level function. It stays, and it is honestly counted in the 7.Verification
find_inline_functionsrecount: minds 7, imbue_common 3 (unchanged)._DIR: only minds changes.apps/minds+imbue_commoninline-function ratchets: pass at 7 and 3.core_test.py: 25 passed.ty checkclean on the changed source.🤖 Generated with Claude Code