Stream: t-compiler/help

Topic: weird issues with finding trait implementations


Joshua Nelson (Jul 18 2020 at 18:52, on Zulip):

Summarizing the major points from https://discordapp.com/channels/442252698964721669/459149231702278154/734115963082899517:

Any ideas?

Joshua Nelson (Jul 18 2020 at 18:52, on Zulip):

for context, we're trying to fix a bug where String::from doesn't resolve in intra-doc links

Joshua Nelson (Jul 18 2020 at 18:54, on Zulip):

oh hold on I think I need to call trait_id_of_impl to print the name

Joshua Nelson (Jul 18 2020 at 20:47, on Zulip):

I figured this out, see vaguely https://discordapp.com/channels/442252698964721669/459149231702278154/734122117095882783

Joshua Nelson (Jul 18 2020 at 22:00, on Zulip):

ok it turns out the issue is that all_impls takes a _trait_, not a _type_

Joshua Nelson (Jul 18 2020 at 22:00, on Zulip):

is there a cheap way to get all traits that a _type_ implements?

Joshua Nelson (Jul 18 2020 at 22:01, on Zulip):

the best I've found is going through every trait from every crate one at a time which doesn't seem right

eddyb (Jul 20 2020 at 02:35, on Zulip):

@Joshua Nelson in case you're still wondering: it is right. although I already linked this on Discord: https://github.com/rust-lang/rust/blob/d7f94516345a36ddfcd68cbdf1df835d356795c3/src/librustc_trait_selection/traits/error_reporting/mod.rs#L1395-L1414

Joshua Nelson (Jul 20 2020 at 02:35, on Zulip):

I was actually just starting to work on this again haha

Joshua Nelson (Jul 20 2020 at 02:36, on Zulip):

this seems kind of slow?

eddyb (Jul 20 2020 at 02:36, on Zulip):

the situation with inherent impls is the wrong one, actually. when you say "type" you presumably mean a struct/enum/union, right?

Joshua Nelson (Jul 20 2020 at 02:36, on Zulip):

right

Joshua Nelson (Jul 20 2020 at 02:36, on Zulip):

(I think)

Joshua Nelson (Jul 20 2020 at 02:36, on Zulip):

anything that Type::item is valid for

eddyb (Jul 20 2020 at 02:36, on Zulip):

that's not what "implements" a trait, and so asking for impls "of it" is not something fundamental(ly meaningful)

Joshua Nelson (Jul 20 2020 at 02:37, on Zulip):

wait, what? don't types implement traits?

eddyb (Jul 20 2020 at 02:37, on Zulip):

semantic types implement traits, and your path example is misleading because it desugars to <Type<..>>::item

Joshua Nelson (Jul 20 2020 at 02:37, on Zulip):

What's a semantic type?

eddyb (Jul 20 2020 at 02:38, on Zulip):

typesystem types. where e.g. type aliases don't matter. ty::Ty

eddyb (Jul 20 2020 at 02:38, on Zulip):

the only thing that's sort of pre-typesystem about (trait) impls is the trait. that's why you can find impls by trait

Joshua Nelson (Jul 20 2020 at 02:39, on Zulip):

ok, so if I run tcx.type_of(def_id), can I get the traits that semantic type implements?

eddyb (Jul 20 2020 at 02:40, on Zulip):

not without going through all the traits in the world, taking every single one of their impls, and comparing them against that type

Joshua Nelson (Jul 20 2020 at 02:40, on Zulip):

well, time to make the CPU cache very unhappy

eddyb (Jul 20 2020 at 02:40, on Zulip):

there's not really any other way the trait system can work, because the type can any type

eddyb (Jul 20 2020 at 02:41, on Zulip):

inherent impls we've managed to keep lying for because we restrict what they can go on

eddyb (Jul 20 2020 at 02:41, on Zulip):

also ugh I feel like I'm explaining this very poorly

Joshua Nelson (Jul 20 2020 at 02:41, on Zulip):

I think I understand what you're saying

eddyb (Jul 20 2020 at 02:41, on Zulip):

if all you could write is impl Trait for AStructOrEnumOrUnion then yes there would make sense to be a mapping

Joshua Nelson (Jul 20 2020 at 02:41, on Zulip):

I'm just not understanding the design decisions that make adding this hard

eddyb (Jul 20 2020 at 02:42, on Zulip):

but you can implement a trait for (&i32, Vec<Foo>)

eddyb (Jul 20 2020 at 02:42, on Zulip):

and that doesn't have a DefId, it can only be represented by aTy

Joshua Nelson (Jul 20 2020 at 02:42, on Zulip):

hmm

eddyb (Jul 20 2020 at 02:42, on Zulip):

and when generics are involved, you end up with patterns

eddyb (Jul 20 2020 at 02:43, on Zulip):

not in the sense of Pat syntax in Rust, but more abstractly

Joshua Nelson (Jul 20 2020 at 02:43, on Zulip):

I think I see, the query I'm imagining that goes from types to traits only works if you have a DefId

Joshua Nelson (Jul 20 2020 at 02:43, on Zulip):

