Fix panic when reexporting primitive type in rustdoc #67972

GuillaumeGomez · 2020-01-07T13:41:09Z

For info: the reexported primitive type won't appear in the documentation. It seems to me to be the expected behavior.

rust-highfive · 2020-01-07T14:01:48Z

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2020-01-07T13:41:46.0823479Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-01-07T13:41:46.0836736Z ##[command]git config gc.auto 0
2020-01-07T13:41:46.0841561Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-01-07T13:41:46.0845533Z ##[command]git config --get-all http.proxy
2020-01-07T13:41:46.0850576Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/67972/merge:refs/remotes/pull/67972/merge

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Mark-Simulacrum · 2020-01-07T14:25:06Z

No, I think that's not quite expected behavior, if I'm following you correctly. I would be fine with either pub use $ty where the type links to primitive docs (if available, currently I believe only in std, though ideally we would change that), or merely a full inlining that treats the new location as the "source of truth", i.e. having the primitive types table there.

GuillaumeGomez · 2020-01-07T14:50:32Z

I don't see any case where reexporting a primitive type as is would be useful. If we're talking about an alias, it'd make sense but a primitive type, definitely not.

Mark-Simulacrum · 2020-01-07T16:21:39Z

It is definitely useful, see the PR that brought this up - #67637.

GuillaumeGomez · 2020-01-07T20:36:45Z

Damn it, that's a good point! Ok, adding it...

GuillaumeGomez · 2020-01-07T23:33:51Z

It now appears in the documentation as well.

Mark-Simulacrum

r=me with nits addressed

Thanks for getting to this so quickly!

Mark-Simulacrum · 2020-01-07T23:35:19Z

src/librustdoc/clean/types.rs

+            hir::PrimTy::Int(syntax::ast::IntTy::I128) => PrimitiveType::I128,
+            hir::PrimTy::Int(syntax::ast::IntTy::Isize) => PrimitiveType::Isize,
+            hir::PrimTy::Float(syntax::ast::FloatTy::F32) => PrimitiveType::F32,
+            hir::PrimTy::Float(syntax::ast::FloatTy::F64) => PrimitiveType::F64,


I think we already ave impls for the IntTy/FloatTy types so maybe we can just call into() on that?

Mark-Simulacrum · 2020-01-07T23:36:13Z

src/librustdoc/clean/utils.rs

+        "str" => tcx.lang_items().str_impl(),
+        "bool" => tcx.lang_items().bool_impl(),
+        "char" => tcx.lang_items().char_impl(),
+        _ => None,


Seems like this should not be a _ but rather an error (e.g., panic!()).

rust-highfive · 2020-01-07T23:53:27Z

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2020-01-07T23:34:01.4255501Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-01-07T23:34:01.4266131Z ##[command]git config gc.auto 0
2020-01-07T23:34:01.4268459Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-01-07T23:34:01.4270728Z ##[command]git config --get-all http.proxy
2020-01-07T23:34:01.4272986Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/67972/merge:refs/remotes/pull/67972/merge

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

ollie27 · 2020-01-08T00:31:27Z

I don't think that the primitive type docs should be inlined into the std::primitive module because it will unnecessarily duplicate the docs. It should just display pub use bool; etc.

Mark-Simulacrum · 2020-01-08T00:39:08Z

Yes, I would prefer that as well -- though it would be great to get the linking working too (even better if we can link from core to std, but I think that's not possible -- i.e., https://doc.rust-lang.org/nightly/core/primitive.char.html not existing is not great).

GuillaumeGomez · 2020-01-08T10:05:56Z

I can link to std directly if you prefer?

Mark-Simulacrum · 2020-01-08T12:28:27Z

I don't know what would work best. It's probably a bit odd to have links in core docs to std docs. Maybe we can move the primitive doc definitions to core and link to those across the board?

OTOH, for ~all other types we re-export from core, we duplicate their documentation, right? So maybe it's a bit weird to only have one location for the primitive types.

GuillaumeGomez · 2020-01-08T16:27:13Z

So what should I do? Link to core or to std? :)

Mark-Simulacrum · 2020-01-08T19:15:44Z

How about this:

In core, we will make the primitive module the canonical location for the primitive docs (i.e., no top-level primitives in rustdoc).

In std, the module would render as pub use [i32](core/primitive/primitive.i32.html).

Does that sound reasonable?

ollie27 · 2020-01-08T21:38:58Z

In core, we will make the primitive module the canonical location for the primitive docs (i.e., no top-level primitives in rustdoc).

What about the primitive types that can't be reexported in core::primitive i.e. array, fn, pointer, reference, slice, tuple, unit and never? Where would they be documented?

Mark-Simulacrum · 2020-01-08T22:01:17Z

Presumably we could document them in the same module, but that might also be a reason to not do it.

I'm fine with the somewhat more minimal option of just linking to existing docs from std and showing all primitive re-exports as pub use, indeed I might prefer this (but to be honest I am not sure). I think @dtolnay thought that pub use would be the right approach for now at least though.

ollie27 · 2020-01-23T21:10:20Z

