Skip to content

Prelude & Edition 2015 import resolution #816

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Feb 13, 2019
Merged

Conversation

flodiebold
Copy link
Member

@flodiebold flodiebold commented Feb 12, 2019

I implemented the prelude import, but it turned out to be useless without being able to resolve any of the imports in the prelude 😅 So I had to add some edition handling and handle 2015-style imports (at least the simplified scheme proposed in rust-lang/rust#57745). So now finally Option resolves 😄

One remaining problem is that we don't actually know the edition for sysroot crates. They're currently hardcoded to 2015, but there's already a bunch of PRs upgrading the editions of various rustc crates, so we'll have to detect the edition somehow, or just change the hardcoding to 2018 later, I guess...

Also currently missing is completion for prelude names, though that shouldn't be hard to add. And Vec still doesn't resolve, so I need to look into that.

@flodiebold flodiebold requested a review from matklad February 12, 2019 22:11
// TODO there must be a nicer way to write this condition
PathKind::Plain | PathKind::Abs
if self.edition == Edition::Edition2015
&& (path.kind == PathKind::Abs || mode == ResolveMode::Import) =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative would be to handle this in the path desugaring layer: when converting ast::Path to hir::Path, check if the current path is use from 2015 crate and make it Abs.

On the one hand, this approach seems cleaner: we concentrate special cases in the "easy" de sugaring step, making the main logic simpler.

On the other hand, I believe we construct paths pretty frequently in the ide_api layer, and figuring edition there seems like some work.

Hm, having typed this, I now think that this PR actually does not work 100% correctly for the IDE case, because IDE uses ::Other mode even for paths in use items.

Do you think it makes sense to switch to de sugaring then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can't completely handle the edition during desugaring, because Abs paths are handled differently between the editions. So it seems better to me to keep all the logic for this in one place instead of distributing it between the two places...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good!

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
…te_graph macro
@flodiebold
Copy link
Member Author

Ok, I made extern crate work correctly in 2015 edition and now Vec resolves properly; and I also implemented completion from prelude 🎉
I also adapted the crate_graph macro to allow specifying the edition.

@matklad
Copy link
Member

matklad commented Feb 13, 2019

bors r+

bors bot added a commit that referenced this pull request Feb 13, 2019
816: Prelude & Edition 2015 import resolution r=matklad a=flodiebold

I implemented the prelude import, but it turned out to be useless without being able to resolve any of the imports in the prelude 😅 So I had to add some edition handling and handle 2015-style imports (at least the simplified scheme proposed in rust-lang/rust#57745). So now finally `Option` resolves 😄 

One remaining problem is that we don't actually know the edition for sysroot crates. They're currently hardcoded to 2015, but there's already a bunch of PRs upgrading the editions of various rustc crates, so we'll have to detect the edition somehow, or just change the hardcoding to 2018 later, I guess...

~Also currently missing is completion for prelude names, though that shouldn't be hard to add. And `Vec` still doesn't resolve, so I need to look into that.~

Co-authored-by: Florian Diebold <flodiebold@gmail.com>
@bors
Copy link
Contributor

bors bot commented Feb 13, 2019

Canceled

@flodiebold
Copy link
Member Author

bors r=matklad

bors bot added a commit that referenced this pull request Feb 13, 2019
816: Prelude & Edition 2015 import resolution r=matklad a=flodiebold

I implemented the prelude import, but it turned out to be useless without being able to resolve any of the imports in the prelude 😅 So I had to add some edition handling and handle 2015-style imports (at least the simplified scheme proposed in rust-lang/rust#57745). So now finally `Option` resolves 😄 

One remaining problem is that we don't actually know the edition for sysroot crates. They're currently hardcoded to 2015, but there's already a bunch of PRs upgrading the editions of various rustc crates, so we'll have to detect the edition somehow, or just change the hardcoding to 2018 later, I guess...

~Also currently missing is completion for prelude names, though that shouldn't be hard to add. And `Vec` still doesn't resolve, so I need to look into that.~

Co-authored-by: Florian Diebold <flodiebold@gmail.com>
@bors
Copy link
Contributor

bors bot commented Feb 13, 2019

Build succeeded

@bors bors bot merged commit 911e32b into rust-lang:master Feb 13, 2019
@flodiebold flodiebold deleted the prelude branch February 24, 2019 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants