[lint] Fix false positive in needless visibility checker#19481
[lint] Fix false positive in needless visibility checker#19481
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
vineethk
changed the title
There was a problem hiding this comment.
Pull request overview
Fixes a false positive in the needless_visibility Move linter by accounting for the post-inlining call graph when determining whether a function is only used within its defining module.
Changes:
- Add
FunctionEnv::get_using_functions_with_transitive_inline()to compute “effective callers” after inline expansion. - Update
needless_visibility’s shared helper to use the new transitive-inline caller computation. - Add a regression test ensuring package visibility isn’t flagged when the only direct caller is an inline function that’s reached cross-module.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| third_party/move/tools/move-linter/tests/model_ast_lints/needless_visibility_lint_fp_fix.move | New regression test reproducing the prior false positive scenario. |
| third_party/move/tools/move-linter/tests/model_ast_lints/needless_visibility_lint_fp_fix.exp | Expected output asserting no diagnostics are emitted. |
| third_party/move/tools/move-linter/src/model_ast_lints/unused_common.rs | Switch same-module-user check to use callers-after-inlining. |
| third_party/move/move-model/src/model.rs | Add cached computation of “using functions” with transitive inline expansion. |
| third_party/move/move-model/src/builder/module_builder.rs | Initialize the new per-function cache field in constructed FunctionData. |
Comments suppressed due to low confidence (1)
third_party/move/tools/move-linter/src/model_ast_lints/unused_common.rs:69
has_same_module_users_onlynow unconditionally callsget_using_functions_with_transitive_inline(), which will panic if call info isn’t available (it usesexpect("call info available")). Previously this function returnedfalsewhenget_using_functions()returnedNone. Consider preserving the non-panicking behavior by handling theNonecase (e.g., returningfalse/empty set) or by making the transitive-inline helper returnOptionand propagating it here.
let using_funs = func.get_using_functions_with_transitive_inline();
let func_qfid = func.get_qualified_id();
let func_module_id = func.module_env.get_id();
let mut has_non_self_user = false;
for user in using_funs.iter() {
if *user == func_qfid {
continue;
}
has_non_self_user = true;
if user.module_id != func_module_id {
return false;
}
}
has_non_self_user
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
👀
Differential Security Review — Move Linter: Fix false positive in needless visibility checker
PR: #19481 | Codebase size: SMALL | Risk classification: LOW (linter tool, not consensus/execution path)
Context
The PR adds FunctionEnv::get_using_functions_with_transitive_inline() — a reverse BFS that walks the caller graph upward through inline functions to find the "effective" non-inline callers after inlining. The needless_visibility checker's has_same_module_users_only is updated to use this new API.
The change is architecturally sound and the BFS logic is correct for the intended semantics. Cache invalidation in FunctionData::new, the module builder, and set_function_def is complete and symmetric with the existing used_functions_with_transitive_inline field.
Finding 1 — LOW: Panic regression vs. prior graceful degradation
Location: model.rs get_using_functions_with_transitive_inline → unused_common.rs has_same_module_users_only
The old has_same_module_users_only explicitly handled a None result from get_using_functions() by returning false:
let Some(using_funs) = func.get_using_functions() else {
return false;
};The new code calls get_using_functions_with_transitive_inline(), which internally calls:
for user in fnc.get_using_functions().expect("call info available") {get_using_functions() returns None whenever any function in the global environment has used_funs: None (it short-circuits on the first ? during the scan). This occurs for placeholder entries created via FunctionData::new() that never receive a definition — for example, functions in binary-only / dependency stubs loaded without source.
Concrete impact: If the linter is ever invoked on a model where not every function has used_funs populated (e.g. a dependency module in a mixed source+binary build), the needless_visibility checker will panic instead of silently returning false as it did before. No current exploit exists, but this is a silent interface contract change — the old API chose Option specifically to signal "call info not available."
Suggestion (not a required fix): Propagate the Option through get_using_functions_with_transitive_inline rather than using expect, or document and assert the precondition that used_funs is always populated by the time the linter runs.
Sent by Cursor Automation: Security Review Bot
vineethk
force-pushed
the
vk/fix-needless-visible-lint
branch
from
318d22a to
6badec4
Compare
vineethk
requested review from
calintat,
georgemitenkov,
rahxephon89,
vgao1996 and
wrwg
vineethk
force-pushed
the
vk/fix-needless-visible-lint
branch
from
b5ef59b to
e09d9c2
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
Description
needless_visibilitylint emitted false positives for package/friend functions whose only direct callers were inline functions in the same module — missing that post-inlining those functions are reachable from external modules. This bug was reported by @zzjas.Fixed by adding
get_using_functions_with_transitive_inline(), a dual to an existing functionget_used_functions_with_transitive_inline().How Has This Been Tested?
Added a new test that triggered a false positive before, but does not now.
Type of Change
Which Components or Systems Does This Change Impact?
Note
Medium Risk
Adjusts Move model call-graph caching and linter logic to treat inline functions as expanded at call sites; mistakes could change cross-module usage detection and lint output.
Overview
Fixes a
needless_visibilityfalse positive by teaching the Move model to compute “using functions after inline expansion”.This adds a new cached query
FunctionEnv::get_using_functions_with_transitive_inline()(and wiring/caches inFunctionData,module_builder, and cache invalidation) and updates the linter’shas_same_module_users_onlycheck to use it so inline-only direct callers don’t hide external reachability.Adds a regression test (
needless_visibility_lint_fp_fix.move/.exp) covering apackagefunction only called via aninlinewrapper that is invoked from another module.Reviewed by Cursor Bugbot for commit e09d9c2. Bugbot is set up for automated code reviews on this repo. Configure here.