Implement renaming for Functions, Constants and Type Variants #4282
Conversation
GearsDatapacks
force-pushed
the
cross-module-value-renaming
branch
2 times, most recently
from
8e78f2c to
d4c87d0
Compare
GearsDatapacks
force-pushed
the
cross-module-value-renaming
branch
from
d4c87d0 to
1f931b9
Compare
|
Ok, this is ready for review now. I'm not super happy with the way I fixed the module access shadowing bug, but I couldn't think of another way to do it. Let me know if you have any thoughts. |
| import mod.{Constructor} | ||
|
|
||
| pub fn main() { | ||
| #(Constructor(75), mod.Constructor(57)) |
There was a problem hiding this comment.
I wonder if when renaming a constructor that has been imported in an unqualified fashion it should alias the import rather than rename it globally.
There was a problem hiding this comment.
Yeah, I wasn't sure about this. Maybe we can ask in Discord and hear other people's opinions on it.
| @@ -1064,6 +1073,8 @@ impl<A, B, C, E> Definition<A, B, C, E> { | |||
| #[derive(Debug, Clone, PartialEq, Eq)] | |||
| pub struct UnqualifiedImport { | |||
| pub location: SrcSpan, | |||
| /// The location excluding the potential `as ...` clause, or the `type` keyword | |||
| pub imported_name_location: SrcSpan, | |||
There was a problem hiding this comment.
I think the start here is redundant as it's the same as location, it could be a single position int instead.
There was a problem hiding this comment.
Actually in the following case: import some_module.{type SomeType as SomeAlias}, the full location contains type SomeType as SomeAlias, whereas the imported_name_location only contains SomeType. Therefore, it is completely separate information to the full location.
| change_annotations: None, | ||
| }; | ||
|
|
||
| for module in modules.values() { |
There was a problem hiding this comment.
Perhaps we should have an import graph so we can get these modules directly rather than traversing everything? Probably doesn't matter. Something for later perhaps.
That aside, this is a hashmap of modules that have been compiled, it does not include any that were loaded from the cache. This means there will be no renaming in any module that have not been recompiled in that editor session!
There was a problem hiding this comment.
Yeah that makes sense. I guess that means we need to store reference information in the module interface and cache it then.
As for an import graph, I'm happy to use one if you want; not exactly sure what that data structure would look like, or how we would build it and where it would be stored.
compiler-core/src/ast.rs
Outdated
| #[derive(Debug, Clone, PartialEq, Eq, Default)] | ||
| pub struct References { | ||
| pub imported_modules: HashSet<EcoString>, | ||
| pub value_references: HashMap<(EcoString, EcoString), Vec<SrcSpan>>, |
There was a problem hiding this comment.
It seems like it may be challenging to identify unused code with this data structure, and replacing the current broken usage tracking system is one of the main goals of the call graph work. Did you have thoughts on how that'd be done?
There was a problem hiding this comment.
That's true, I wasn't thinking too much about the current usage tracking. We already generate an in-module call-graph for dependency ordering, so would it be possible to use just that for usage tracking? We can't use that for renaming because it doesn't track cross-module usage.
|
Ok, I have now added the reference information to the cache so we can rename values in cached modules. |
This PR closes #3259
This is pretty much ready, except for two things: