Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Highlight when APIs panic in rustdoc #46963

Open
Manishearth opened this issue Dec 23, 2017 · 17 comments
Open

Highlight when APIs panic in rustdoc #46963

Manishearth opened this issue Dec 23, 2017 · 17 comments
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@Manishearth
Copy link
Member

When an API specifically has a "will panic on X condition" guarantee (e.g. indexing APIs, unwrap, expect, perhaps allocating APIs) it would be nice if the stdlib could highlight these the way we do unsafe methods.

https://twitter.com/0x424c41434b/status/944316751919140865

Worth prototyping this behind a feature flag for now, and then discussing whether we should be stabilizing it (or keeping it forever unstable for the stdlib).

cc @rust-lang/docs

(I'm mentoring 0x424c41434b on implementing this, even if we eventually decide not to do this)

@Manishearth Manishearth added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-dev-tools-rustdoc labels Dec 23, 2017
@Manishearth
Copy link
Member Author

The relevant code is probably in doc_impl_item, see where we call document_stability to do something similar for stability messages.

You should be able to find the attribute on the clean::Item https://michael-f-bryan.github.io/rustc-internal-docs/rustdoc/clean/struct.Item.html

@bluss
Copy link
Member

bluss commented Dec 23, 2017

We already have a strong convention -- panics should be mentioned in the doc. In rust-lang/rust, under the # Panics heading (example). One part of this issue is to increase coverage? To make sure even the "obvious" cases are covered, for example the trait method Index::index wherever implemented with panicking bounds checks.

How does a label relate to the already present documentation?

Also, the twitter thread has some misconceptions, like Vec::new and String::new can panic (and they can't). And that panics are only catchable at thread boundaries (but we have catch_unwind).

@Manishearth
Copy link
Member Author

The label is what we should turn the present documentation convention into.

I don't think the issues with the thread are relevant. For vec::new there are proposals for oom=panic which I thought had landed, and I corrected the thing about catch_unwind.

@GuillaumeGomez
Copy link
Member

@Manishearth: Can you write it on the roadmap of the doc team so we can discuss about it in our next meeting (which will be in 2 weeks) please?

@Manishearth
Copy link
Member Author

@GuillaumeGomez rust-lang/rust-roadmap-2017#20 ? It doesn't seem like folks leave todo items on that issue. Could you mention this in the appropriate place?

@frewsxcv
Copy link
Member

just added this to the docs team meeting agenda

@steveklabnik
Copy link
Member

steveklabnik commented Dec 25, 2017 via email

@Manishearth
Copy link
Member Author

Manishearth commented Dec 25, 2017 via email

@sfackler
Copy link
Member

How is oom=panic related to the capacity used with Vec::new and String::new?

@Manishearth
Copy link
Member Author

Manishearth commented Dec 25, 2017 via email

@sfackler
Copy link
Member

@Manishearth
Copy link
Member Author

Manishearth commented Dec 25, 2017 via email

@QuietMisdreavus
Copy link
Member

We talked about this a little at the docs team meeting today. Overall, we think it could be a good thing, but we'd like to see an initial implementation before casting final judgement. If you're still interested, feel free to sketch out something.

@StaticallyTypedAnxiety
Copy link

Hello,

Sorry kind of nervous, first comment on git. I thought this would be great and I would like to work on it if that is okay

@Manishearth
Copy link
Member Author

Sure! My second comment has some info on how to get started with this, let me know (here or on Twitter) if you have questions!

@QuietMisdreavus
Copy link
Member

It's also worth adding whatever processing to the document function so that it can end up on bare function pages too. (Bare functions are rendered in item_function, which defers its stability/markdown printing to document.)

Otherwise, feel free to work on it! If you have any questions and @Manishearth isn't available, feel free to ask me! I love to help people work on rustdoc.

@Manishearth
Copy link
Member Author

@BitExplosion hey, have you had a chance to look into this yet?

@jkordish jkordish added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Apr 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

9 participants