src/librustdoc/clean/inline.rs

+            let def_id = match clean::utils::res_to_def_id(cx, &item.res) {
+                Some(did) => did,
+                None => continue,
+            };
+            if did == def_id || !visited.insert(def_id) {
+                continue;
+            }


As the def_id is only used to avoid infinite recursion, we only really care about the def_id of modules so using mod_def_id() should be all that's needed to fix the ICE:

if let Some(def_id) = item.res.mod_def_id() { if did == def_id || !visited.insert(def_id) { continue; } }

ollie27 · 2020-01-23T21:36:55Z

src/librustdoc/clean/inline.rs

+                record_extern_fqn(cx, def_id, TypeKind::Primitive);
+                cx.renderinfo.borrow_mut().inlined.insert(def_id);
+                items.push(clean::Item {
+                    source: cx.tcx.def_span(def_id).clean(cx),
+                    name: Some(item.ident.clean(cx)),
+                    attrs: cx.tcx.get_attrs(def_id).clean(cx),
+                    inner: clean::ItemEnum::PrimitiveItem(clean::PrimitiveType::from(primitive)),
+                    visibility: clean::Public,
+                    stability: cx.tcx.lookup_stability(def_id).clean(cx),
+                    deprecation: cx.tcx.lookup_deprecation(def_id).clean(cx),
+                    def_id,
+                });


It might be better to synthesize a clean::Import here rather than trying to directly inline primitive types.

ollie27 · 2020-01-26T21:47:08Z

I had a go at fixing this such that the re-exports show up as pub use bool; with working links: #68556

rust-highfive assigned Mark-Simulacrum Jan 7, 2020

Fix panic when reexporting primitive type in rustdoc

18d211e

GuillaumeGomez force-pushed the reexport-primitive-panic branch from ed564b2 to 18d211e Compare January 7, 2020 14:04

Make reexported primitive types appear in modules

21e69e6

Mark-Simulacrum approved these changes Jan 7, 2020

View reviewed changes

hdhoang added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 9, 2020

ollie27 reviewed Jan 23, 2020

View reviewed changes

ollie27 mentioned this pull request Jan 26, 2020

rustdoc: Fix re-exporting primitive types #68556

Merged

bors closed this in edfa0f4 Jan 29, 2020

GuillaumeGomez deleted the reexport-primitive-panic branch February 10, 2020 15:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix panic when reexporting primitive type in rustdoc #67972

Fix panic when reexporting primitive type in rustdoc #67972

GuillaumeGomez commented Jan 7, 2020

rust-highfive commented Jan 7, 2020

Mark-Simulacrum commented Jan 7, 2020

GuillaumeGomez commented Jan 7, 2020

Mark-Simulacrum commented Jan 7, 2020

GuillaumeGomez commented Jan 7, 2020

GuillaumeGomez commented Jan 7, 2020

Mark-Simulacrum left a comment

Mark-Simulacrum Jan 7, 2020

Mark-Simulacrum Jan 7, 2020

rust-highfive commented Jan 7, 2020

ollie27 commented Jan 8, 2020

Mark-Simulacrum commented Jan 8, 2020

GuillaumeGomez commented Jan 8, 2020

Mark-Simulacrum commented Jan 8, 2020

GuillaumeGomez commented Jan 8, 2020 •

edited

Loading

Mark-Simulacrum commented Jan 8, 2020

ollie27 commented Jan 8, 2020

Mark-Simulacrum commented Jan 8, 2020

ollie27 Jan 23, 2020

ollie27 Jan 23, 2020

ollie27 commented Jan 26, 2020

Fix panic when reexporting primitive type in rustdoc #67972

Fix panic when reexporting primitive type in rustdoc #67972

Conversation

GuillaumeGomez commented Jan 7, 2020

rust-highfive commented Jan 7, 2020

Mark-Simulacrum commented Jan 7, 2020

GuillaumeGomez commented Jan 7, 2020

Mark-Simulacrum commented Jan 7, 2020

GuillaumeGomez commented Jan 7, 2020

GuillaumeGomez commented Jan 7, 2020

Mark-Simulacrum left a comment

Choose a reason for hiding this comment

Mark-Simulacrum Jan 7, 2020

Choose a reason for hiding this comment

Mark-Simulacrum Jan 7, 2020

Choose a reason for hiding this comment

rust-highfive commented Jan 7, 2020

ollie27 commented Jan 8, 2020

Mark-Simulacrum commented Jan 8, 2020

GuillaumeGomez commented Jan 8, 2020

Mark-Simulacrum commented Jan 8, 2020

GuillaumeGomez commented Jan 8, 2020 • edited Loading

Mark-Simulacrum commented Jan 8, 2020

ollie27 commented Jan 8, 2020

Mark-Simulacrum commented Jan 8, 2020

ollie27 Jan 23, 2020

Choose a reason for hiding this comment

ollie27 Jan 23, 2020

Choose a reason for hiding this comment

ollie27 commented Jan 26, 2020

GuillaumeGomez commented Jan 8, 2020 •

edited

Loading