this is a shame because rustdoc will never have to deal with (&i32, Vec<Foo>) :/

Joshua Nelson (Jul 20 2020 at 02:43, on Zulip):

at least not for intra links

eddyb (Jul 20 2020 at 02:44, on Zulip):

anyway, the for_each_relevant_impl query does a compromise: for each trait we cache the list of impls partitioned by a "simplified type"

eddyb (Jul 20 2020 at 02:45, on Zulip):

(and if one is impossible to derive from the semantic type, it's treated as "blanket". so every time you look for a specific struct's impl, you'll get all the e.g. impl<T> From<Foo> for T impls)

eddyb (Jul 20 2020 at 02:45, on Zulip):

(because there's no way to tell you don't want those impls)

eddyb (Jul 20 2020 at 02:46, on Zulip):

@Joshua Nelson okay waking up enough to formulate the other constraints: the crate graph can form "diamonds"

eddyb (Jul 20 2020 at 02:46, on Zulip):
    a
  /   \
 b     c
  \   /
    d
eddyb (Jul 20 2020 at 02:46, on Zulip):

this sort of shape of dependencies

Joshua Nelson (Jul 20 2020 at 02:46, on Zulip):

sure

eddyb (Jul 20 2020 at 02:47, on Zulip):

so you can't do arbitrary indexing even with the "simplified types" trick

eddyb (Jul 20 2020 at 02:48, on Zulip):

without putting a lot of work on every crate to combine together all the traits' impls into one big indexed map

eddyb (Jul 20 2020 at 02:48, on Zulip):

@Joshua Nelson furthermore, this is never needed (outside of diagnostics), as you either have to name the trait or have it in scope, in order to use it, normally

Joshua Nelson (Jul 20 2020 at 02:48, on Zulip):

because a downstream crate can implement a local trait for a foreign type, right

Joshua Nelson (Jul 20 2020 at 02:48, on Zulip):

ok it's starting to make sense why this can't be done cheaply

eddyb (Jul 20 2020 at 02:49, on Zulip):

ideally we'd have a very fancy pattern-indexed thing. and maybe Chalk can do some of that (not sure), but it would likely still be partitioned by traits just because of how much less effort that makes things

eddyb (Jul 20 2020 at 02:49, on Zulip):

sorry for starting at the (arguably) wrong tail-end of this

Joshua Nelson (Jul 20 2020 at 02:49, on Zulip):

you're good, I was doing some code munging in the meantime

Joshua Nelson (Jul 20 2020 at 02:51, on Zulip):

adding back inherent impls since apparently it was very incorrect to remove those lookups lol

eddyb (Jul 20 2020 at 02:51, on Zulip):

@Joshua Nelson oh btw it's not an ambiguity to have both inherent and trait available. the inherent will always take precedence

eddyb (Jul 20 2020 at 02:51, on Zulip):

(and you specify the trait one by Trait::method or <T as Trait>::method etc.)

Joshua Nelson (Jul 20 2020 at 02:52, on Zulip):

the issue is that rustdoc doesn't currently support <T as Trait>

eddyb (Jul 20 2020 at 02:52, on Zulip):

so you probably want to use that same logic

Joshua Nelson (Jul 20 2020 at 02:52, on Zulip):

so even though it's not ambiguous in the language it's ambiguous in the context of intra-doc links

eddyb (Jul 20 2020 at 02:52, on Zulip):

well, no, the trait one should just not be reachable :P

Joshua Nelson (Jul 20 2020 at 02:52, on Zulip):

well yeah but that's not ideal either lol

eddyb (Jul 20 2020 at 02:52, on Zulip):

unless you have examples where this is a problem?

Joshua Nelson (Jul 20 2020 at 02:53, on Zulip):

it just seems unfortunate to have an item that's unreachable

Joshua Nelson (Jul 20 2020 at 03:02, on Zulip):

@eddyb so I see two ways this is currently done: in rustc_trait_selection::error_reporting it calls all_traits, then for_each_relevant_impl on each trait. But in rustdoc::collect_trait_impls it calls all_trait_implementations instead: https://github.com/rust-lang/rust/blob/master/src/librustdoc/passes/collect_trait_impls.rs#L30. Which should I use?

Joshua Nelson (Jul 20 2020 at 03:03, on Zulip):

I'll go with for_each_relevant_impl since you said it was indexed by the type

eddyb (Jul 20 2020 at 03:04, on Zulip):

@Joshua Nelson all_trait_implementations doesn't give you any indexing so yeah

Joshua Nelson (Jul 20 2020 at 03:05, on Zulip):

actually I can filter this even more by skipping traits that don't have this associated item

Joshua Nelson (Jul 20 2020 at 03:06, on Zulip):

so then I don't need to look at each impl unless it's relevant

eddyb (Jul 20 2020 at 03:06, on Zulip):

I kinda doubt all_trait_implementations is that useful now that I think about it hmm, since it shouldn't be slower to iterate the traits (since the total work is still linear in the number of total impls)

eddyb (Jul 20 2020 at 03:06, on Zulip):

@Joshua Nelson ah yeah! I remember suggesting that on Discord :P

eddyb (Jul 20 2020 at 03:07, on Zulip):

the error reporting code filters by confusable trait path but in your case, the associated item is relevant

Joshua Nelson (Jul 20 2020 at 03:07, on Zulip):

great minds think alike :P

Joshua Nelson (Jul 20 2020 at 03:07, on Zulip):

I'd use associated_items() for that?

eddyb (Jul 20 2020 at 03:08, on Zulip):

I think so? make sure it has an API to query by name

eddyb (Jul 20 2020 at 03:08, on Zulip):

so that you don't have to iterate all the associated items before getting to look at their names

Joshua Nelson (Jul 20 2020 at 03:08, on Zulip):

yup: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/struct.AssociatedItems.html#method.filter_by_name

Joshua Nelson (Jul 20 2020 at 03:09, on Zulip):

uh oh

Multiple items may have the same name if they are in different Namespaces. For example, an associated type can have the same name as a method. Use one of the find_by_name_and_* methods below if you know which item you are looking for.

eddyb (Jul 20 2020 at 03:10, on Zulip):

you should use .filter_by_name_unhygienic(name).next().is_some()

Joshua Nelson (Jul 20 2020 at 03:10, on Zulip):

actually I already have a namespace argument so this works out

eddyb (Jul 20 2020 at 03:10, on Zulip):

oh you do? hmm

Joshua Nelson (Jul 20 2020 at 03:10, on Zulip):

@eddyb I need the AssocKind of the item, not just that it exists

eddyb (Jul 20 2020 at 03:10, on Zulip):

I still think you need to use the the unhygienic variant

eddyb (Jul 20 2020 at 03:10, on Zulip):

but you could try to pass the DefId of the item being documented as parent_def_id if you can?

Joshua Nelson (Jul 20 2020 at 03:11, on Zulip):

what's the difference between hygenic and unhygenic here?

eddyb (Jul 20 2020 at 03:11, on Zulip):

macro hygiene

eddyb (Jul 20 2020 at 03:11, on Zulip):

specifically for the case here of associated items, macros 2.0 (macro foo {...} or macro foo(...) {...}), not macro_rules!

Joshua Nelson (Jul 20 2020 at 03:12, on Zulip):

shouldn't parent_def_id be that of the trait?

eddyb (Jul 20 2020 at 03:12, on Zulip):

I doubt it's the parent of the associated item, since the associated item knows that itself

eddyb (Jul 20 2020 at 03:12, on Zulip):

ugh I might be wrong

Joshua Nelson (Jul 20 2020 at 03:13, on Zulip):

the docs for this are non-existent lol

Joshua Nelson (Jul 20 2020 at 03:13, on Zulip):

you have to guess from the types

eddyb (Jul 20 2020 at 03:13, on Zulip):

yeah calls pass the trait DefId. I don't understand why though

eddyb (Jul 20 2020 at 03:15, on Zulip):

like AssociatedItems could just contain that DefId if looking it up seems too expensive

eddyb (Jul 20 2020 at 03:15, on Zulip):

@Joshua Nelson lol they're not more efficient https://github.com/rust-lang/rust/blob/master/src/librustc_middle/ty/mod.rs#L277-L316

eddyb (Jul 20 2020 at 03:16, on Zulip):

so you should be fine with doing this unhygienically and filtering by namespace or w/e

Joshua Nelson (Jul 20 2020 at 03:16, on Zulip):

:face_palm:

Joshua Nelson (Jul 20 2020 at 03:16, on Zulip):

it looks like find_by_name_and_namespace is what I want

eddyb (Jul 20 2020 at 03:17, on Zulip):

oh that returns a single one. okay fine :P

eddyb (Jul 20 2020 at 03:18, on Zulip):

this is what typeck does:

tcx
    .associated_items(def_id)
    .find_by_name_and_namespace(tcx, item_name, ns, def_id)
    .copied()
eddyb (Jul 20 2020 at 03:18, on Zulip):

as usual, grepping is far more useful than docs :P

eddyb (Jul 20 2020 at 03:19, on Zulip):

oh but you're filtering traits. so it's not like you need the ty::AssocItem

Joshua Nelson (Jul 20 2020 at 03:19, on Zulip):

how do I get info about the impl once I have it?

Joshua Nelson (Jul 20 2020 at 03:19, on Zulip):

I want to know what type it's for

eddyb (Jul 20 2020 at 03:19, on Zulip):

once you have it from for_each_relevant_impl?

eddyb (Jul 20 2020 at 03:20, on Zulip):

impl_trait_ref ideally but impl_self_ty also works IIRC

Joshua Nelson (Jul 20 2020 at 03:21, on Zulip):

I don't see an impl_self_ty

eddyb (Jul 20 2020 at 03:22, on Zulip):

oh right it wasn't made its own query lol

eddyb (Jul 20 2020 at 03:22, on Zulip):

@Joshua Nelson just use impl_trait_ref then

eddyb (Jul 20 2020 at 03:22, on Zulip):

it's less confusing

Joshua Nelson (Jul 20 2020 at 03:22, on Zulip):

wait, why does that return an Option??

eddyb (Jul 20 2020 at 03:22, on Zulip):

None for inherent impls

eddyb (Jul 20 2020 at 03:22, on Zulip):

(you can unwrap it if you found it from a trait)

Joshua Nelson (Jul 20 2020 at 03:28, on Zulip):

well it compiles at least

Joshua Nelson (Jul 20 2020 at 03:28, on Zulip):

let me see if it actually works

Joshua Nelson (Jul 20 2020 at 03:29, on Zulip):

it's just as complicated as I thought btw: https://hastebin.com/zeviyuxuco.js

eddyb (Jul 20 2020 at 03:30, on Zulip):

if trait_ref.self_ty() == ty { will not work heh

Joshua Nelson (Jul 20 2020 at 03:30, on Zulip):

oh no :(

eddyb (Jul 20 2020 at 03:30, on Zulip):

in general, don't == on Ty unless it's a fast-path

Joshua Nelson (Jul 20 2020 at 03:31, on Zulip):

is there a deep_equals() or something?

eddyb (Jul 20 2020 at 03:31, on Zulip):

think what happens if generics are involved and the trait impl used different names for the parameters than the definition

eddyb (Jul 20 2020 at 03:31, on Zulip):

@Joshua Nelson relate but doubtful you need it. where'd you get the ty

Joshua Nelson (Jul 20 2020 at 03:31, on Zulip):

tcx.type_of(def_id)

eddyb (Jul 20 2020 at 03:31, on Zulip):

(or worse than parameter names: the impl is for specific types replacing some of the parameters)

eddyb (Jul 20 2020 at 03:32, on Zulip):

okay so it's not an arbitrary type so you wouldn't need to do anything deep

eddyb (Jul 20 2020 at 03:32, on Zulip):

just check for ty::Adt(def, _) and compare def.did

eddyb (Jul 20 2020 at 03:32, on Zulip):

this will effectively ignore blanket impls (which idk if you can do more efficiently otherwise)

Joshua Nelson (Jul 20 2020 at 03:32, on Zulip):

lol it failed even earlier

$ rustdoc +stage1 str_from.rs
thread 'rustc' panicked at 'assertion failed: `(left == right)`
  left: `crate1`,
 right: `crate0`', src/librustc_typeck/check/method/suggest.rs:1305:9
eddyb (Jul 20 2020 at 03:33, on Zulip):

looks like non_blanket_impls is not exposed so there's not much you can do

Joshua Nelson (Jul 20 2020 at 03:33, on Zulip):

eddyb said:

just check for ty::Adt(def, _) and compare def.did

should i also check for Foreign(def)?

eddyb (Jul 20 2020 at 03:34, on Zulip):

oh maybe. I keep forgetting

eddyb (Jul 20 2020 at 03:34, on Zulip):

also do you want to support e.g. i32::method?

Joshua Nelson (Jul 20 2020 at 03:34, on Zulip):

oh ugh I probably should

Joshua Nelson (Jul 20 2020 at 03:34, on Zulip):

even though primitives are all sorts of broken right now

eddyb (Jul 20 2020 at 03:34, on Zulip):

also you should ban this feature on type aliases

eddyb (Jul 20 2020 at 03:35, on Zulip):

@Joshua Nelson at least for the named primitives == will do what you need (since they have no parameters)

eddyb (Jul 20 2020 at 03:36, on Zulip):

(the problem with type aliases is they can be arbitrarily complicated so checking if TypeAlias::method would resolve in typeck requires asking the trait system nicely, which is more effort)

Joshua Nelson (Jul 20 2020 at 03:36, on Zulip):

yeah I just want an MVP for now lol

Joshua Nelson (Jul 20 2020 at 03:36, on Zulip):

since before it was so broken it depended where in the crate you put the link lol

eddyb (Jul 20 2020 at 03:37, on Zulip):

so you can do == and fallback to comparing ty::Adts

Joshua Nelson (Jul 20 2020 at 03:40, on Zulip):

how's this?

                    let trait_ref = cx.tcx.impl_trait_ref(impl_).expect("this is not an inherent impl");
                    // Check if these are the same type.
                    let impl_type = trait_ref.self_ty();
                    // Fast path: if this is a primitive simple `==` will work
                    let same_type = (impl_type == ty) || match impl_type.kind {
                        // Check if these are the same def_id
                        TyKind::Adt(def, _) => def.did == did,
                        TyKind::Foreign(def_id) => def_id == did,
                        _ => false,
                    };
eddyb (Jul 20 2020 at 03:40, on Zulip):

does that not warn about unnecessary parens? :P

Joshua Nelson (Jul 20 2020 at 03:41, on Zulip):

I don't want to remember whether == or || has higher precedence lol

eddyb (Jul 20 2020 at 03:41, on Zulip):

|| and && can't mess with comparisons

Joshua Nelson (Jul 20 2020 at 03:41, on Zulip):

ok same panic as before

Joshua Nelson (Jul 20 2020 at 03:41, on Zulip):

(unsurprisingly)

eddyb (Jul 20 2020 at 03:41, on Zulip):

since if conditions are || of && of comparisons, without parens, lol

eddyb (Jul 20 2020 at 03:41, on Zulip):

like that's what they're "optimized" for :P

Joshua Nelson (Jul 20 2020 at 03:42, on Zulip):
thread 'rustc' panicked at 'assertion failed: `(left == right)`
  left: `crate1`,
 right: `crate0`', src/librustc_typeck/check/method/suggest.rs:1305:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

query stack during panic:
#0 [all_traits] fetching all foreign and local traits
end of query stack
eddyb (Jul 20 2020 at 03:42, on Zulip):

@Joshua Nelson wait how are you calling that? the suggestions code uses all_traits(LOCAL_CRATE)

Joshua Nelson (Jul 20 2020 at 03:43, on Zulip):

... please don't tell me this only works for the local crate

eddyb (Jul 20 2020 at 03:43, on Zulip):

it's a nulary query

Joshua Nelson (Jul 20 2020 at 03:43, on Zulip):

oh wait we only care about traits available for the current crate

eddyb (Jul 20 2020 at 03:43, on Zulip):

we should just make it tcx.all_traits() but nobody has done it yet

Joshua Nelson (Jul 20 2020 at 03:43, on Zulip):

... why would it take a parameter if it's just going to ignore it lol

Joshua Nelson (Jul 20 2020 at 03:43, on Zulip):

ok yeah

eddyb (Jul 20 2020 at 03:43, on Zulip):

because queries started out as a macro making memoization cleaner :P

eddyb (Jul 20 2020 at 03:44, on Zulip):

and it has to figure out which crate to compute the query "relative to"

Joshua Nelson (Jul 20 2020 at 03:47, on Zulip):

warning: [std::collections::BTreeMap::into_iter] cannot be resolved, ignoring it.

great

Joshua Nelson (Jul 20 2020 at 03:50, on Zulip):

eddyb said:

Joshua Nelson at least for the named primitives == will do what you need (since they have no parameters)

hold on, primitives don't have a def_id

Joshua Nelson (Jul 20 2020 at 03:50, on Zulip):

so since I got the type from a def id it will never be a primitive

eddyb (Jul 20 2020 at 03:51, on Zulip):

so you're just not supporting them, right :P

Joshua Nelson (Jul 20 2020 at 03:51, on Zulip):

works for me

eddyb (Jul 20 2020 at 03:51, on Zulip):

if you start with a Res, you should be able to support them

eddyb (Jul 20 2020 at 03:51, on Zulip):

(by handling cases other than Res::Def)

Joshua Nelson (Jul 20 2020 at 03:51, on Zulip):

ah yup you're right

Joshua Nelson (Jul 20 2020 at 03:52, on Zulip):

I'll fix it up later

Joshua Nelson (Jul 20 2020 at 03:53, on Zulip):

ooh I just realized, even though into_iter doesn't work the explicit impl did :) impl MyTrait for MyType

Joshua Nelson (Jul 20 2020 at 03:54, on Zulip):
pub trait T1 {
    fn method();
}

/// See [S::method]
pub struct S;

impl T1 for S {
    /// [S::method] on method
    fn method() {}
}
eddyb (Jul 20 2020 at 03:54, on Zulip):

add a type parameter, curious if that works

eddyb (Jul 20 2020 at 03:54, on Zulip):

and/or a trait from a different crate

Joshua Nelson (Jul 20 2020 at 03:54, on Zulip):

different crate definitely doesn't work

warning: [std::collections::BTreeMap::into_iter] cannot be resolved, ignoring it.

eddyb (Jul 20 2020 at 03:55, on Zulip):

not like that, in your test

eddyb (Jul 20 2020 at 03:55, on Zulip):

assume that the most general case is hopelessly broken in 3 different ways

eddyb (Jul 20 2020 at 03:55, on Zulip):

so test things one at a time

Joshua Nelson (Jul 20 2020 at 03:55, on Zulip):

I see

eddyb (Jul 20 2020 at 03:55, on Zulip):

like, modify one variable at a time. that BTreeMap example has alloc type, core trait, and the type is generic,

Joshua Nelson (Jul 20 2020 at 03:57, on Zulip):

this also fails:

/// Link to [S], [S::clone]
pub struct S;

impl Clone for S {
    fn clone(&self) -> Self {
        S
    }
}
eddyb (Jul 20 2020 at 03:57, on Zulip):

this is worrying

Joshua Nelson (Jul 20 2020 at 03:57, on Zulip):

warning: [S::clone] cannot be resolved, ignoring it.

Joshua Nelson (Jul 20 2020 at 03:58, on Zulip):

well it's looking at the items so it's probably a bug in my code

[DEBUG rustdoc::passes::collect_intra_doc_links] considering explicit impl for trait DefId(2:1636 ~ core[92f6]::clone[0]::Clone[0])
[DEBUG rustdoc::passes::collect_intra_doc_links] considering item (DefId(2:1637 ~ core[92f6]::clone[0]::Clone[0]::clone[0]), Fn)
[DEBUG rustdoc::passes::collect_intra_doc_links] considering type &T
[DEBUG rustdoc::passes::collect_intra_doc_links] considering type &mut T
[DEBUG rustdoc::passes::collect_intra_doc_links] considering type S
eddyb (Jul 20 2020 at 03:59, on Zulip):

ugh all_traits is implemented by diagnostic logic, this is worrying

Joshua Nelson (Jul 20 2020 at 03:59, on Zulip):

maybe I need to check more type kinds?

eddyb (Jul 20 2020 at 03:59, on Zulip):

but it does handle cross-crate

eddyb (Jul 20 2020 at 03:59, on Zulip):

struct is ty::Adt though

Joshua Nelson (Jul 20 2020 at 03:59, on Zulip):

let me use debug printing for the types

eddyb (Jul 20 2020 at 03:59, on Zulip):

that does nothing

eddyb (Jul 20 2020 at 04:00, on Zulip):

you can at most name your struct something that's not confusable. but assuming it's S it should work

eddyb (Jul 20 2020 at 04:01, on Zulip):

also, s/TyKind::/ty::, idk why you don't get an internal warning for that

Joshua Nelson (Jul 20 2020 at 04:01, on Zulip):

you are correct that debug printing does nothing

Joshua Nelson (Jul 20 2020 at 04:01, on Zulip):

I changed the name to MyStruct but basically the same as before

[DEBUG rustdoc::passes::collect_intra_doc_links] considering explicit impl for trait
DefId(2:1636 ~ core[92f6]::clone[0]::Clone[0])
[DEBUG rustdoc::passes::collect_intra_doc_links] considering item (DefId(2:1637 ~ cor
e[92f6]::clone[0]::Clone[0]::clone[0]), Fn)
[DEBUG rustdoc::passes::collect_intra_doc_links] considering type &T
[DEBUG rustdoc::passes::collect_intra_doc_links] considering type &mut T
[DEBUG rustdoc::passes::collect_intra_doc_links] considering type MyStruct
eddyb (Jul 20 2020 at 04:01, on Zulip):

okay I think I know what the problem is

eddyb (Jul 20 2020 at 04:02, on Zulip):

make it struct MyStruct { foo: () }

eddyb (Jul 20 2020 at 04:02, on Zulip):

that might just work

Joshua Nelson (Jul 20 2020 at 04:03, on Zulip):

still breaks :(

eddyb (Jul 20 2020 at 04:03, on Zulip):

although if it wasn't the right DefId- oh right, type_of(ctor_def_id) for an unit constructor still gives you the original type

Joshua Nelson (Jul 20 2020 at 04:03, on Zulip):
[DEBUG rustdoc::passes::collect_intra_doc_links] considering explicit impl for trait
DefId(2:1636 ~ core[92f6]::clone[0]::Clone[0])
[DEBUG rustdoc::passes::collect_intra_doc_links] considering item (DefId(2:1637 ~ cor
e[92f6]::clone[0]::Clone[0]::clone[0]), Fn)
[DEBUG rustdoc::passes::collect_intra_doc_links] considering type &T with kind Ref(ReEarlyBound(0, '_), T, Not)
[DEBUG rustdoc::passes::collect_intra_doc_links] considering type &mut T with kind Ref(ReEarlyBound(0, '_), T, Mut)
[DEBUG rustdoc::passes::collect_intra_doc_links] considering type MyStruct with kind Adt(MyStruct, [])
/// Link to [MyStruct], [MyStruct::clone]
pub struct MyStruct { foo: () }

impl Clone for MyStruct {
    fn clone(&self) -> Self {
        MyStruct
    }
}
eddyb (Jul 20 2020 at 04:03, on Zulip):

okay print the Res / DefId you're starting from

eddyb (Jul 20 2020 at 04:04, on Zulip):

@Joshua Nelson there really isn't anything else the type can be. the DefId you have however might be not the struct one

eddyb (Jul 20 2020 at 04:04, on Zulip):

but rather S's unit constructor

eddyb (Jul 20 2020 at 04:05, on Zulip):

however, why would local trait work

Joshua Nelson (Jul 20 2020 at 04:05, on Zulip):

ohh

Joshua Nelson (Jul 20 2020 at 04:05, on Zulip):

wait yeah that's confusing

eddyb (Jul 20 2020 at 04:05, on Zulip):

just print it, we'll know when we see it

eddyb (Jul 20 2020 at 04:06, on Zulip):

(it might only affect the struct MyStruct; case)

Joshua Nelson (Jul 20 2020 at 04:06, on Zulip):
[DEBUG rustdoc::passes::collect_intra_doc_links] comparing type MyStruct with kind Adt(MyStruct, []) against def_id DefId(0:3 ~ str_from[8787]::MyStruct[0])
Joshua Nelson (Jul 20 2020 at 04:06, on Zulip):

looks right to me?

eddyb (Jul 20 2020 at 04:06, on Zulip):

okay just print the DefId in the ty::Adt, and also the comparison result

Joshua Nelson (Jul 20 2020 at 04:06, on Zulip):

this is for pub struct MyStruct { foo: () }

eddyb (Jul 20 2020 at 04:07, on Zulip):

how sure are you that the comparison doesn't succeed?

eddyb (Jul 20 2020 at 04:07, on Zulip):

heck it's not generic, the == will likely work :P

eddyb (Jul 20 2020 at 04:07, on Zulip):

yeah even if constructors were involved, impl_self_ty == ty would return true, sorry for the derail

eddyb (Jul 20 2020 at 04:08, on Zulip):

so it must be something else that's breaking it

Joshua Nelson (Jul 20 2020 at 04:08, on Zulip):

watch me accidentally forget to add it to the candidates or something lol

Joshua Nelson (Jul 20 2020 at 04:08, on Zulip):

here's the code so I have more eyes on it:

                let trait_ref = cx.tcx.impl_trait_ref(impl_).expect("this is not an inherent impl");
                // Check if these are the same type.
                let impl_type = trait_ref.self_ty();
                debug!("comparing type {} with kind {:?} against def_id {:?}", impl_type, impl_type.kind, did);
                // Fast path: if this is a primitive simple `==` will work
                let same_type = impl_type == ty || match impl_type.kind {
                    // Check if these are the same def_id
                    TyKind::Adt(def, _) => {
                        debug!("adt did: {:?}", def.did);
                        def.did == did
                    }
                    TyKind::Foreign(def_id) => def_id == did,
                    _ => false,
                };
                if same_type {
                    // We found it!
                    debug!("found a match!");
                    candidates.push(assoc_item);
                }
Joshua Nelson (Jul 20 2020 at 04:09, on Zulip):

and then at the end

    // Cleanup and go home
    Ok(candidates.pop().map(|(_, kind)| kind))
Joshua Nelson (Jul 20 2020 at 04:09, on Zulip):

oh god damn it

[DEBUG rustdoc::passes::collect_intra_doc_links] comparing type MyStruct with kind Ad
t(MyStruct, []) against def_id DefId(0:3 ~ str_from[8787]::MyStruct[0])
[DEBUG rustdoc::passes::collect_intra_doc_links] found a match!
Joshua Nelson (Jul 20 2020 at 04:09, on Zulip):

so it's a bug somewhere else lol

eddyb (Jul 20 2020 at 04:10, on Zulip):

there we go

Joshua Nelson (Jul 20 2020 at 04:12, on Zulip):

wait wtf

Joshua Nelson (Jul 20 2020 at 04:12, on Zulip):

my debugging isn't showing up outside this function

Joshua Nelson (Jul 20 2020 at 04:12, on Zulip):
                    // Check if item_name belogns to `impl SomeTrait for SomeItem`
                    // This gives precedence to `impl SomeItem`:
                    // Although having both would be ambiguous, use impl version for compat. sake.
                    // To handle that properly resolve() would have to support
                    // something like [`ambi_fn`](<SomeStruct as SomeTrait>::ambi_fn)
                    if kind.is_none() {
                        kind = resolve_associated_trait_item(did, item_name, ns, &self.cx)?;
                    }
                    debug!("got associated item kind {:?}", kind);
Joshua Nelson (Jul 20 2020 at 04:12, on Zulip):

'got associated item kind' just isn't in the log at all

Joshua Nelson (Jul 20 2020 at 04:13, on Zulip):

and I only call this function from here so it's definitely running

Joshua Nelson (Jul 20 2020 at 04:15, on Zulip):

oh hold on I have a sinking suspicion that it's reporting the wrong error for multiple matches

Joshua Nelson (Jul 20 2020 at 04:16, on Zulip):

god damn it people

    let hir_id = match cx.as_local_hir_id(item.def_id) {
        Some(hir_id) => hir_id,
        None => {
            // If non-local, no need to check anything.
            return;
        }
    };
Joshua Nelson (Jul 20 2020 at 04:16, on Zulip):

this is in the error reporting for an ambiguous link

Joshua Nelson (Jul 20 2020 at 04:18, on Zulip):

presumably struct_span_lint_hir has a counterpart that takes DefIds?

eddyb (Jul 20 2020 at 04:18, on Zulip):

@Joshua Nelson highly suspecting item.def_id is not what you want to emit the lint on

eddyb (Jul 20 2020 at 04:19, on Zulip):

it can't possibly not be local

Joshua Nelson (Jul 20 2020 at 04:19, on Zulip):

this is a good point

eddyb (Jul 20 2020 at 04:19, on Zulip):

the HirId lints take is the thing in the local crate you want to warn about

Joshua Nelson (Jul 20 2020 at 04:19, on Zulip):

unless it's a re-export, whic it's not

eddyb (Jul 20 2020 at 04:19, on Zulip):

then you need to pass the reexport DefId, or silence it

eddyb (Jul 20 2020 at 04:19, on Zulip):

pick your poison :P

Joshua Nelson (Jul 20 2020 at 04:20, on Zulip):

I'll worry about re-exports later

eddyb (Jul 20 2020 at 04:20, on Zulip):

how did that trigger here though?

eddyb (Jul 20 2020 at 04:20, on Zulip):

with just a local struct

Joshua Nelson (Jul 20 2020 at 04:21, on Zulip):

uhh apparently it didn't

Joshua Nelson (Jul 20 2020 at 04:21, on Zulip):

I'm so confused

Joshua Nelson (Jul 20 2020 at 04:22, on Zulip):

let me try assert!(candidates.len() >= 2);

Joshua Nelson (Jul 20 2020 at 04:22, on Zulip):

I think it won't behave properly if there are no candidates

Joshua Nelson (Jul 20 2020 at 04:23, on Zulip):

nope, no assertion failure

Joshua Nelson (Jul 20 2020 at 04:23, on Zulip):

ugh I hate rustdoc

Joshua Nelson (Jul 20 2020 at 04:24, on Zulip):

oh ffs

 value_ns: match self.resolve(
                                path_str,
                                disambiguator,
                                ValueNS,
                                &current_item,
                                base_node,
                                &extra_fragment,
                            ) {
                                Err(ErrorKind::AnchorFailure(msg)) => {
                                    anchor_failure(cx, &item, &ori_link, &dox, link_range, msg);
                                    continue;
                                }
                                x => x.ok(),
                            }
                            .and_then(|(res, fragment)| {
                                // Constructors are picked up in the type namespace.
                                match res {
                                    Res::Def(DefKind::Ctor(..), _) | Res::SelfCtor(..) => None,
                                    _ => match (fragment, extra_fragment) {
                                        (Some(fragment), Some(_)) => {
                                            // Shouldn't happen but who knows?
                                            Some((res, Some(fragment)))
                                        }
                                        (fragment, None) | (None, fragment) => {
                                            Some((res, fragment))
                                        }
                                    },
                                }
Joshua Nelson (Jul 20 2020 at 04:24, on Zulip):

it completely ignores the error

Joshua Nelson (Jul 20 2020 at 04:24, on Zulip):

it assumes it was a resolution failure :face_palm:

Joshua Nelson (Jul 20 2020 at 04:27, on Zulip):

let's just copy/paste 50 lines of code, that seems like a reasonable thing to do

Joshua Nelson (Jul 20 2020 at 04:28, on Zulip):

lmao there we go

warning: `MyStruct::clone` is an associated function, an associated function, an associated function, an associated function, and an associated function
Joshua Nelson (Jul 20 2020 at 04:28, on Zulip):

well at least it's something

Joshua Nelson (Jul 20 2020 at 04:29, on Zulip):

it looks like it's considering the same trait multiple times?

Joshua Nelson (Jul 20 2020 at 04:30, on Zulip):

yeah [DEBUG rustdoc::passes::collect_intra_doc_links] considering explicit impl for trait DefId(2:1636 ~ core[92f6]::clone[0]::Clone[0]) shows up 5 times

Joshua Nelson (Jul 20 2020 at 04:30, on Zulip):

this is coming from all_traits

Joshua Nelson (Jul 20 2020 at 04:31, on Zulip):

I'm out of time for tonight, I'll pick this back up later

Joshua Nelson (Jul 20 2020 at 04:31, on Zulip):

WIP at https://github.com/rust-lang/rust/pull/74489/ in case you're interested

Joshua Nelson (Jul 20 2020 at 04:37, on Zulip):

oh and when you get a chance do you know where to find the source for all_traits? All I found was the one in suggest.rs which itself is calling the query, so that's not it.

Joshua Nelson (Jul 20 2020 at 04:38, on Zulip):

oh it's right next to it lol, compute_all_traits

Joshua Nelson (Jul 20 2020 at 04:39, on Zulip):

but yeah I think something's buggy there

Joshua Nelson (Jul 20 2020 at 22:19, on Zulip):

so @Manish Goregaokar suggested that I give up on ambiguity errors for now until there's a way to disambiguate (https://github.com/rust-lang/rust/issues/74563)

Joshua Nelson (Jul 20 2020 at 22:19, on Zulip):

I'm currently trying to resolve associated types

Joshua Nelson (Jul 20 2020 at 22:20, on Zulip):

but something is fishy with for_each_relevant_impl

Joshua Nelson (Jul 20 2020 at 22:21, on Zulip):

it definitely calls it here: https://github.com/rust-lang/rust/pull/74489/files#diff-2bcad7b16b56ef2ebb92f8e60ae33773R474
because I'm getting the debug output

[DEBUG rustdoc::passes::collect_intra_doc_links] looking for associated item named In
put for item DefId(0:3 ~ str_from[8787]::MyStruct[0])
[DEBUG rustdoc::passes::collect_intra_doc_links] considering explicit impl for trait
DefId(0:9 ~ str_from[8787]::T[0])
[DEBUG rustdoc::passes::collect_intra_doc_links] considering item (DefId(0:10 ~ str_from[8787]::T[0]::Input[0]), Type)
[DEBUG rustdoc::passes::collect_intra_doc_links] considering explicit impl for trait DefId(0:12 ~ str_from[8787]::T1[0])
Joshua Nelson (Jul 20 2020 at 22:21, on Zulip):

but the closure is never called, it skips straight over it as if there are no trait impls!

Joshua Nelson (Jul 20 2020 at 22:23, on Zulip):

... I'm being dumb, my test case was wrong lol

Last update: Sep 28 2020 at 16:15UTC