Stream: rustdoc

Topic: More rdj duplication


view this post on Zulip Nixon Enraght-Moony (Mar 31 2021 at 20:22):

So I went over an ran rustdoc-json on all the crates you can run in rustdoc (link) and found 2 new issues

Would love input on either

view this post on Zulip Joshua Nelson (Mar 31 2021 at 20:26):

the other I have no idea whats happing, and am extremely scared by as it makes literaly no sence to me how changing unrelated code can remove it

I bet this has to do with the generic parameters somehow

view this post on Zulip Joshua Nelson (Mar 31 2021 at 20:26):

I want to simplify it a bit before looking at it too much

view this post on Zulip Joshua Nelson (Mar 31 2021 at 20:36):

@Nixon Enraght-Moony how did you get that pretty diff output?

view this post on Zulip Joshua Nelson (Mar 31 2021 at 20:39):

@Nixon Enraght-Moony I think it's because the trait declaration and the function in the implementation have the same name. This crashes:

pub trait Get {}

pub trait Load {
    fn load() {}
}

impl<P: Get> Load for P {
    fn load() {}
}

pub struct Wrapper<T>(T);
impl Get for Wrapper<usize> {}

but this does not:

pub trait Get {}

pub trait Load {
    fn load() {}
}

impl<P: Get> Load for P {}

pub struct Wrapper<T>(T);
impl Get for Wrapper<usize> {}

view this post on Zulip Joshua Nelson (Mar 31 2021 at 20:41):

not sure why P: Get is important, but if I remove it and do impl Load for Wrapper directly the crash goes away

view this post on Zulip Joshua Nelson (Mar 31 2021 at 20:42):

ooh and one has output: None while the other has output: Some(Tuple([])) - I bet one is going through HIR and the other is going through metadata, that could explain why the generic parameter matters

view this post on Zulip Nixon Enraght-Moony (Mar 31 2021 at 20:43):

Joshua Nelson said:

Nixon Enraght-Moony how did you get that pretty diff output?

https://docs.rs/pretty_assertions

view this post on Zulip Joshua Nelson (Mar 31 2021 at 20:43):

Joshua Nelson said:

not sure why P: Get is important, but if I remove it and do impl Load for Wrapper directly the crash goes away

yeah it doesn't need the P: Get bound, just for P to be generic

view this post on Zulip Joshua Nelson (Mar 31 2021 at 20:44):

mcve:

pub trait Load {
    fn load() {}
}

impl<P> Load for P {
    fn load() {}
}

pub struct Wrapper {}

view this post on Zulip Joshua Nelson (Mar 31 2021 at 20:46):

left a comment on the issue (nothing that's not here)

view this post on Zulip Joshua Nelson (Mar 31 2021 at 20:50):

It's unlear to me what the correct representation should be, as currently for items pub used from a non pub mod, we add the item to the mod it's being imported to, with the name it's imported under.

json ids https://github.com/rust-lang/rust/issues/80664#issuecomment-753681341

view this post on Zulip Nixon Enraght-Moony (Mar 31 2021 at 20:55):

What does new ids do for this. Are you proposing that we just put it in the index twice, with a diffent name (and new json-id), but the same everthing else? Because I think that we want to somewhow indicate that these are the same underlying type.

view this post on Zulip Joshua Nelson (Mar 31 2021 at 20:58):

Nixon Enraght-Moony said:

What does new ids do for this. Are you proposing that we just put it in the index twice, with a diffent name (and new json-id), but the same everthing else?

Yes, that's what I mean. I don't see another way to keep the info about both re-exports.

Because I think that we want to somewhow indicate that these are the same underlying type.

We could add a new original_id field maybe?

view this post on Zulip Joshua Nelson (Mar 31 2021 at 20:59):

alternatively we could remove re-exports from the index altogether and add a new field that's only re-exports and is keyed by (module, name) instead of id

view this post on Zulip Nixon Enraght-Moony (Mar 31 2021 at 21:23):

Yeah, I'm not sure which of these things is better. The both seem quite bad, but I'm not sure good answers exits. I guess my primary consern is that we force consumers to acknoledge that items can have many names. (I thing this is right, as otherwize we'll get tooling that works 99% of the time, but falls down when someone does something weird.)

view this post on Zulip Joshua Nelson (Mar 31 2021 at 21:26):

I guess my primary consern is that we force consumers to acknoledge that items can have many names. (I thing this is right, as otherwize we'll get tooling that works 99% of the time, but falls down when someone does something weird.)

Yeah, I don't see a way around this. The only alternative is to omit the info altogether and only document the first re-export, which seems at least as bad.

view this post on Zulip Joshua Nelson (Mar 31 2021 at 21:26):

cc @HeroicKatora btw

view this post on Zulip Nixon Enraght-Moony (Mar 31 2021 at 21:27):

Doing all this has realy made me appreciate that maybe go was right to have such a simple module system

view this post on Zulip Joshua Nelson (Mar 31 2021 at 21:27):

how does go's module system work?

view this post on Zulip Joshua Nelson (Mar 31 2021 at 21:27):

this doesn't seem like a super complicated part to me - it's just allowing renaming

view this post on Zulip Nixon Enraght-Moony (Mar 31 2021 at 21:29):

Joshua Nelson said:

how does go's module system work?

No reexports, no renaming, everything in a folder is the same namespace (as opposed to file in rust). No need for mod statements to discover files. No inline modules

view this post on Zulip Nixon Enraght-Moony (Mar 31 2021 at 21:30):

Joshua Nelson said:

this doesn't seem like a super complicated part to me - it's just allowing renaming

But the fact that you can have an item with no "canonical" name (esp if both reeexports rename) is super trippy IMO

view this post on Zulip Joshua Nelson (Mar 31 2021 at 21:36):

oh well good point

view this post on Zulip Joshua Nelson (Mar 31 2021 at 21:36):

yeah I've been meaning to make an RFC for doc(canonical)

view this post on Zulip Joshua Nelson (Mar 31 2021 at 21:36):

https://github.com/rust-lang/rfcs/issues/3011

view this post on Zulip CraftSpider (Apr 01 2021 at 01:00):

The first one definitely looks like custom IDs are needed, or something similar. Personally I lean towards re-exports pointing towards their originator somehow, rather than making them their own field. Though that's more because it's less work and feels more 'consistent' with how most all items are just items currently.

view this post on Zulip CraftSpider (Apr 01 2021 at 01:00):

The second one, oh geez I'll take a look into how it handles the generics

view this post on Zulip Joshua Nelson (Apr 01 2021 at 01:00):

CraftSpider said:

The second one, oh geez I'll take a look into how it handles the generics

I expect this to be a bug in rustdoc itself FWIW, not in the JSON backend

view this post on Zulip Joshua Nelson (Apr 01 2021 at 01:00):

happy to help if you get stuck

view this post on Zulip CraftSpider (Apr 01 2021 at 01:03):

Yeah, my suspicion was that I'd start looking at clean to see how rustdoc is getting the information. I'll ask if I run into issues


Last updated: Oct 21 2021 at 20:47 UTC