Add derive and doc comment capabilities to newtype_index macro

[Nashenas88]

This moves RustcDecodable and RustcEncodable out of the macro definition and into the macro uses. They were conflicting with CrateNum's impls of serialize::UseSpecializedEncodable and serialize::UseSpecializedDecodable, and now it's not :). CrateNum is now defined with the newtype_index macro. I also added support for doc comments on constant definitions and allowed a type to remove the pub specification on the tuple param (otherwise a LOT of code would refuse to compile for CrateNum). I was getting dozens of errors like this if CrateNum was defined as pub struct CrateNum(pub u32):

error[E0530]: match bindings cannot shadow tuple structs
   --> src/librustc/dep_graph/dep_node.rs:624:25
    |
63  | use hir::def_id::{CrateNum, DefId, DefIndex, CRATE_DEF_INDEX};
    |                   -------- a tuple struct `CrateNum` is imported here
...
624 |     [] MissingLangItems(CrateNum),
    |                         ^^^^^^^^ cannot be named the same as a tuple struct

I also cleaned up the formatting of the macro bodies as they were getting impossibly long. Should I go back and fix the matching rules to this style too?

I also want to see what the test results look like because CrateNum used to just derive Debug, but the newtype_index macro has a custom implementation. This might require further pushes.

Feel free to bikeshed on the macro language, I have no preference here.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[Nashenas88]

r? @nikomatsakis

[Nashenas88]

cc @spastorino

[kennytm]

Tidy errors.

[00:03:33] tidy error: /checkout/src/librustc/hir/def_id.rs:26: line longer than 100 chars
[00:03:33] tidy error: /checkout/src/librustc_data_structures/indexed_vec.rs:89: line longer than 100 chars
[00:03:33] tidy error: /checkout/src/librustc_data_structures/indexed_vec.rs:125: line longer than 100 chars
[00:03:33] tidy error: /checkout/src/librustc_data_structures/indexed_vec.rs:136: line longer than 100 chars
[00:03:33] tidy error: /checkout/src/librustc_data_structures/indexed_vec.rs:148: line longer than 100 chars
[00:03:33] tidy error: /checkout/src/librustc_data_structures/indexed_vec.rs:160: line longer than 100 chars
[00:03:33] tidy error: /checkout/src/librustc_data_structures/indexed_vec.rs:172: line longer than 100 chars
[00:03:33] tidy error: /checkout/src/librustc_data_structures/indexed_vec.rs:184: line longer than 100 chars

[spastorino]

@Nashenas88 thanks for working on this :).

Is there another way to get rid of the nopub thing?, not sure what's the issue we are having with that.

Another thing that I'm thinking about is if it's better to add the derive = something or just pass some kind of flag only in the case you want to define your encodings, something like do_not_derive_encodings.

[spastorino]

Another two things, we should make /~https://github.com/rust-lang/rust/blob/690ff045/src/librustc/dep_graph/graph.rs#L47-L66 and /~https://github.com/rust-lang/rust/blob/690ff045/src/librustc/hir/def_id.rs#L96-L125 also go away.
With DefIndex we have another issue related to implementing Debug. The implementation is different to what we are doing in the macro.
Maybe it's a good time to try to come up with something more general and good for all the cases. /cc @nikomatsakis

[nikomatsakis]

Regarding the debug business, seems like it'd be a good idea to have something more general. One simple thing might be allowing the impl to specify that it does not want a debug impl at all. Something like DEBUG_NAME=custom, and then you can just write your own impl Debug.

In #45538, I changed from DEBUG_NAME to DEBUG_FORMAT, but that was a very simple tweak that lets you write DEBUG_FORMAT="foo[{}]", and the {} gets substituted with the index. Not especially general.

[nikomatsakis]

I agree with @spastorino that it would be nice if derive[RustcEncodable, RustcDecodable] were the default. I suppose types could do derive[] to disable it, right?

[Nashenas88]

@nikomatsakis I'm wondering if I can somehow track the derive list so that derive[Debug] implies DEBUG_FORMAT=custom (Though reading that seems a little backwards). Also, only RustcEncodable and RustcDecodable should be removable, the rest should be left as is, right? What are your thoughts on the nopub? I couldn't think of a simpler way to implement that. Should I try to make everything compile when CrateNum's u32 is pub?

[Nashenas88]

Also, failing tests will be fixed when Debug trait is addressed.

[nikomatsakis]

@Nashenas88

I'm wondering if I can somehow track the derive list so that derive[Debug] implies DEBUG_FORMAT=custom

As you said, that seems a bit surprising to me.

Also, only RustcEncodable and RustcDecodable should be removable, the rest should be left as is, right?

