Add stability tags to ImportItem. #83900

torhovland · 2021-04-05T19:00:02Z

rust-highfive · 2021-04-05T19:00:05Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @CraftSpider (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

CraftSpider · 2021-04-06T07:37:28Z

Could you include a picture/screenshot of the new output, and add a test case to ensure it is output or not as expected? The test should go in src/test/rustdoc, you can look at other tests there for examples.

torhovland · 2021-04-06T08:19:53Z

CraftSpider · 2021-04-07T16:16:49Z

This just needs a test, updating the labels
@rustbot label: -S-waiting-on-review +S-waiting-on-author

torhovland · 2021-04-08T21:06:17Z

Could you include a picture/screenshot of the new output, and add a test case to ensure it is output or not as expected? The test should go in src/test/rustdoc, you can look at other tests there for examples.

@CraftSpider OK, should be good now.

CraftSpider

Thanks for the patience! CC @jyn514 if you wouldn't mind taking a look, as this affects frontend, otherwise I'm happy to merge this.

src/test/rustdoc/issue-83832.rs

jyn514

Mostly just nits, overall this looks great :)

src/librustdoc/html/render/print_item.rs

jyn514 · 2021-04-13T04:42:39Z

src/librustdoc/html/render/print_item.rs

-                    "<tr><td><code>{}{}</code></td></tr>",
-                    myitem.visibility.print_with_space(cx.tcx(), myitem.def_id, cx.cache()),
-                    import.print(cx.cache(), cx.tcx()),
+                    "<tr class=\"{stab}{add}module-item\">\


Are you sure module-item is correct here? These aren't modules, they're imports.

Introduced an import-item class.

src/librustdoc/html/render/print_item.rs

src/test/rustdoc/issue-83832.rs

jyn514 · 2021-04-13T14:50:49Z

clean::Item is the one you want.

torhovland · 2021-04-14T19:50:00Z

I had to do some more changes in order to support Portability tags, and I've added tests for Deprecated, Portability, and Unstable.

jyn514

Do you mind squashing the commits? 24 is a little much 😅

jyn514 · 2021-04-18T03:56:30Z

src/librustdoc/clean/inline.rs

@@ -477,6 +477,7 @@ fn build_module(
                                }],
                            },
                            did: None,
+                            attrs: None,


Why add attrs as a new field? You can look this up on-demand.

@jyn514 Because if I understand correctly, I would have to do

let attrs = Box::new(cx.tcx().get_attrs(def_id).clean(cx));

in print_item.rs, and it didn't seem appropriate to call clean() methods at that stage, not least because clean() expects a DocContext, and we don't have that in print_item.rs.

Let me know if you see a better way!

it didn't seem appropriate to call clean() methods at that stage, not least because clean() expects a DocContext, and we don't have that in print_item.rs.

Got it, thanks. At this point, I think I would extract a free function in clean::types for stability_class so you only have to pass in the DefId of the item, not a whole clean::Item. Note that stability_class never uses the attributes, only the DefId. I would rather not make rustdoc slower just because the API is bad.

You'll have to also extract free functions for stability, deprecation, and is_fake; all that seems fine.

cc #83183

I could do that, but I would still have to pass in attrs, and it also wouldn't solve the issue with the fake Item. You see, the problem isn't so much with stability_class, but rather with extra_info_tags. It really does seem to need an Item with attrs on it.

Hmm, ok. I guess this is a larger refactor than really belongs in this PR, I opened #84304.

Aha, there's a simpler way: impl Clean for [ast::Attribute] doesn't use DocContext at all:

rust/src/librustdoc/clean/mod.rs

Line 242 in b021bee

Attributes::from_ast(cx.sess().diagnostic(), self, None)

So you can remove the attrs field and call let attrs = Box::new(Attributes::from_ast(tcx.sess().diagnostic(), tcx.get_attrs(def_id), None)); on-demand instead.

OK, should be good now.

GuillaumeGomez · 2021-04-18T14:13:40Z

Yep, CSS changes look good to me (and the rest of the code too 😉 ).

@bors: r=jyn514,GuillaumeGomez

bors · 2021-04-18T14:13:41Z

📌 Commit e2a77b3 has been approved by jyn514,GuillaumeGomez

…illaumeGomez Add stability tags to ImportItem. Fixes rust-lang#83832.

jyn514 · 2021-04-18T14:39:16Z

@GuillaumeGomez this is not yet ready to merge: #83900 (comment)

@bors r-

GuillaumeGomez · 2021-04-18T14:43:50Z

Outch sorry, went too quickly...

src/librustdoc/clean/types.rs

jyn514 · 2021-04-20T04:08:06Z

