rustdoc: fix doctests with non-feature crate attrs #38161

durka · 2016-12-04T19:12:01Z

The book says that any top-level crate attributes at the beginning of a doctest are moved outside the generated fn main, but it was only checking for #![feature, not #![.

These attributes previously caused warnings but were then ignored, so in theory this could change the behavior of doctests in the wild.

rust-highfive · 2016-12-04T19:12:07Z

r? @steveklabnik

(rust_highfive has picked a reviewer for you, use r? to override)

bluss · 2016-12-04T19:47:56Z

This looks good to me, it's just a bugfix, or can it cause trouble?

durka · 2016-12-04T19:58:57Z

@bluss since these attributes were "silently" ignored before, in theory a test like this

/// ```
/// #![no_std]
/// assert_eq!(1 + 1, 2);
/// ```
pub fn foo() {}

passed before but fails after this PR.

However it makes the code agree with what the book previously claimed.

durka · 2016-12-04T20:39:36Z

Ha, I thought that was hypothetical but it was in the example for E0152. Fixing.

durka · 2016-12-05T05:09:35Z

Ah crap, it also fails with multi-line attributes, i.e.

#![cfg_attr(unix,
            feature(foo)]

But code such as

#![feature(x,
           y)]

would have failed before as well.

Really, partition_source() needs to use the parser instead of a simple string substition.

durka · 2016-12-05T06:25:22Z

I went for the easy solution: comment out the affected doctests and leave a FIXME.

GuillaumeGomez · 2016-12-07T08:27:16Z

cc @alexcrichton

alexcrichton · 2016-12-07T08:36:46Z

Hm it sounds like this is a breaking change? Could you elaborate on the precise breakage that's happening here and why you think it's acceptable to land anyway?

durka · 2016-12-07T14:56:12Z

@alexcrichton

Well, I didn't really expect breakage until I found it in the docs of rust itself :)

It may be acceptable to land anyway because

The breakage happens because an attribute which the user intended to take effect was not taking effect (it prints a warning which is usually swallowed by the doctest runner).
It was definitely a bug before and this change brings the code in line with what the docs have always said.

Unfortunately I don't think we can do anything with doctests on crater. But we could try to do some clever grepping for doctests that would be affected.

Edit: and the alternative to breakage is to change the docs to say what the code does, and provide an example of how to put in a non-feature crate attribute that actually works.

durka · 2016-12-09T17:19:58Z

Updated -- I learned from #38239 how to write the test without using run-make.

alexcrichton · 2016-12-11T18:38:31Z

Hm I'm not personally comfortable with the breakage outlined, but others may have different opinions.

steveklabnik · 2016-12-12T19:08:57Z

/cc @rust-lang/core wrt breakage

aturon · 2016-12-13T17:47:00Z

@alexcrichton

To be clear, AIUI, this PR is changing from silently ignoring attributes to actually using them (modulo parse problems). While this could in principle break a test, it seems like a very clear-cut bugfix to me, and personally I would want such tests to break. It's also trivial to fix the breakage: if you delete the attribute, it'll work precisely as it did before.

👍 from me.

alexcrichton · 2016-12-16T00:59:29Z

@aturon in many cases though in the book it was intended for the attribute to show off example usage and shouldn't be removed. For example #![no_std] was accidentally not getting placed on the crate root but it's showing off the various no_std examples.

It's true though that there's always a way to write code which works across all versions here, so in that sense it could be fine to consider this a standard bugfix.

The rustdoc test generation at this point seems so janky though that any "bug fix" is inevitably going to break some examples...

aturon · 2016-12-21T23:53:19Z

@alexcrichton

in many cases though in the book it was intended for the attribute to show off example usage and shouldn't be removed. For example #![no_std] was accidentally not getting placed on the crate root but it's showing off the various no_std examples.

I'm having a hard time understanding what you're saying. Maybe I'm totally misunderstanding the change here, but I thought the attributes would appear in the docs (so would continue to "show off" the attribute) but would also start taking actual effect? In my mind, what we want is for the tested code and the code on the screen to be doing the same thing.

bluss · 2016-12-22T00:04:17Z

I'm for this as a bug fix. Similar to lint changes it can break your tests or CI, but not the crate itself.

alexcrichton · 2016-12-28T17:34:14Z

@aturon to clarify you originally said:

It's also trivial to fix the breakage: if you delete the attribute, it'll work precisely as it did before.

I was mainly responding to that, I believe. The "fix" would defeat the purpose of the original documentation, so this wouldn't want to be something that we'd recommend to crate authors if they hit this breakage.

If we want to consider this a bug fix though and just swallow the breakage then that's ok with me. Authors can always tag examples with ignore to not actually compile anything.

aturon · 2017-01-04T18:48:25Z

@alexcrichton

I was mainly responding to that, I believe. The "fix" would defeat the purpose of the original documentation, so this wouldn't want to be something that we'd recommend to crate authors if they hit this breakage.

Got it. But yeah, I'm comfortable going forward here with the advice you mention.

nikomatsakis · 2017-01-04T20:42:15Z

@brson can we use cargobomb to test for rustdoc tests? I believe so? It'd be nice to have data.

The typical policy for bugfixes like this is to attempt a warning period, though i'm not sure if that is feasible in this case. I feel also that this sounds like a bugfix, and as likely to fix things as to break them.

steveklabnik · 2017-01-23T21:34:19Z

So, I am assigned this PR, but I don't know what to do with it. Who's making the call here?

