rustdoc: redirect URLs

[nrc]

cc #35020 which does this properly

r? @alexcrichton

[nrc]

cc @steveklabnik, @ollie27

[alexcrichton]

@nrc so just to clarify, this emits Foo.v.html which redirects to struct.Foo.html?

[ollie27]

As with the previous PR, methods and struct fields are in the same namespace so we get conflicts. For example std::ops::Range has two start.v ids. Is that not a problem?

[nrc]

As with the previous PR, methods and struct fields are in the same namespace so we get conflicts. For example std::ops::Range has two start.v ids. Is that not a problem?

It is, but without changing the canonical URLs/anchors there doesn't seem to be a great way to disambiguate. However, since these are not canonical URLs, it doesn't really matter - for tool users who link to Rustdoc, there will be a very few URLs that don't work properly, but I expect this to be a pretty small edge case.

[nrc]

so just to clarify, this emits Foo.v.html which redirects to struct.Foo.html?

Well, Foo.t.html, but yes that is basically what is happening here, plus the extra anchors for 'sub-items'.

[alexcrichton]

@bors: r+ 525b77e

[ollie27]

There are a few issues with the implementation:

• Nested <a> tags are invalid so you'll need to use another tag for the new ids.
• ids are supposed to be unique so you'll need to call derive_id for all the new ones.
• There should probably be a few new tests 😉.

[alexcrichton]

@bors: r-, ah, good points!

[bors]

⌛ Testing commit 525b77e with merge 8d86593...

[alexcrichton]

@bors: r-

er... maybe a mis-parse?

[bors]

💔 Test failed - auto-win-msvc-64-opt-rustbuild

[nrc]

bors is clearly hungry for this PR, it won't take 'no' for an answer

[nrc]

re tests, does anyone (@ollie27?) have suggestions for how to test the existence of the redirects? I don't see a way to fit it into the existing framework, but I don't know it very well.

[alexcrichton]

I think you can at least test for their existence with the rustdoc tests, and maybe html tags inside them as well?

[ollie27]

Check out test/rustdoc/redirect.rs and test/rustdoc/issue-33302.rs for examples which test the same kind of things.

For example I'd do something like the following for structs:

#![crate_name = "foo"]// @has 'foo/FooStruct.t.html'// @has - '//p/a' 'struct.FooStruct.html'// @has 'foo/struct.FooStruct.html'pubstructFooStruct{// @has - '//*[@id="foo.v"]' 'foo'pubfoo:(),// @has - '//*[@id="dupe.v"]' 'dupe'pubdupe:(),}implFooStruct{// @has - '//*[@id="bar.v"]' 'bar'pubfnbar(){}// @has - '//*[@id="dupe.v-1"]' 'dupe'pubfndupe(){}}

You should be able repeat that for every other item type.

[nrc]

@ollie27 I tried something like this and it doesn't work. I'm not really clear on the semantics of @has, but I'm assuming it is ensuring that the text string occurs in the generated document. So, your example is saying that there should be a link to each of struct.FooStruct.html and FooStruct.t.html, which there isn't - there is only a link to the former. Is my understanding correct? Is there a way to check for the existence of a file?

[ollie27]

Check out src/etc/htmldocck.py for a better explanation than I could give.

[nrc]

Thanks for the link, I knew these must be documented somewhere.

PATH is relative to the output directory. It can be given as - which repeats the most recently used PATH.

This is what was tripping me up.

[nrc]

Updated with tests, spans instead of anchors, and unique ids.

@alexcrichton r?

[alexcrichton]

@bors: r+ 6765f7e

[bors]

⌛ Testing commit 6765f7e with merge 30adf96...

[bors]

💔 Test failed - auto-win-gnu-32-opt-rustbuild

[ollie27]

You'll need to use derive_id for these as well. The existing ids are also missing a call to derive_id so you may as well also fix that while you're at it.

[ollie27]

Same here.

[ollie27]

I was hoping you'd add a new test covering every single item type. For example enum variant struct fields and macros are different so need explicit tests.

Are you going to document this feature anywhere? What kind of stability guarantees do these new URLs have? Can other people use them?

[nrc]

I was hoping you'd add a new test covering every single item type. For example enum variant struct fields and macros are different so need explicit tests.

I suppose a few more tests couldn't hurt. The main code paths are exercised and the coverage seems as good or better than existing Rustdoc tests.

Are you going to document this feature anywhere?

No. I see no point in encouraging use of these until the long-term URL solution is figured out.

What kind of stability guarantees do these new URLs have?

None, use at your own risk.

Can other people use them?

Sure, but I wouldn't encourage it.

You'll need to use derive_id for these as well. The existing ids are also missing a call to derive_id so you may as well also fix that while you're at it.

I was assuming there was some logic behind not calling derive_id here (e.g., you can't have duplicate named fields, so I assume there can't be a name clash).

[ollie27]

The main code paths are exercised and the coverage seems as good or better than existing Rustdoc tests.

The test coverage in rustdoc isn't very good right now so we should be aiming for better coverage.

I was assuming there was some logic behind not calling derive_id here (e.g., you can't have duplicate named fields, so I assume there can't be a name clash).

derive_id is also used by the markdown so I was worried it might conflict there but having a closer look, it turns out the markdown will never create an id containing a . so technically they can't conflict. It wouldn't harm to call derive_id though, we definitely need to call it for the new ones anyway.

[nrc]

@bors r=@alexcrichton

[bors]

📌 Commit 879637f has been approved by @alexcrichton

[bors]

⌛ Testing commit 879637f with merge 6c0d66a...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-cargotest, auto-linux-64-cross-freebsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-no-mir, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-no-mir, auto-win-msvc-64-opt-rustbuild
Approved by: @alexcrichton
Pushing 6c0d66a to master...