-
Notifications
You must be signed in to change notification settings - Fork 286
feat(assembly): warn about unused constants #2993
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 4 commits
f4b7ef5
69b437e
939b412
5cc929c
fda2ca5
7fdaa28
49fca2c
c3ef64b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -171,6 +171,14 @@ pub fn analyze( | |
| } | ||
| } | ||
|
|
||
| // Check unused constants | ||
| analyzer.resolve_constant_usage(); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I could repro a small mismatch here with a local test. A program shaped like only emits The reason seems to be that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice catch - fixed in fda2ca5. The visit_mut_constant_ref path in VerifyInvokeTargets was bumping alias.uses unconditionally, so imports reached only through dead constants slipped past the unused-import check. Now constant exports record their import references as deferred edges in AnalysisContext instead of incrementing directly. After resolve_constant_usage() runs, only imports from live constants get credited. Moved the unused-import check after constant liveness resolution too. Added dead_constant_does_not_mask_unused_import to cover the exact scenario you described. |
||
| for constant in module.constants() { | ||
| if !analyzer.is_constant_used(constant) { | ||
| analyzer.error(SemanticAnalysisError::UnusedConstant { span: constant.span }); | ||
| } | ||
| } | ||
|
|
||
| analyzer.into_result().map(move |_| module) | ||
| } | ||
|
|
||
|
|
@@ -328,6 +336,7 @@ fn add_advice_map_entry( | |
| ConstantExpr::Word(Span::new(entry.span, WordValue(*key))), | ||
| ); | ||
| context.define_constant(module, cst); | ||
| context.mark_constant_used(&entry.name); | ||
| match module.advice_map.get(&key) { | ||
| Some(_) => { | ||
| context.error(SemanticAnalysisError::AdvMapKeyAlreadyDefined { span: entry.span }); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because
get()runs duringsimplify_constants(), this marksAas used even in a dead chain likeconst A = 1andconst B = Awhere neither constant is referenced from any procedure or exported API. In that case onlyBwarns andAis missed.To really address this, we need to track constant-to-constant edges here and compute reachability from real roots.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch - fixed in 5cc929c. Instead of marking constants as used during \get(), I now track constant-to-constant edges in a separate map and then do a BFS from real roots (procedure references + public constants) to figure out which ones are actually reachable. So \const A = 1; const B = A\ correctly warns on both if neither is referenced.
Added tests for both cases - both unused should warn, and transitive usage through a chain shouldn't.