-
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 all 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 |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| use alloc::{ | ||
| boxed::Box, | ||
| collections::{BTreeMap, BTreeSet}, | ||
| collections::{BTreeMap, BTreeSet, VecDeque}, | ||
| sync::Arc, | ||
| vec::Vec, | ||
| }; | ||
|
|
@@ -17,6 +17,16 @@ use crate::ast::{ | |
| /// This maintains the state for semantic analysis of a single [Module]. | ||
| pub struct AnalysisContext { | ||
| constants: BTreeMap<Ident, Constant>, | ||
| used_constants: BTreeSet<Ident>, | ||
| /// Edges from a constant to the constants it references. | ||
| /// Used to compute transitive usage from real roots. | ||
| constant_deps: BTreeMap<Ident, BTreeSet<Ident>>, | ||
| /// Edges from a constant to the imports it references. | ||
| /// Used to avoid marking imports as used when only dead constants reference them. | ||
| constant_import_refs: BTreeMap<Ident, BTreeSet<alloc::string::String>>, | ||
| /// When set, references are recorded as constant-to-constant edges | ||
| /// rather than marking the target as directly used. | ||
| simplifying_constant: Option<Ident>, | ||
| imported: BTreeSet<Ident>, | ||
| procedures: BTreeSet<ProcedureName>, | ||
| errors: Vec<SemanticAnalysisError>, | ||
|
|
@@ -38,6 +48,14 @@ impl constants::ConstEnvironment for AnalysisContext { | |
| #[inline] | ||
| fn get(&mut self, name: &Ident) -> Result<Option<CachedConstantValue<'_>>, Self::Error> { | ||
| if let Some(constant) = self.constants.get(name) { | ||
| let is_self_ref = self.simplifying_constant.as_ref() == Some(name); | ||
| if !is_self_ref { | ||
| if let Some(ref parent) = self.simplifying_constant { | ||
| self.constant_deps.entry(parent.clone()).or_default().insert(name.clone()); | ||
| } else { | ||
| self.used_constants.insert(name.clone()); | ||
| } | ||
| } | ||
| Ok(Some(CachedConstantValue::Miss(&constant.value))) | ||
| } else if self.imported.contains(name) { | ||
| // We don't have the definition available yet | ||
|
|
@@ -67,6 +85,10 @@ impl AnalysisContext { | |
| pub fn new(source_file: Arc<SourceFile>, source_manager: Arc<dyn SourceManager>) -> Self { | ||
| Self { | ||
| constants: Default::default(), | ||
| used_constants: Default::default(), | ||
| constant_deps: Default::default(), | ||
| constant_import_refs: Default::default(), | ||
| simplifying_constant: None, | ||
| imported: Default::default(), | ||
| procedures: Default::default(), | ||
| errors: Default::default(), | ||
|
|
@@ -98,6 +120,76 @@ impl AnalysisContext { | |
| self.imported.insert(name); | ||
| } | ||
|
|
||
| /// Returns true if the constant has been referenced by another constant or | ||
| /// by a procedure body, or is publicly visible. | ||
| pub fn is_constant_used(&self, constant: &Constant) -> bool { | ||
| constant.visibility.is_public() || self.used_constants.contains(&constant.name) | ||
| } | ||
|
|
||
| /// Mark a constant as used. | ||
| /// | ||
| /// This is used for constants created as a side effect of other declarations | ||
| /// (e.g. advice map entries) that should not trigger unused constant warnings. | ||
| pub fn mark_constant_used(&mut self, name: &Ident) { | ||
| self.used_constants.insert(name.clone()); | ||
| } | ||
|
|
||
| /// Record that constant `constant_name` references import `import_name`. | ||
| /// | ||
| /// These edges are used to defer import-usage bookkeeping until after | ||
| /// constant liveness has been resolved, so that imports reached only from | ||
| /// dead constants are correctly reported as unused. | ||
| pub fn record_constant_import_ref( | ||
| &mut self, | ||
| constant_name: &Ident, | ||
| import_name: alloc::string::String, | ||
| ) { | ||
| self.constant_import_refs | ||
| .entry(constant_name.clone()) | ||
| .or_default() | ||
| .insert(import_name); | ||
| } | ||
|
|
||
| /// Increment `alias.uses` only for imports that are referenced by live | ||
| /// constants. Must be called after `resolve_constant_usage`. | ||
| pub fn apply_live_constant_import_refs(&self, module: &mut Module) { | ||
| for (constant_name, import_names) in &self.constant_import_refs { | ||
| if !self.used_constants.contains(constant_name) { | ||
|
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. The liveness check here skips That means Marking public constants as live before applying deferred import refs, and adding a regression test for |
||
| continue; | ||
| } | ||
| for import_name in import_names { | ||
| if let Some(alias) = | ||
| module.aliases_mut().find(|a| a.name().as_str() == import_name.as_str()) | ||
| { | ||
| alias.uses += 1; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Propagate usage from real roots through constant-to-constant edges. | ||
| /// | ||
| /// A constant is considered truly used if it is public, directly referenced | ||
| /// by a procedure body, or transitively referenced by such a constant. | ||
| /// This must be called before checking for unused constants. | ||
| pub fn resolve_constant_usage(&mut self) { | ||
| let mut worklist: VecDeque<Ident> = self.used_constants.iter().cloned().collect(); | ||
| for (name, constant) in &self.constants { | ||
| if constant.visibility.is_public() && self.used_constants.insert(name.clone()) { | ||
| worklist.push_back(name.clone()); | ||
| } | ||
| } | ||
| while let Some(name) = worklist.pop_front() { | ||
| if let Some(deps) = self.constant_deps.get(&name) { | ||
| for dep in deps { | ||
| if self.used_constants.insert(dep.clone()) { | ||
| worklist.push_back(dep.clone()); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Define a new constant `constant` | ||
| /// | ||
| /// Returns `Err` if a constant with the same name is already defined | ||
|
|
@@ -133,6 +225,7 @@ impl AnalysisContext { | |
| let constants = self.constants.keys().cloned().collect::<Vec<_>>(); | ||
|
|
||
| for constant in constants.iter() { | ||
| self.simplifying_constant = Some(constant.clone()); | ||
| let expr = ConstantExpr::Var(Span::new( | ||
| constant.span(), | ||
| PathBuf::from(constant.clone()).into(), | ||
|
|
@@ -145,6 +238,7 @@ impl AnalysisContext { | |
| self.errors.push(err); | ||
| }, | ||
| } | ||
| self.simplifying_constant = None; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,9 @@ pub struct VerifyInvokeTargets<'a> { | |
| module: &'a mut Module, | ||
| procedures: &'a BTreeSet<Ident>, | ||
| current_procedure: Option<ProcedureName>, | ||
| /// When visiting a constant export, holds the constant's name so that | ||
| /// import references can be deferred until constant liveness is resolved. | ||
| current_constant: Option<Ident>, | ||
| invoked: BTreeSet<Invoke>, | ||
| } | ||
|
|
||
|
|
@@ -39,9 +42,15 @@ impl<'a> VerifyInvokeTargets<'a> { | |
| module, | ||
| procedures, | ||
| current_procedure, | ||
| current_constant: None, | ||
| invoked: Default::default(), | ||
| } | ||
| } | ||
|
|
||
| /// Set the constant name whose export is currently being visited. | ||
| pub fn set_current_constant(&mut self, name: Option<Ident>) { | ||
| self.current_constant = name; | ||
| } | ||
| } | ||
|
|
||
| impl VerifyInvokeTargets<'_> { | ||
|
|
@@ -110,6 +119,11 @@ impl VerifyInvokeTargets<'_> { | |
| } | ||
| } | ||
| fn track_used_alias(&mut self, name: &Ident) { | ||
| // A locally-defined constant with the same name shadows any import of that name, | ||
| // so the import should not be credited as used in that case. | ||
| if self.analyzer.get_constant(name).is_ok() { | ||
| return; | ||
| } | ||
| if let Some(alias) = self.module.aliases_mut().find(|a| a.name() == name) { | ||
| alias.uses += 1; | ||
| } | ||
|
|
@@ -281,11 +295,25 @@ impl VisitMut for VerifyInvokeTargets<'_> { | |
| } | ||
| fn visit_mut_constant_ref(&mut self, path: &mut Span<Arc<Path>>) -> ControlFlow<()> { | ||
| if let Some(name) = path.as_ident() { | ||
| self.track_used_alias(&name); | ||
| } else if let Some((module, _)) = path.split_first() | ||
| && let Some(alias) = self.module.aliases_mut().find(|a| a.name().as_str() == module) | ||
| { | ||
| alias.uses += 1; | ||
| if let Some(ref const_name) = self.current_constant { | ||
|
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. Could we check that Local constants can reuse an imported alias name:
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. Fixed in 3e09839. The guard is in two places in
Added a regression test ( |
||
| // Only defer as an import ref if this identifier is not a local constant. | ||
| // A local constant shadows any same-named import, so the import gets no credit. | ||
| if self.analyzer.get_constant(&name).is_err() { | ||
| self.analyzer.record_constant_import_ref(const_name, name.as_str().into()); | ||
| } | ||
| } else { | ||
| self.track_used_alias(&name); | ||
| } | ||
| } else if let Some((module, _)) = path.split_first() { | ||
| if let Some(ref const_name) = self.current_constant { | ||
| // Defer: record the edge so we only credit the import when this | ||
| // constant is proven live. | ||
| self.analyzer.record_constant_import_ref(const_name, module.into()); | ||
| } else if let Some(alias) = | ||
| self.module.aliases_mut().find(|a| a.name().as_str() == module) | ||
| { | ||
| alias.uses += 1; | ||
| } | ||
| } | ||
| ControlFlow::Continue(()) | ||
| } | ||
|
|
||
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.