Fix unused imports checker #1205
Conversation
There was a problem hiding this comment.
Thank you! Looks good! I left a couple of small comments inline, but I also wonder if a slightly different approach would be better.
Specifically, instead of modifying ProcReExport struct, we could add reexported_procs field to the ModuleImports struct. This would work similarly to invoked_procs field. That is, inside ParserContext::parse_reexported_procedure() we could add the re-exported procedure to module imports.
A potential benefit of this approach is that all info about imports and their usage is encapsulated in the ModuleImports struct, and we can determine if there are any unused imports by looking at only that struct.
assembly/src/ast/mod.rs
Outdated
| let import_lib_paths = import_info.import_paths(); | ||
| let invoked_procs_paths: Vec<&LibraryPath> = | ||
| import_info.invoked_procs().iter().map(|(_id, (_name, path))| path).collect(); | ||
| fn check_unused_imports(context: &ParserContext) { |
There was a problem hiding this comment.
We can probably specify the type for context as &super::ParserContext. This way, we won't get a warning under no-std compilation.
|
I made a new version of unused imports checker, but it contains serialization functions which work only with a Winterfell 0.8, so I'll push it after we merge #1234 |
Fumuran
force-pushed
the
andrew-unused-checker-fix
branch
from
a921638 to
ed5f10e
Compare
assembly/src/ast/mod.rs
Outdated
| let import_lib_paths = import_info.import_paths(); | ||
| let invoked_procs_paths: Vec<&LibraryPath> = | ||
| import_info.invoked_procs().iter().map(|(_id, (_name, path))| path).collect(); | ||
| let reexported_procs_paths = import_info.reexported_procs_paths(); | ||
|
|
||
| for lib in import_lib_paths { | ||
| if !invoked_procs_paths.contains(&lib) { | ||
| if !invoked_procs_paths.contains(&lib) && !reexported_procs_paths.contains(lib) { | ||
| event!(Level::WARN, "unused import: \"{}\"", lib); | ||
| } | ||
| } |
There was a problem hiding this comment.
Most of this function could go into ModuleImports, right? For example, we could have something like:
pub fn hash_unused_imports(&self) -> boolin ModuleImports, and then here, we'd check and print the warning.
assembly/src/ast/imports.rs
Outdated
| pub fn new( | ||
| imports: ImportedModulesMap, | ||
| invoked_procs: InvokedProcsMap, | ||
| reexported_procs_paths: Vec<LibraryPath>, |
There was a problem hiding this comment.
Should this be a BTreeSet?
Also, this contains module paths rather than procedure paths, right? If so, I'd call it reexported_modules.
assembly/src/ast/imports.rs
Outdated
| pub fn get_module_path(&self, module_name: &str) -> Option<LibraryPath> { | ||
| self.imports.get(&module_name.to_string()).cloned() | ||
| } |
There was a problem hiding this comment.
Why do we need to change this?
There was a problem hiding this comment.
I need to clone the module path to be able to insert in to the reexported_modules, otherwise I will try to mutate import_info, having immutable reference to it. I use it in the context.rs file
assembly/src/ast/imports.rs
Outdated
| /// Returns a reference to the vector of module paths from which some procedures were reexported. | ||
| pub(super) fn reexported_procs_paths(&self) -> &Vec<LibraryPath> { | ||
| &self.reexported_procs_paths | ||
| } |
There was a problem hiding this comment.
If we add hash_unsued_imports() method, we probably won't need this one.
Fumuran
force-pushed
the
andrew-unused-checker-fix
branch
2 times, most recently
from
fecf9ad to
a84b81d
Compare
Fumuran
force-pushed
the
andrew-unused-checker-fix
branch
2 times, most recently
from
defcd26 to
a992fe6
Compare
Fumuran
force-pushed
the
andrew-unused-checker-fix
branch
from
a992fe6 to
fe51dad
Compare
|
@Overcastan are you still working on this? |
|
I believe this has been implemented in #1277. cc @bitwalker |
|
Yes, this should be superseded by the changes in #1277, which will emit warnings for this. I haven't evaluated how well diagnostics get rendered via the CLI after merging that PR, but that would be a separate issue anyway. |
|
Superseded by #1277. |
This PR fixes the the unused import checker. After this update it will check not only invoked procedures, but also reexported procedures in modules.