Turn extern crate and use into first-class items.

[eddyb]

This implements an important part of RFC PR 385 (see #18219).

[rust-highfive]

r? @huonw

(rust_highfive has picked a reviewer for you, use r? to override)

[alexcrichton]

These checks seem to have gone away with no replacement, was that intentional?

[eddyb]

They're duplicated in the item checking path, though they use a slightly different error message.

[alexcrichton]

Nice! I'll review the parser changes in more detail soon, but everything else looked pretty good to me (just one small nit). Some tests I'd like to see as well:

• Mixing imports/items
• Mixing imports/extern crate/items
• Shadowing a previously declared item with an extern crate or import (just to see what happens)
• Imports in the middle of a block expression

[eddyb]

@alexcrichton r? this has had rustdoc fixed for a while - tests are still missing but @Kimundi has expressed interest in working on that while I deal with other things.

[alexcrichton]

Can you update the description to not close #18219? The changes there are quite extensive and look like they go beyond just the changes made here.

[alexcrichton]

I'm also pretty worried about allowing view items anywhere within blocks just yet. The RFC says that the original reason for the ordering restriction was due to the way shadowing was implemented (to make it more clear), but with shadowing now removed the semantics are clear not matter what order items are in.

I'm not sure this is true for blocks though:

let foo = 3;
use bar::foo;
println!("{}", foo); // what does this print?

I think that lifting the restriction for items are fine, but due to our allowance of shadowing with local variables, I don't think we should lift the restriction in expressions.

[alexcrichton]

I also do not want to land this without any tests, new features like this really do need to be extensively tested so we don't have to hold up a release (for an otherwise backwards-compatible change) with another blocking issue while we fix a bug that may have been turned up with tests.

[eddyb]

I think I would handle such ordering restrictions in resolve, since I expect that's where the proper logic will be -eventually- implemented.
Does that make sense, @alexcrichton?
cc @nikomatsakis

[Kimundi]

As far as I know, those ordering restrictions don't apply to items, as those are mutually recursive visible:

// In a block:

static FOO ... // visible before and after
use Bar ... // visible before and after
extern crate Baz ... // visible before and after
fn f() ... // visible before and after
let x ... // visible _after_

So, what a construct like let foo; use foo means in regard to shadowing depends on what items in those positions are supposed to mean in general, and is not specific to view items.

[eddyb]

Ah, this causes the weird "only irrefutable patterns allowed here" error (can we stop matching constants in irrefutable patterns already?):

fn main() {
    let foo = 5u;
    const foo: uint = 42;
    println!("{}", foo);
}

And when that is not an issue, it seems that local bindings shadow items:

fn main() {
    let foo = || 5u;
    fn foo() -> uint { 42 }
    println!("{}", foo()); // prints 5
}

[alexcrichton]

This is partly why I would like to be conservative here and lift the restriction at a later date. I don't want to allow rearranging items hoping for changes to resolve to land soon, I would rather land the two at the same time.

@Kimundi has a good point though that other items are already plagued by this unfortunate shadowing, so it may not be as dire as I think, but I'd prefer to stem the flow of awkward questions rather than pile more on.

[Kimundi]

I guess the forward compatible thing would be to either artificially keep the order restriction in blocks, or to make local bindings with the same name as an item in the same flat scope a hard error.

---

This is of course not the place to discuss those things, but I guess the only "sane" semantic for items in blocks in general is the current situation: items are visible everywhere in a scope, but locals, having the property of allowing forward-only shadowing of elements in the same scope, can shadow them.

Otherwise you get weird inconsistent semantic between those two entities:

• items: shadowable with scope only, visible forward and backwards
• locals: shadowable with scope and later locals, visible forward-only

[eddyb]

@alexcrichton now that alpha's over, I've rebased this branch and added that error you requested, in the last commit. r?

[alexcrichton]

I think this may have snuck back in

[eddyb]

Oops, indeed, a few of these were merged automatically during the rebase.

[alexcrichton]

Alright, this all looks great to me, thanks @eddyb! This is a pretty big change though so I would like to bring it up at the next weekly meeting just to make sure everyone's comfortable with pulling the trigger.

[alexcrichton]

@bors: retry

[bors]

⌛ Testing commit 2d17a33 with merge 4a0ed0d...

[bors]

💔 Test failed - auto-linux-64-nopt-t

[alexcrichton]

@bors: retry

[bors]

⌛ Testing commit 2d17a33 with merge 7f674df...

[bors]

💔 Test failed - auto-linux-32-nopt-t

[alexcrichton]

@bors: retry

[bors]

⌛ Testing commit 2d17a33 with merge 1499596...

[bors]

💔 Test failed - auto-linux-64-x-android-t

[alexcrichton]

@bors: retry

[bors]

⌛ Testing commit 2d17a33 with merge 4b417b8...

[bors]

💔 Test failed - auto-linux-32-opt

[alexcrichton]

@bors: retry

[SimonSapin]

Does this fix #18810 ?