First attempt at fixing #20591 #24818
Conversation
This isn't quite right, but it's interesting.
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
Oh, should have r? nrc since he was mentoring issue #20591 |
It compiles, yay.
Still compiling, but I think I have it!
I don't this we need to print the definition of the imported value too, though it's quite possible.
tbelaire
changed the title
|
Hi @tbelaire, is this ready for review now? (I couldn't work out from the conversation, sorry). |
|
Yup, sorry, I was nattering a lot. |
| TypeNS => "type", | ||
| ValueNS => "value", | ||
| }, | ||
| use syntax::ast_map::NodeItem; |
There was a problem hiding this comment.
either use a qualified path for NodeItem, or move this import to the top of the file. Not really worth having it here
|
Alright, should be all fixed, I just finished recompiling and testing. |
|
@nrc This is ready to land, looks good? |
|
@tbelaire sorry it took a while - I was on vacation last week |
This isn't quite right, but it's interesting.
| span_err!(self.resolver.session, import_span, E0252, "{}", &msg[..]); | ||
| let use_id = import_resolution.id(namespace); | ||
| let item = self.resolver.ast_map.expect_item(use_id); | ||
| // item is syntax::ast::Item; |
There was a problem hiding this comment.
Nit: I'd prefer a type annotation in code rather than in a comment.
(I won't block this PR on that nit; it's more a note for the future...)
There was a problem hiding this comment.
I always feel so weirded out when just annotating the type is what forces an import. I went back and forth on that line, it felt strange to be the only line importing from syntax::map.
Just as a style note, I feel like it'd be easier to navigate the code if even more type annotations were placed, (or I had a IDE which could tell me the type on mouseover). But I'm sure the core team already knows the types of most of the functions, so it's unnecessary for them, and in fact, just extra noise.
Which way should I favour in general? I'm hesitant to break with the established style, but I feel it'd be helpful to the newer contributors to add a fair number of annotations.
There was a problem hiding this comment.
DXR, which should be coming online soon, does type on mouse-over for most variables.
I'd probably remove the comment and not add an annotation, tbh - I think a programmer should probably expect the result of expect_item to be an Item.
There was a problem hiding this comment.
Aye, but what kind of item? syntax ::map isn't even imported.
I know I had to open up a number of files side by side and carefully trace through struct and function definitions to pin down some types in this block. A simple type annotation or two would have saved me at least 10 minutes of tracking things down.
Currently, for `use` declarations with multiple paths, only the `use` item itself is saved in the AST map, not the individual path nodes. This can lead to a problem when a span of a specific path node is needed. For example, rust-lang#24818 caused an ICE because of this, in `ImportResolver::check_for_conflicting_import()`. Fixes rust-lang#25763.
Currently, for `use` declarations with multiple paths, only the `use` item itself is saved in the AST map, not the individual path nodes. This can lead to a problem when a span of a specific path node is needed. For example, rust-lang#24818 caused an ICE because of this, in `ImportResolver::check_for_conflicting_import()`. Fixes rust-lang#25763.
Currently, for `use` declarations with multiple paths, only the `use` item itself is saved in the AST map, not the individual path nodes. This can lead to a problem when a span of a specific path node is needed. For example, #24818 caused an ICE because of this, in `ImportResolver::check_for_conflicting_import()`. Fixes #25763.
Currently, for `use` declarations with multiple paths, only the `use` item itself is saved in the AST map, not the individual path nodes. This can lead to a problem when a span of a specific path node is needed. For example, rust-lang#24818 caused an ICE because of this, in `ImportResolver::check_for_conflicting_import()`. Fixes rust-lang#25763.
This isn't quite right, but it's interesting.