Stream: t-compiler

Topic: uniform-paths


eddyb (Aug 10 2018 at 06:30, on Zulip):

@Vadim Petrochenkov hi! should've contacted you here sooner. anyway, I'm still trying to fix the fallout from some of my changes and I keep finding bugs :(

eddyb (Aug 10 2018 at 06:31, on Zulip):

so what I noticed is the use $crate; warning doesn't fire for use $crate::{self}; http://play.rust-lang.org/?gist=b4cf86e1f8bf5b2007165912f38afc57&version=stable&mode=debug&edition=2015

eddyb (Aug 10 2018 at 06:32, on Zulip):

I'm assuming I can just move that around so it always fires if the final path is just one $crate segment

eddyb (Aug 10 2018 at 06:33, on Zulip):

also, use $crate; was converted into use crate_name as crate_name; not use $crate as crate_name; which meant things could go wrong if the extern crate the macro came from was renamed :/ (also, it doesn't work same-crate at all)

eddyb (Aug 10 2018 at 06:35, on Zulip):

I made it a bit more uniform so now it can resolve same-crate (but if you don't rename the import you can't access it because it picks up the crate name from the root module and that's empty for the current crate). use $crate as foo; seems... reasonable IMO, it's only use $crate; that's bad because of the rename to $crate

eddyb (Aug 10 2018 at 06:38, on Zulip):

so one weird thing is I broke import-crate-var, it gives me:

error[E0425]: cannot find function `f` in module `import_crate_var`
  --> /home/eddy/Projects/rust-2/src/test/compile-fail/import-crate-var.rs:22:5
   |
LL |     m!();
   |     ^^^^^ not found in `import_crate_var`
   |
   = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
help: possible candidate is found in another module, you can import it into scope
   |
LL | use import_crate_var::f;
   |
eddyb (Aug 10 2018 at 06:39, on Zulip):

so what sort of weird module with f missing, is that? ohhhhhh it's not hitting the cross-crate code path, which means it's not getting populated, hah! (I'm rubber ducking now, I guess)

eddyb (Aug 10 2018 at 07:13, on Zulip):

hmm populate_module_if_necessary is not enough to be able to access items from the "module"

eddyb (Aug 10 2018 at 07:26, on Zulip):

ugh use $crate; probably only worked because it was basically use "" as ""; which is cyclical

nikomatsakis (Aug 10 2018 at 08:37, on Zulip):

@eddyb I'm not sure how often @Vadim Petrochenkov comes online here; internals is maybe best?

Vadim Petrochenkov (Aug 10 2018 at 09:46, on Zulip):

I'm not sure how often @Vadim Petrochenkov comes online here

Twice a day, in the morning and in the evening (GMT+3), same for internals.

Vadim Petrochenkov (Aug 10 2018 at 09:56, on Zulip):

@eddyb
Yes, single segment imports were already buggy for all special segment (self, super, $crate, now crate) - e.g. https://github.com/rust-lang/rust/issues/37156, https://github.com/rust-lang/rust/issues/35612.
I wanted to fix it this summer, but other stuff was higher priority.

use $crate; was a warning long enough, I think it can be made an error now if necessary.
The intended (at least by me, but it was also discussed in one of the module rfc threads with the same conclusion), but not yet implemented rule is that every import must have a name

use crate; // BAD, same for $crate
use crate as name; // GOOD, same for $crate
use super; // BAD
use super as name; // GOOD
use super::{self}; // BAD
use name::{self}; // GOOD, name can come from a previous segment for `self`
use super::{self as name}; // Also GOOD

and that implicit naming like use $crate -> use crate_name should never be done.

eddyb (Aug 10 2018 at 10:03, on Zulip):

I agree, although I think for now I'll try to do the simplest thing that works, since this is taking way too long

eddyb (Aug 10 2018 at 10:57, on Zulip):

@Vadim Petrochenkov do you have an opinion on https://github.com/rust-lang/rust/issues/53128#issuecomment-411991950? I have the "force extern_prelude on everyone" in my PR but I think I need to take it out to avoid having to rewrite all the tests, and I think I'll go with the second thing (i.e. test::foo doesn't imply ::test::foo unless test is in extern_prelude, for both ambiguity checking and actual resolution)

eddyb (Aug 10 2018 at 11:00, on Zulip):

I finally figured out how edition-lint-paths is supposed to work, heh, so you turn on some features from the new edition, without switching to it (so #![feature(rust_2018_preview)]is only a subset of --edition 2018's effects), and then you get a bunch of lints

Vadim Petrochenkov (Aug 10 2018 at 11:16, on Zulip):

Alternatively, we can make ::crate_name not work for any crate not in the extern_prelude, and still require extern crate

I think we can do that, at least initially.
This mostly affects proc_macros and private rustc crates, so this should be rare and shouldn't affect new users.

eddyb (Aug 10 2018 at 11:18, on Zulip):

also bench stuff I guess

eddyb (Aug 10 2018 at 11:18, on Zulip):

we can land it but maybe we should do it separately from the main PR, whereas everything else is needed for #![feature(uniform_paths)] to even work at all

Vadim Petrochenkov (Aug 10 2018 at 11:19, on Zulip):

In the "true uniform path" model imports are resolved as any non-imports, so local modules shadow extern crates and ::test vs self::test is not an error.
When it's implemented we can allow ::my_crate to refer to non-prelude crates again.

eddyb (Aug 10 2018 at 11:20, on Zulip):

FWIW I... have that implemented

eddyb (Aug 10 2018 at 11:21, on Zulip):

or, well, with one tiny caveat, single-segment imports have the preference order flipped

Vadim Petrochenkov (Aug 10 2018 at 11:22, on Zulip):

(Also, I should go right now, will answer later.)

eddyb (Aug 10 2018 at 11:22, on Zulip):

but the main part of the resolution involves treating import paths as relative paths

eddyb (Aug 10 2018 at 12:02, on Zulip):

@Vadim Petrochenkov so uhhh I think I actually implemented "try in self and only if it's not in there try an external crate". I'll put it in a separate commit from the ambiguity canary thing

eddyb (Aug 10 2018 at 13:13, on Zulip):

@Vadim Petrochenkov while I'm waiting for another build, I came up with this:

fn main() {
    mod test { pub struct Foo; }
    use test::Foo;
    use test as foo;
}

I don't know yet if #![feature(uniform_paths)] allows it (assuming the ambiguity canary doesn't trip, e.g. due to --extern test=... - cause that has explicit self which shouldn't allow reaching the inner module) but it might, and I think I know how to solve it:

Add another argument to resolve_path for the initial value of module, and pass Some(UniformRoot) for imports, in all editions.
This will ensure that single-segment imports don't go through a different codepath than the first segment of a multi-segment import.

eddyb (Aug 10 2018 at 14:09, on Zulip):

hmm resolve_ident_in_lexical_scope is a pretty bad thing to hit because it resolves absolutely if there are no ribs

eddyb (Aug 10 2018 at 17:31, on Zulip):

@Vadim Petrochenkov see the edit here: https://github.com/rust-lang/rust/pull/52923#issuecomment-412120808 - basically, since (I think) I'm much closer to your intended "uniform approach", we can probably get rid of the hacky ambiguity detection and instead just check if both things we wanted to resolve to, exist, while resolving them

maybe the UniformRoot(keywords::Invalid.name()) hack is too much of a hack and we should have two variants (or Option around the name), one for the "absolute mode" (use ::x::...;) and one for the "maybe-relative mode" (use x::...;), but other than that...

Vadim Petrochenkov (Aug 11 2018 at 00:17, on Zulip):

I'm much closer to your intended "uniform approach", we can probably get rid of the hacky ambiguity detection and instead just check if both things we wanted to resolve to, exist, while resolving them

The key issue with import (and macro) paths is that "whether they exist" is not a well-formed question.
It may be nonexistent at one moment of time when we check it, but then start to exist at later moment of time.
That's why resolve_lexical_macro_path_segment has to track all these "potentially ambiguous results" and work in two passes - one for initial resolution and another for validation (in fn finalize_current_module_macro_resolutions).

The self.resolve_ident_in_module_unadjusted(...).is_ok() in https://github.com/rust-lang/rust/pull/52923/commits/5e24cb92245ae7f124a55f5413c2e8b4ba25c12c falls exactly into this trap - is_ok can return false at one moment of time, but the local module can be defined after that.

Vadim Petrochenkov (Aug 11 2018 at 00:23, on Zulip):

The scheme with use cloning was supposed to avoid dealing with this more tricky stuff to get the result quicker, even if it's more conservative than strictly necessary.

Vadim Petrochenkov (Aug 11 2018 at 00:25, on Zulip):

I think the conclusion from this is that the canaries are still useful to conservatively error in all weird cases when resolve_ident_in_module_unadjusted returns false negative.

eddyb (Aug 11 2018 at 05:00, on Zulip):

Okay, thanks, that means I'll keep the current version of the canaries and note the fact that they're needed because the implementation isn't sophisticated enough to always pick self::x over ::x if the former eventually exists, wrt macro expansion

eddyb (Aug 11 2018 at 06:23, on Zulip):

@Vadim Petrochenkov heh, funnily enough this works as expected:

macro_rules! m {
    () => { mod std { pub struct Foo; } }
}
use std::{Foo, Bar};
m!();

(even though there's the ambiguity error, only Bar also gets a resolution failure, Foo succeeds)
So I'm guessing a problematic testcase would be more involved?

eddyb (Aug 11 2018 at 08:16, on Zulip):

@Vadim Petrochenkov Hmm I think I came up with a simpler ambiguity canary scheme: since we rely on extern_prelude for checking if we're supposed to resolve to ::x, ever, we can have only one canary import, for self::x, and in finalize_imports, ignore errors for these canaries and treat their success as ambiguity errors, instead of relying on a pair of them coexisting

eddyb (Aug 11 2018 at 08:18, on Zulip):

Furthermore, if we do it this way, it should be possible to have one canary for each of the non-module scopes ("non-normal modules" in rustc_resolve terminology) around the , to also error in a case like this:

enum Foo { A, B }
fn main() {
    enum Foo {}
    use /*self::*/Foo::*;
    let _ = (A, B);
}
eddyb (Aug 11 2018 at 08:19, on Zulip):

As one might expect that not writing self there, with #![feature(uniform_paths)], would resolve to the inner Foo, but self always resolves in the "normal module"

eddyb (Aug 14 2018 at 08:44, on Zulip):

@Vadim Petrochenkov woo, it landed! and it broke clippy & rls, sadly. r? https://github.com/rust-lang/rust/pull/53335

Josh Huber (Aug 15 2018 at 21:23, on Zulip):

Should bugs be filed for things like uniform-paths? I'm fairly sure this is a bug. At the very least It's confusing to me. Enabling uniform_paths, there's a detected ambiguous use for log. But I can't figure out where the non-crate log is coming from. Furthermore, if I change use log; to use self::log;, just to see if it'll work, the compiler claims there's no log module in the root. Weird, maybe?

https://play.rust-lang.org/?gist=f749f38540242c143f2e4c41e2f1625c&version=nightly&mode=debug&edition=2018

error: import from `log` is ambiguous
 --> src/main.rs:3:5
  |
3 | use log;
  |     ^^^
  |     |
  |     could refer to external crate `::log`
  |     could also refer to `self::log`
  |
  = help: write `::log` or `self::log` explicitly instead
  = note: relative `use` paths enabled by `#![feature(uniform_paths)]`
Josh Huber (Aug 15 2018 at 21:24, on Zulip):

after changing to use self::log;:

error[E0432]: unresolved import `self::log`
 --> src/main.rs:3:5
  |
3 | use self::log;
  |     ^^^^^^^^^ no `log` in the root

:)

simulacrum (Aug 15 2018 at 21:53, on Zulip):

@wootsuit Yes, if there's problems or inconsistencies please do file bugs

Josh Huber (Aug 15 2018 at 21:58, on Zulip):

Ok, I will! Thanks!

Josh Huber (Aug 15 2018 at 22:40, on Zulip):

Ok, makes sense now (thanks @eddyb :), and still surprising to me! Turns out, it's that the log crate has a log name exported, so it's the act of using log, or even log::log that's creating the ambiguity! https://github.com/rust-lang/rust/issues/53408

eddyb (Aug 15 2018 at 23:46, on Zulip):

oh sorry I didn't see this here (you need to write @eddyb and press enter to let it complete to @eddyb - only the latter notifies me)

eddyb (Aug 15 2018 at 23:47, on Zulip):

@uberjay ^^

Josh Huber (Aug 16 2018 at 00:59, on Zulip):

Ah, I didn’t realize! Thanks, again!

eddyb (Aug 16 2018 at 13:23, on Zulip):

@uberjay fix is up! https://github.com/rust-lang/rust/pull/53427

Josh Huber (Aug 16 2018 at 16:01, on Zulip):

So fast! I just tried it out (yes I am recreationally building the compiler :), and it seems to work well! :tada:

eddyb (Aug 18 2018 at 05:20, on Zulip):

Update: nightly now includes these Rust 2018 changes:

eddyb (Aug 18 2018 at 05:20, on Zulip):

@uberjay so you should be able to use nightly :D

eddyb (Aug 19 2018 at 14:42, on Zulip):

@Vadim Petrochenkov ugh, I just realized pub use crate_name; doesn't work with uniform_paths and that seems a bit bad

nikomatsakis (Aug 21 2018 at 17:30, on Zulip):

yeah, that is what I was worried about

simulacrum (Aug 21 2018 at 22:08, on Zulip):

Part of me wants to say that you shouldn't be doing that anyway because it's better to depend directly, in most cases.

This should work though, right? So there is a replacement:

pub mod crate_name {
    pub use ::crate_name::*;
}
nikomatsakis (Aug 21 2018 at 22:42, on Zulip):

@simulacrum pub use ::crate_name also works

nikomatsakis (Aug 21 2018 at 22:42, on Zulip):

it's just awkward

simulacrum (Aug 21 2018 at 22:42, on Zulip):

not.. too bad, though; I'd sort of presume that sort of thing is fairly rare

simulacrum (Aug 21 2018 at 22:42, on Zulip):

I wonder how difficult it would be to get statistics

nikomatsakis (Aug 21 2018 at 22:42, on Zulip):

presumably

nikomatsakis (Aug 21 2018 at 22:42, on Zulip):

it's a bit annoying because it's a "false ambiguity", in a way

simulacrum (Aug 21 2018 at 22:43, on Zulip):

sure, yeah -- the problem is that crate_name is already imported, right?

simulacrum (Aug 21 2018 at 22:43, on Zulip):

In theory we could ignore that for crates? We'd need to special-case it I guess

nikomatsakis (Aug 21 2018 at 22:44, on Zulip):

I haven't really thought that deeply about it, tbh

nikomatsakis (Aug 21 2018 at 22:44, on Zulip):

feels like we should model it with prolog and let a slg solver loose on it :P

nikomatsakis (Aug 21 2018 at 22:44, on Zulip):

/me only half joking

Vadim Petrochenkov (Aug 22 2018 at 00:05, on Zulip):

the problem is that crate_name is already imported, right?

That's implementation details leaking, not a fundamental restriction.

simulacrum (Aug 22 2018 at 00:39, on Zulip):

In theory I think the std prelude today is use std::prelude::* so I wonder if we can't do the same treatment here (redefining prelude imports is fine)

eddyb (Aug 22 2018 at 02:00, on Zulip):

Yeah this is fine, it's just the ambiguity canary that's externally visible.

eddyb (Aug 22 2018 at 02:05, on Zulip):

@Vadim Petrochenkov Do you think we should "just" detect the case where the use self::x; canary detected the import itself, and omit the canary error? That would allow use crate_name; but not use crate_name::{self}; (because of different ID)

eddyb (Aug 22 2018 at 02:06, on Zulip):

we can warn that it's unnecessary or whatever, if private, but at least it wouldn't error

Vadim Petrochenkov (Aug 22 2018 at 02:17, on Zulip):

@Vadim Petrochenkov Do you think we should "just" detect the case where the use self::x; canary detected the import itself, and omit the canary error? That would allow use crate_name; but not use crate_name::{self}; (because of different ID)

I think we can do that in short term.

eddyb (Aug 22 2018 at 02:18, on Zulip):

I kept trying to come up with nice ways of catching the relevant cases but allowing them seems easier than having any sort of coherent error (and the errors don't make sense if there's a re-export - for whatever reason)

eddyb (Aug 23 2018 at 06:53, on Zulip):

@Vadim Petrochenkov what if we inject the canaries on Rust2015 with #![feature(uniform_paths)] and warn about writing use ::crate_name when they'd get an ambiguity error after they switch version? they don't create name conflicts anymore, and have their own error reporting, so on non-Rust2018 we can just emit a lint instead, including automatically applicable diagnostics, I think (in the case where you wrote use name::... and not use {name::: - wait, hang on, use {::std::io} works with uniform_paths lol, that's nice)

eddyb (Aug 23 2018 at 07:08, on Zulip):

we do need a way to know whether rustfix needs to add :: or not, to refer to crates, so it's actually a must. and we have to fix rustfmt to not strip leading :: if uniform_paths is enabled

we could also add canaries for ::foo and self::foo and basically check whether an ambiguity error would be emitted and just suggest removing the :: / self:: if they don't trigger, so you can just run rustfix!

eddyb (Aug 23 2018 at 07:13, on Zulip):

hang on, how did I not notice this before: because of how the canaries work now, we gather N of them, plus an implicit external_prelude one and then require that only one succeeds. if we make the external_prelude one an explicit ::name canary, then we can actually have uniform_paths on Rust2015, modulo ambiguities from macro expansion shenanigans (but the canaries would catch those!).

We just have to be careful to not do explicit ::name in Rust2018 because that would forcefully load an extern crate.

Also, maybe only use ::foo; should do the "forceful" thing, and ::foo paths elsewhere. I know it's not consistent but it's closer to requiring --extern foo and having to put that in Cargo.toml. although, we could just implement the Cargo.toml thing now. does rustfix know how to edit Cargo.toml? Because that would be grand :D

eddyb (Aug 23 2018 at 07:18, on Zulip):

awww just saw https://github.com/rust-lang/rust/issues/53130#issuecomment-415217754

Last update: Nov 16 2019 at 01:05UTC