@bors r+

This is great, thank you for sticking with it!

bors · 2021-04-20T04:08:07Z

📌 Commit 64a68ae has been approved by jyn514

bors · 2021-04-20T06:03:19Z

⌛ Testing commit 64a68ae with merge a70fbf6...

bors · 2021-04-20T08:33:46Z

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing a70fbf6 to master...

rust-highfive assigned CraftSpider Apr 5, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 5, 2021

torhovland marked this pull request as ready for review April 5, 2021 19:49

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 7, 2021

CraftSpider approved these changes Apr 13, 2021

View reviewed changes

src/test/rustdoc/issue-83832.rs Outdated Show resolved Hide resolved

jyn514 reviewed Apr 13, 2021

View reviewed changes

jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 13, 2021

This comment has been minimized.

Sign in to view

jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 14, 2021

This comment has been minimized.

Sign in to view

jyn514 reviewed Apr 18, 2021

View reviewed changes

jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 18, 2021

Tor Hovland added 4 commits April 18, 2021 10:45

Add stability tags to ImportItem.

0c736e9

Added a test.

41f49aa

Give import items their own CSS class.

1e2ab99

Now also displays portability tags.

fca088a

GuillaumeGomez approved these changes Apr 18, 2021

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 18, 2021

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Apr 18, 2021

Rollup merge of rust-lang#83900 - torhovland:issue-83832, r=jyn514,Gu…

40ccb63

…illaumeGomez Add stability tags to ImportItem. Fixes rust-lang#83832.

GuillaumeGomez mentioned this pull request Apr 18, 2021

Rollup of 5 pull requests #84302

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 18, 2021

jyn514 mentioned this pull request Apr 18, 2021

rustdoc: Get rid of Item::Attributes #84304

Open

jyn514 reviewed Apr 18, 2021

View reviewed changes

src/librustdoc/clean/types.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

Put attrs in a Box for memory efficiency.

c975fe2

torhovland force-pushed the issue-83832 branch from 6ba7058 to c975fe2 Compare April 18, 2021 16:02

jyn514 assigned jyn514 and unassigned CraftSpider Apr 18, 2021

No longer need to add attrs to ImportSource.

64a68ae

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 20, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 20, 2021

bors merged commit a70fbf6 into rust-lang:master Apr 20, 2021

rustbot added this to the 1.53.0 milestone Apr 20, 2021

torhovland deleted the issue-83832 branch April 20, 2021 08:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add stability tags to ImportItem. #83900

Add stability tags to ImportItem. #83900

torhovland commented Apr 5, 2021

rust-highfive commented Apr 5, 2021

CraftSpider commented Apr 6, 2021

torhovland commented Apr 6, 2021 •

edited

Loading

CraftSpider commented Apr 7, 2021

torhovland commented Apr 8, 2021

CraftSpider left a comment

jyn514 left a comment

jyn514 Apr 13, 2021

torhovland Apr 14, 2021

This comment has been minimized.

jyn514 commented Apr 13, 2021

torhovland commented Apr 14, 2021

This comment has been minimized.

jyn514 left a comment

jyn514 Apr 18, 2021

torhovland Apr 18, 2021

jyn514 Apr 18, 2021

torhovland Apr 18, 2021

jyn514 Apr 18, 2021

jyn514 Apr 18, 2021

torhovland Apr 19, 2021

GuillaumeGomez commented Apr 18, 2021

bors commented Apr 18, 2021

jyn514 commented Apr 18, 2021

GuillaumeGomez commented Apr 18, 2021

This comment has been minimized.

jyn514 commented Apr 20, 2021

bors commented Apr 20, 2021

bors commented Apr 20, 2021

bors commented Apr 20, 2021

Add stability tags to ImportItem. #83900

Add stability tags to ImportItem. #83900

Conversation

torhovland commented Apr 5, 2021

rust-highfive commented Apr 5, 2021

CraftSpider commented Apr 6, 2021

torhovland commented Apr 6, 2021 • edited Loading

CraftSpider commented Apr 7, 2021

torhovland commented Apr 8, 2021

CraftSpider left a comment

Choose a reason for hiding this comment

jyn514 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

jyn514 commented Apr 13, 2021

torhovland commented Apr 14, 2021

This comment has been minimized.

jyn514 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Apr 18, 2021

bors commented Apr 18, 2021

jyn514 commented Apr 18, 2021

GuillaumeGomez commented Apr 18, 2021

This comment has been minimized.

jyn514 commented Apr 20, 2021

bors commented Apr 20, 2021

bors commented Apr 20, 2021

bors commented Apr 20, 2021

torhovland commented Apr 6, 2021 •

edited

Loading