Changes required to help facilitate a refactoring tool #27493
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @huonw (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
r? @nrc |
|
cc @rust-lang/compiler - some minor changes to resolve to make it usable from external tools. Anyone passionately object to this? Or can think of a better way to offer the facility? |
| let mut loader = self.file_loader.borrow_mut(); | ||
| *loader = file_loader; | ||
| } | ||
|
|
There was a problem hiding this comment.
Glad to see someone made use of this before I got back to my IDE-like project!
Any specific reason why you need to swap the file loader after initialization?
There was a problem hiding this comment.
Mainly because it seemed to be an issue trying to plumb down the file loader through the session in the compiler API, including modifying the entry point. The remaining fields for the codemap follow similar access, so it didn't seem too extreme.
There was a problem hiding this comment.
Don't you need a custom diagnostics handler? In my custom rustc driver I had to create everything myself, so it didn't seem unreasonable to have to pass the file loader when creating the CodeMap.
There was a problem hiding this comment.
Using a different entry point, but I see what you've done. I think I can remove this change, although I need to add some other changes to my code to do the same thing.
|
Not sure how I feel about a callback being employed, what's the intended usecase here? |
|
@eddyb |
|
@GSam Why would you have to replicate anything? EDIT: It seems I had forgotten about rib scopes. My bad, I'll ponder on this some more, but if you don't hear from me, feel free to proceed with this approach. |
|
So, this requirement seems well-served by the plan that @nrc and I have kicked about for separating resolve into one component which builds up "name tables", indicating what identifiers are imported into modules, and one component for resolving paths. That service would be readily exportable, since the rest of the compiler would have to use it as well. I haven't looked at the changes in this particular patch -- but in general I don't have objections except that there is the usual caveat that everything in the compiler is hyper unstable and hence plugin authors should be prepared for us to tear it all up and throw it all out. |
| @@ -1110,6 +1140,11 @@ pub struct Resolver<'a, 'tcx:'a> { | |||
|
|
|||
| used_imports: HashSet<(NodeId, Namespace)>, | |||
| used_crates: HashSet<CrateNum>, | |||
|
|
|||
| // Callback function for intercepting walks | |||
There was a problem hiding this comment.
I think some more docs are in order. For example, what is the purpose of this boolean flag?
There was a problem hiding this comment.
It's to allow the walker to fall out while preserving the ribs (to call resolve_path afterwards). I've added a few more comments now.
Sets a flag to pass through the rest of the walker.
|
@nikomatsakis I believe it would be more of a "scope tree" due to blocks in functions being able to have their own imports (and then there's all the type parameters and whatnot, adding identifiers to ribs). I really want something like that to replace the hacky "trait map" (which maps certain expressions to traits in scope those expressions might use) and eventually allow for cleaner error messages, printing types with the shortest form available in scope. |
Yes. |
| if callback(ast_map::Node::NodeLocal(&*local.pat), &mut self.resolved) { | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you use a macro for this repeated idiom?
|
lgtm, r+ with a macro |
|
Not too familiar with macros, is the new patch sort of what you wanted? |
|
📌 Commit fc9ecae has been approved by |
In order to test the validity of identifiers, exposing the name resolution module is necessary. Other changes mostly comprise of exposing modules publicly like parts of save-analysis, so they can be called appropriately.
resolve: improve diagnostics and lay groundwork for resolving before ast->hir This PR improves diagnostics in `resolve` and lays some groundwork for resolving before ast->hir. More specifically, - It removes an API in `resolve` intended for external refactoring tools (see #27493) that appears not to be in active use. The API is incompatible with resolving before ast->hir, but could be rewritten in a more compatible and less intrusive way. - It improves the diagnostics for pattern bindings that conflict with `const`s. - It improves the diagnostics for modules used as expressions (fixes #33186). - It refactors away some uses of the hir map, which is unavavailable before ast->hir lowering. r? @eddyb
In order to test the validity of identifiers, exposing the name resolution module is necessary. Other changes mostly comprise of exposing modules publicly like parts of save-analysis, so they can be called appropriately.