Resolve core types #2201
Resolve core types #2201
Conversation
This adds support for completion and goto definition of types defined within the "core" crate. The core crate is added as a dependency to each crate in the project. The core crate exported it's own prelude. This caused now all crates to inherit the core crates prelude instead of the std crates. In order to avoid the problem the prelude resolution has been changed to overwrite an already resolved prelude if this was set to a crate named core - in order to pick a better prelude like std. Fixes rust-lang#2199
| let map = db.crate_def_map(dep.crate_id); | ||
| if map.prelude.is_some() { | ||
| def_map.prelude = map.prelude; | ||
| prelude_is_core = dep.name == "core"; |
There was a problem hiding this comment.
Let me know if this is the right thing to do, or whether a string comparison is not good enough. As far as I can see the crate id for libcore is not available in this method. But I think since this compares crate names and those have to be unique it should be ok?
| if def_map.prelude.is_none() { | ||
| // If the prelude is the "core" prelude, try to replace it with a higher | ||
| // level prelude (e.g. "std") if available. | ||
| if def_map.prelude.is_none() || prelude_is_core { |
There was a problem hiding this comment.
This is obviously special-casing core. Wondering if something more universal would apply. Like
- checking if the crate which defines the currently set prelude is a dependency of the next checked dependency
- if yes replace the prelude with the one from that crate
However that also seems wrong, since most other things are dependent on std, which means we would always replac the std prelude with something else? Don't know how the stdlib and compiler are handling that, and whether special casing is the way to go here.
There was a problem hiding this comment.
Looking more through the code and trying to understand the difference between def_map.prelude and def_map.extern_prelude: Is it actually a good idea to populate .prelude (which is a special case) by populating through the full list of dependencies? How is it actually made sure that only sysroot crates have the prelude field set to begin with?
There was a problem hiding this comment.
I think what rustc does is just to always let the prelude be overwritten, and rely on the fact that it adds std after core. I think that's what we should do as well.
Is it actually a good idea to populate .prelude (which is a special case) by populating through the full list of dependencies?
Sysroot crates are just normal crates that are added to the crate graph in a special way. I don't think we should have special casing for them in other places; rustc doesn't either, as far as I know.
How is it actually made sure that only sysroot crates have the prelude field set to begin with?
Using the #[prelude_import] attribute requires the prelude_import feature, which will never be stable (but we don't check features, of course). Apart from that, nothing: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=09fade7ca53f3a9dd875da7f42751bdf
the difference between
def_map.preludeanddef_map.extern_prelude
The extern prelude is a different thing btw.; it's the set of external crates that are in scope everywhere, as described here: https://doc.rust-lang.org/reference/items/extern-crates.html#extern-prelude
There was a problem hiding this comment.
Thanks for the pointers here. I wasn't aware that #[prelude_import] is only a std thing.
and rely on the fact that it adds std after core. I think that's what we should do as well.
Actually it IS added after core. But the prelude is only picked once if is hasn't been set before. So we can go for 1 of 2 routes from here:
- Change the order to add the
stddependency beforecore - Always overwrite the prelude if a later dependency also has one configured
As far as I understand you the second option might be closer to what std is doing?
There was a problem hiding this comment.
Btw: The last doc you mentioned describes
The core crate is always added to the extern prelude. The std crate is added as long as the no_std attribute is not specified in the crate root.
That sounds like rustc merges all preludes into the same set?
There was a problem hiding this comment.
As far as I understand you the second option might be closer to what std is doing?
Yep, that's what I meant by "always let the prelude be overwritten", directly before the part you quoted 😉
That sounds like rustc merges all preludes into the same set?
This is about the extern prelude, which is a different thing. It just means that you can always use paths starting with core and std.
There was a problem hiding this comment.
Ok. I will then push an update which keeps the dependency order and always overwrites the prelude with later defined ones
| } | ||
| } | ||
| } | ||
| if let Some(core) = libcore { |
There was a problem hiding this comment.
Playing around a bit with this, just changing the dependency order here also seems to fix the prelude issue. std is picked up before core. However I think that's a less stable solution, since I'm not sure if the dependencies are really an ordered list which is guaranteed to be stable. It normally shouldn't be necessary, so not betting on it is likely better.
There was a problem hiding this comment.
I agree with Florian that we should just rely on dependency order here, rather than special case the core name
This removes the special casing for the "core" prelude. Whenever a later dependency also exports a prelude, it will replace the formerly imported prelude. The utilized prelude then depends purely on import order.
|
bors r+ Thanks! |
2201: Resolve core types r=matklad a=Matthias247 This adds support for completion and goto definition of types defined within the "core" crate. The core crate is added as a dependency to each crate in the project. The core crate exported it's own prelude. This caused now all crates to inherit the core crates prelude instead of the std crates. In order to avoid the problem the prelude resolution has been changed to overwrite an already resolved prelude if this was set to a crate named core - in order to pick a better prelude like std. Fixes #2199 Co-authored-by: Matthias Einwag <[email protected]>
Build succeeded |
3 participants
This adds support for completion and goto definition of
types defined within the "core" crate. The core crate is
added as a dependency to each crate in the project.
The core crate exported it's own prelude. This caused
now all crates to inherit the core crates prelude instead
of the std crates. In order to avoid the problem the
prelude resolution has been changed to overwrite
an already resolved prelude if this was set to a crate
named core - in order to pick a better prelude like std.
Fixes #2199