Hmm, probably. I can't think when you want to do otherwise. Maybe in that case it's easier to just have a flag to disable encodable? e.g., ENCODABLE = custom or something.

Should I try to make everything compile when CrateNum's u32 is pub?

Regarding nopub, are you saying you get compilation errors if the field is public?

[Nashenas88]

Regarding nopub, are you saying you get compilation errors if the field is public?

Yes, the error is in the original comment I created the PR with, link. I was seeing that error 20 or so times.

I like ENCODABLE = custom. I'll go with that.

Regarding Debug I'll try to allow both derive[Debug] and DEBUG_FORMAT = custom which will both disable the existing Debug impl, but each has an additional piece of code it will inject, deriving the trait or just leave the impl blank for the user, respectively.

[Nashenas88]

@nikomatsakis I tried matching what you wrote in #45538 re DEBUG_FORMAT. This way the merge conflict can simply be resolved with a take theirs/ours. I also looked into the pub compilation failure. It's a weird one. This code will compile fine:

mod X {
    pub struct CrateNum(u32);
    impl CrateNum {
        pub fn new(val: u32) -> Self {
            CrateNum(val)
        }
    }
}

use X::CrateNum;

pub enum Enum {
    CrateNumVal(CrateNum)
}

fn main() {
    let val = Enum::CrateNumVal(CrateNum::new(4));
    match val {
        Enum::CrateNumVal(CrateNum) => {
            println!("hello world");
        }
    }
}

but this will not:

mod X {
    pub struct CrateNum(pub u32);
    impl CrateNum {
        pub fn new(val: u32) -> Self {
            CrateNum(val)
        }
    }
}

use X::CrateNum;

pub enum Enum {
    CrateNumVal(CrateNum)
}

fn main() {
    let val = Enum::CrateNumVal(CrateNum::new(4));
    match val {
        Enum::CrateNumVal(CrateNum) => {
            println!("hello world");
        }
    }
}

error[E0530]: match bindings cannot shadow tuple structs
  --> src/main.rs:20:27
   |
11 | use X::CrateNum;
   |     ----------- a tuple struct `CrateNum` is imported here
...
20 |         Enum::CrateNumVal(CrateNum) => {
   |                           ^^^^^^^^ cannot be named the same as a tuple struct

The behavior is the same when it's outside the module, regardless of visibility:

pub struct CrateNum(u32);
impl CrateNum {
    pub fn new(val: u32) -> Self {
        CrateNum(val)
    }
}

pub enum Enum {
    CrateNumVal(CrateNum)
}

fn main() {
    let val = Enum::CrateNumVal(CrateNum::new(4));
    match val {
        Enum::CrateNumVal(CrateNum) => {
            println!("hello world");
        }
    }
}

The version with the mod is more or less what's occurring in the define_dep_nodes macro in src/librustc/dep_graph/dep_node.rs. The pub specifier being added to CrateNum causes the code to fail to compile the generated match.

[nikomatsakis]

@Nashenas88 that is indeed odd. I am familiar with the rules giving rise to that error, but I think it's sort of a bug in the existing (dep-node) macro that we are relying on them. We could work around it by changing the various usages of - [] foo(CrateNum) to be [] foo { crate_num: CrateNum }, but that's kind of annoying.

The situation is that in general we resolve a naming ambiguity in binding pattern (i.e., a pattern with a plain identifier like x) by checking whether a tuple struct constructor x is in scope or not -- if so, then the pattern is considered to match the tuple struct, otherwise it's defining a new variable. However, tuple struct constructors with private arguments are not visible outside their module. So when the field is private, we don't have a conflict with a pattern like Foo(CrateNum).

This whole situation with the privacy of the inner field is kind of annoying. @spastorino I'm trying to remember why we made it pub in the first place... something to do with constants?

[spastorino]

@nikomatsakis we made it public because there were some match clauses if I recall correctly doing stuff like Struct(x) => ... or something like that

[bors]

☔ The latest upstream changes (presumably #45538) made this pull request unmergeable. Please resolve the merge conflicts.

[Nashenas88]

@nikomatsakis @spastorino I changed it so it's not pub by default. Only FirstStatementIndex needed a pub index, so that's the only one that has a pub marker at the top of its config section. I used pub idx so it's clear only the index is made pub. Let me know if you prefer something else, but I think I've addressed all issues at this point. The macro definition is now at a slim 283 lines long 😄 😅

[nikomatsakis]

private by default FTW =)

[nikomatsakis]

@bors r+

[bors]

📌 Commit 97692af has been approved by nikomatsakis

[bors]

⌛ Testing commit 97692af with merge a6885cb...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing a6885cb to master...