nikomatsakis · 2017-01-24T21:48:35Z

@steveklabnik seems like a tools team thing?

alexcrichton · 2017-02-04T21:55:25Z

@bors: r+

bors · 2017-02-04T21:55:25Z

📌 Commit ec356bb has been approved by alexcrichton

bors · 2017-02-05T04:00:33Z

⌛ Testing commit ec356bb with merge 4efb8ae...

rustdoc: fix doctests with non-feature crate attrs Fixes #38129. The book says that any top-level crate attributes at the beginning of a doctest are moved outside the generated `fn main`, but it was only checking for `#![feature`, not `#![`. These attributes previously caused warnings but were then ignored, so in theory this could change the behavior of doctests in the wild.

bors · 2017-02-05T05:35:04Z

💔 Test failed - status-travis

alexcrichton · 2017-02-05T05:54:39Z

@bors: retry * segfault in musl tests #38618

…

On Sat, Feb 4, 2017 at 9:35 PM, bors ***@***.***> wrote: 💔 Test failed - status-travis <https://travis-ci.org/rust-lang/rust/builds/198476782> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#38161 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD95ERvQ3nvPpBaw3So7wEh1JPNMN2jks5rZV-KgaJpZM4LDqkM> .

bors · 2017-02-05T05:54:48Z

⌛ Testing commit ec356bb with merge 696f5c1...

rustdoc: fix doctests with non-feature crate attrs Fixes #38129. The book says that any top-level crate attributes at the beginning of a doctest are moved outside the generated `fn main`, but it was only checking for `#![feature`, not `#![`. These attributes previously caused warnings but were then ignored, so in theory this could change the behavior of doctests in the wild.

bors · 2017-02-05T08:24:40Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 696f5c1 to master...

rust-highfive assigned steveklabnik Dec 4, 2016

durka force-pushed the rustdoc-crate-attrs branch from 9424739 to a65d7ad Compare December 4, 2016 19:20

bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Dec 4, 2016

durka force-pushed the rustdoc-crate-attrs branch from a65d7ad to b0e0bf0 Compare December 4, 2016 20:45

durka force-pushed the rustdoc-crate-attrs branch 2 times, most recently from aafc598 to 528c463 Compare December 5, 2016 06:24

durka force-pushed the rustdoc-crate-attrs branch from 528c463 to 9e5ba49 Compare December 5, 2016 08:31

rustdoc: fix doctests with non-feature crate attrs

ec356bb

durka force-pushed the rustdoc-crate-attrs branch from 9e5ba49 to ec356bb Compare December 9, 2016 17:18

bors merged commit ec356bb into rust-lang:master Feb 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustdoc: fix doctests with non-feature crate attrs #38161

rustdoc: fix doctests with non-feature crate attrs #38161

durka commented Dec 4, 2016

rust-highfive commented Dec 4, 2016

bluss commented Dec 4, 2016

durka commented Dec 4, 2016 •

edited

Loading

durka commented Dec 4, 2016

durka commented Dec 5, 2016

durka commented Dec 5, 2016

GuillaumeGomez commented Dec 7, 2016

alexcrichton commented Dec 7, 2016

durka commented Dec 7, 2016 •

edited

Loading

durka commented Dec 9, 2016

alexcrichton commented Dec 11, 2016

steveklabnik commented Dec 12, 2016

aturon commented Dec 13, 2016

alexcrichton commented Dec 16, 2016

aturon commented Dec 21, 2016

bluss commented Dec 22, 2016

alexcrichton commented Dec 28, 2016

aturon commented Jan 4, 2017

nikomatsakis commented Jan 4, 2017

steveklabnik commented Jan 23, 2017

nikomatsakis commented Jan 24, 2017

alexcrichton commented Feb 4, 2017

bors commented Feb 4, 2017

bors commented Feb 5, 2017

bors commented Feb 5, 2017

alexcrichton commented Feb 5, 2017 via email

bors commented Feb 5, 2017

bors commented Feb 5, 2017

rustdoc: fix doctests with non-feature crate attrs #38161

rustdoc: fix doctests with non-feature crate attrs #38161

Conversation

durka commented Dec 4, 2016

rust-highfive commented Dec 4, 2016

bluss commented Dec 4, 2016

durka commented Dec 4, 2016 • edited Loading

durka commented Dec 4, 2016

durka commented Dec 5, 2016

durka commented Dec 5, 2016

GuillaumeGomez commented Dec 7, 2016

alexcrichton commented Dec 7, 2016

durka commented Dec 7, 2016 • edited Loading

durka commented Dec 9, 2016

alexcrichton commented Dec 11, 2016

steveklabnik commented Dec 12, 2016

aturon commented Dec 13, 2016

alexcrichton commented Dec 16, 2016

aturon commented Dec 21, 2016

bluss commented Dec 22, 2016

alexcrichton commented Dec 28, 2016

aturon commented Jan 4, 2017

nikomatsakis commented Jan 4, 2017

steveklabnik commented Jan 23, 2017

nikomatsakis commented Jan 24, 2017

alexcrichton commented Feb 4, 2017

bors commented Feb 4, 2017

bors commented Feb 5, 2017

bors commented Feb 5, 2017

alexcrichton commented Feb 5, 2017 via email

bors commented Feb 5, 2017

bors commented Feb 5, 2017

durka commented Dec 4, 2016 •

edited

Loading

durka commented Dec 7, 2016 •

edited

Loading