Should doctests on local let statements be found / executed?

[TedDriggs]

Consider the following code:

#![allow(dead_code)]

fn main() {
    /// Here, we initialize our thing to two.
    ///
    /// ```
    /// panic!("oh no what does rustdoc do here");
    /// ```
    let asdf = 2;
    
    println!("hello {}!", asdf);
}

RLS and Racer would like to be able to show docs for asdf on hover in the println! statement. The markdown block in the example would successfully be desugared into a #[doc="..."] attribute, so everything seems fine. However, the doctest in this won't be found or executed today.

Tagging @QuietMisdreavus, and @Ralith, who all weighed in on IRC.

This relates to racer-rust/racer#740.

[QuietMisdreavus]

(imperio on IRC is actually @GuillaumeGomez, ftr)

[QuietMisdreavus]

cc @rust-lang/dev-tools

[oli-obk]

If they are not executed, they need to be linted about. My vote is for executing them, since that's the least surprising thing

[GuillaumeGomez]

I'm mostly in favor of this, however: why adding doc comments in code? I can't see any use for this...

[TedDriggs]

@GuillaumeGomez in IDEs, RLS/Racer would show these docs when autocompleting or hovering over local variable usages. JS and TypeScript already do this in a variety of editors (you can try it in VSCode) and in large functions or deeply-nested closures it's very useful.

@oli-obk I agree with execution-by-default as the least-surprising policy. The test would only have access to the crate's public API, but that's already true for doc-tests on non-public functions or types.

[steveklabnik]

I personally wouldn't expect doc comments to work on anything other than top-level items.

[TedDriggs]

wouldn't expect them to work

@steveklabnik do you mean they should be a compile error, or that tooling should ignore them? Making them a compiler error would break code that I (and possibly others?) have written.

[steveklabnik]

I'm surprised that they weren't a compiler error already, but given that they aren't, I agree it's not necessarily feasible to make them into an error.

I'd expect tooling to ignore them. Again, just speaking personally.

[GuillaumeGomez]

I agree with @steveklabnik. If this is needed and everyone agree on it, I'll make them go into hard error.

[TedDriggs]

That would break already-shipped code, so I don't think a hard error is a good idea.

[oli-obk]

I'm sure it would go through the warning -> deny lint -> hard error cycle

[TedDriggs]

My larger objection to disallowing comments on let statements is that denying them takes away something of value for no clear gain. RLS and Racer can both take advantage of doc attributes on let statements to show explanatory text on hover at usage sites or in completion lists.

One-line comments explaining the purpose of local variables are hardly rare in complex functions, but neither Racer nor RLS would want to try and pair regular comments up with statements for the use cases above; Racer masks comments before analysis, and I'm not sure how RLS would even see comments given that it's using the compiler for more of its data.

[QuietMisdreavus]

I'm in favor of leaving them there, executing the doctests (with the caveat of only being able to access public items from the crate, just like doctests on private items), but not surfacing them in rustdoc. If it's possible to surface the doc attributes on the items for things like racer or RLS, then I'm for it. If people want to document individual bindings, then I say we not only let them, but also let their IDE pick up on it.

[steveklabnik]

no clear gain.

To me, the gain is basically support for a very niche thing, once we declare we support this, we have to keep doing it forever. In this case, given that we already do, that might be past...

[brson]

I am not a fan of this feature, but if it ends up being kept and considered documentation, then I think we need to run tests on them.

[brson]

My preference is to stop parsing these as attributes and turn them into a warning forever.

[nrc]

We discussed this at the dev-tools meeting today. We did not reach consensus, though I think we agree that the current situation needs addressing.

There was some debate about whether this feature was necessary and the cost of removing it.

Assuming the feature stayed, we thought it was probably best to run tests if that could be implemented. If that is not possible, then tests should be linted against.

