Remove weird edge case for "Type::inner_def_id" #90726
Conversation
GuillaumeGomez
added
the
T-rustdoc
rust-highfive
added
the
S-waiting-on-review
|
It appears that this edge case was added as part of #43560. That PR's description says this:
Do you know of anything that could break if this edge case is removed? |
|
The obvious thing to test is whether |
I think the scope is larger though; what about links to |
Good catch! It removed all implementations. So this fix is invalid. I think I'll instead filter out references from the search generation. |
|
Which isn't possible in this PR, I'll comment in the other PR instead. Thanks to both of you! |
…less-search-index-data, r=notriddle,camelid Remove potential useless data for search index I uncovered this case when working on rust-lang#90726 to debug rust-lang#90385. Explanations: if we have a full generic, we check if it has generics then we do the following: * If it has only one generic, we remove one nested level in order to not keep the "parent" generic (since it has empty name, it's useless after all). * Otherwise we add it alongside its generics. However, I didn't handle the case where a generic had no generics. Meaning that we were adding items with empty names in the search index. So basically useless data in the search index. r? `@camelid`
It's kind of concerning that this was not detected by the test suite. Could you add some tests for the behavior that the edge case causes? |
|
We don't test primitive types' page much and I'm not sure how to test it. I think we'd need to write a test which redefines the |
|
I opened #90775. I'll try to do it in the next days. |
…case, r=Manishearth Ignore `reference`s in "Type::inner_def_id" Fixes rust-lang#90775. Reopening of rust-lang#90726. As discussed on [zulip](https://rust-lang.zulipchat.com/#narrow/stream/266220-rustdoc/topic/rendering.20for.20reference.20primitive.20doc.20page), the reference page shouldn't list these implementations (since they are listed on the types and on the traits in any case). And more generally, you don't implement something on a reference but on something behind a reference. I think it's the important point. So currently it looks like this:  With this PR, only the implementations over generics behind a reference are kept. You can test it [here](https://rustdoc.crud.net/imperio/def-id-remove-weird-case/std/primitive.reference.html). cc `@camelid`
…case, r=Manishearth Ignore `reference`s in "Type::inner_def_id" Fixes rust-lang#90775. Reopening of rust-lang#90726. As discussed on [zulip](https://rust-lang.zulipchat.com/#narrow/stream/266220-rustdoc/topic/rendering.20for.20reference.20primitive.20doc.20page), the reference page shouldn't list these implementations (since they are listed on the types and on the traits in any case). And more generally, you don't implement something on a reference but on something behind a reference. I think it's the important point. So currently it looks like this:  With this PR, only the implementations over generics behind a reference are kept. You can test it [here](https://rustdoc.crud.net/imperio/def-id-remove-weird-case/std/primitive.reference.html). cc ``@camelid``
Needed for #90385.
In this case, the generic is not considered as a generic because it is behind a reference. I'm surprised we had this for so long going unnoticed...
Extra explanations: we discovered it #90385 because since
Primitive::Referenceneeds thecache, we simply ignored it before becausedef_id_no_primitivesreturnedNone. But since we're now using it everywhere, it is not discarded anymore, which allowed us to uncover this bug.cc @jyn514
r? @camelid