I think the next step here is deciding if we want to keep allowing doc comments on statements. It feels pretty weird to have doc comments on non-public things and it is a pretty niche feature. OTOH, it follows fairly naturally from allowing attributes on statements and does not add complexity (beyond doc tests). Removing the feature would mean treating the doc comment like a normal comment and issuing a warning (so would be technically backwards compatible).

cc @rust-lang/lang

[GuillaumeGomez]

I opened a PR for this thinking the debate was already over but apparently it isn't. I linked it so you can see how much lines it'd require to make those comments "forbidden" (I think we should put them into warnings instead of hard errors, I'll update my PR later).

[nikomatsakis]

This seems like one of those cases where I don't think it is worth breaking existing code. Issueing a warning seems ok. But maybe we can estimate the impact? I can do a crater run on @GuillaumeGomez's PR.

[GuillaumeGomez]

Just wait that I turn the hard errors into warnings then please @nikomatsakis :D

[nikomatsakis]

Crater run failed because the build of this branch failed. Skim to the end of the log.

[GuillaumeGomez]

@nikomatsakis: that's what I said...

[nikomatsakis]

An update: we've been trying to get a sense for how frequently these occur in practice, but we were hampered by the fact that the build depends on various crates (notably libunicode-bidi) that make use of doc-comments on locals; so a PR that turns them into a hard-error doesn't work so well. I think that @GuillaumeGomez was going to investigate having a build that -- when building rustc itself -- just let them be warnings, so that we could run crater (or some other tool) and use the regressions to figure out how many crates are affected (note that if we just issue warnings, there will be zero regressions, so we'll get zero data).

[GuillaumeGomez]

I'll make a lint out of it to make it into a hard error while running rustc. It'll take a bit of time to be able to get it done.

[nikomatsakis]

We're still encountering some troubles getting a decent survey of how much this "feature" is used in the wild. I think I personally am fine with just removing support for ///-comments on literals (using a future-compatibility warning, of course, not a hard break), even without data on what number of crates in the wild are affected. The only alternative seems to be implementing some reasonable semantics, which I would also support personally, but I sensed some reluctance in the wild.

I think those "reasonable semantics" would be:

• run the tests (also a compatibility hazard, of course!)
• supply doc info to RLS etc (as already happens, iiuc)

[nikomatsakis]

I'm marking this as both T-lang and T-dev-tools, since I think it impacts both. In the interest of moving on with our lives, I'm going to propose that we make doc comments on local variables an error (using a future-compatibility warning), rather than trying to give them reasonable semantics. I do not believe they were intended to be supported, and that seemed to be the general feeling of most commenters.

@rfcbot fcp merge

[Manishearth]

I don't think we should actually make it an error -- I feel we do this too often and too quickly, and we should really be reducing such breakages. In this case doc comments on statements don't hurt anyone, so it would be nicer if it was just a warning, maybe becoming an error after many (not a few) release cycles (but even if it doesn't that would be ok imo)

[rfcbot]

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged teams:

• @aturon
• @brson
• @eddyb
• @japaric
• @jonathandturner
• @killercup
• @michaelwoerister
• @nikomatsakis
• @nrc
• @pnkfelix
• @withoutboats

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[joshtriplett]

@nikomatsakis If I understand the issue correctly: attaching attributes (including documentation attributes via the /// syntactic sugar) to statements is something that existing crates do rely on, and would find hard to eliminate. (Not necessarily with tests, but in general.) As an example, the error-chain crate has intricate macros that carry attributes around, and it relies on the ability to attach those attributes to statements. (I ran into that when attempting to make error-chain work on Rust 1.10.)

[GuillaumeGomez]

Well, it's already implemented as a lint. :)

[Mark-Simulacrum]

Another (related) issue: #21823. I assume there are more cases; I'm not sure but I think the lint doesn't cover the match arm case at least...

[GuillaumeGomez]

@Mark-Simulacrum: It is: https://github.com/rust-lang/rust/pull/43009/files#diff-611a5e56abd9b4488d52f9f830688d5fR19

[sophiajt]

What the status of this? Seems like folks are disinterested in merging it?

[nikomatsakis]

@Manishearth

I don't think we should actually make it an error -- I feel we do this too often and too quickly, and we should really be reducing such breakages. In this case doc comments on statements don't hurt anyone, so it would be nicer if it was just a warning, maybe becoming an error after many (not a few) release cycles (but even if it doesn't that would be ok imo)

That is indeed standard procedure and precisely what I was proposing (that is, having a /// comment attached to a statement would become an error eventually).

The alternatives that I see:

• Ignore it, as today.
• Warn (in rustdoc?) if it contains tests.
• Detect tests in rustdoc and execute them.
• Error (what was proposed).

---

@joshtriplett

attaching attributes (including documentation attributes via the /// syntactic sugar) to statements is something that existing crates do rely on

I think we are only talking about /// (and the desugared #[doc]) form -- unless I am very confused! Other attributes would continue to be accepted.

[Manishearth]

That is indeed standard procedure

I know, I'm suggesting that we should stop doing it so often, or at least slow down the process.

Currently we add the warning and then turn it into a hard error in very few cycles. I'd prefer an approach where it's turned into a warning and perhaps eventually removed, but that "eventually" could be in a year or so, not a few releases.

Basically, when upgrading Gecko's rust version (we usually upgrade every two) we've hit problems with these warning-turned-errors rather often (and if we've hit it others have too, Gecko's only has a medium-sized amount of rust code), and I think that we should be far more careful about technically-breaking changes like this. Once it's a warning there's extremely little benefit to making it an error, and a lot of risk in doing so. There's no reason to rush it -- of course leaving deprecation warnings around forever will just lead to accumulated cruft, but that doesn't mean we should get it done quickly.

A six week release cycle is great for Rust, but that's not the cycle all software moves at, and there's no reason for everything in Rust to follow that breakneck speed.

If you look at this year's survey results there still is a significant fraction of folks experiencing non-dependency-related stable rustc upgrade breaking changes. It seems like these are all minor, but we really should be reducing the risk here and becoming much more careful about the kinds of breaking changes we allow. Soundness fixes are great, but this kind of thing doesn't harm anyone once it's a warning; we should let it bake for quite a while before making it an error.

I guess it's kind of not the venue to appeal against a more general policy, just that this is the perfect example of a thing that would really be fine as a warning (perhaps with an "this will eventually be a hard error" message) that we're considering putting through this process.

[joshtriplett]

On July 29, 2017 12:29:12 PM PDT, Niko Matsakis ***@***.***> wrote: @joshtriplett > attaching attributes (including documentation attributes via the /// syntactic sugar) to statements is something that existing crates do rely on I think we are only talking about `///` (and the desugared `#[doc]`) form -- unless I am very confused! Other attributes would continue to be accepted.

The desugared form is exactly the concern here; macros can copy all attributes from a statement fed into it onto every statement it generates.

[sophiajt]

It seems we're not reaching consensus and that the discussion has stalled. Should we opt to take no action?

[nrc]

To summarise the current status: we currently have a lint that is warn-by-default for doc comments on local statements. I think that is good. We might want to go further:

• we could turn those warnings into errors (deny by default lint or hard error) either sooner or later
• we could also do something specific about doc tests
• we could change our mind and make doc comments legal (either allow-by-default lint, or no lint)

I think we should just leave things as they are: a warning here seems fine, we might make the lint deny-by-default in the long term, but I don't think we need to track that. I think a warning on all doc comments in functions is fine, doc tests in such places are going to be rare and there is already some warning. This is really edge-casey and is not worth spending this much time on.

If anyone feels really strongly about this (like strongly enough to push it through discussion and then do any implementation required) speak up in the next few days, otherwise lets close this issue. (I'm explicitly not doing an rfcbot thing, I think this is a small enough thing and has been open long enough that people can opt out of consensus, rather than in).