rustdoc breaking change in #84445 - # in line start is not stripped

[SergioBenitez]

Rocket has:

// This (long, contrived) form string...
# assert_form_parses! { Foo,
"[k:top_key][i][k:sub_key]name=Bobert&\
[k:top_key][i][k:sub_key]age=22&\
[k:top_key][i][sub_key]=1337&\
[top_key][7]name=Builder&\
[top_key][7]age=99",

// We could also set the top-level value's key explicitly:
// [top_key][k:7]=7
# "[k:top_key][i][k:sub_key]name=Bobert&\
# [k:top_key][i][k:sub_key]age=22&\
# [top_key][k:7]=7&\
# [k:top_key][i][sub_key]=1337&\
# [top_key][7]name=Builder&\
# [top_key][7]age=99",
# =>

Previously, the #s in the second string were stripped by rustdoc, and so the string passed to the macro did not contain the leading #s. Now, they are not stripped, and so a string with #s gets passed to the macro, causing the test to fail.

cc #84445, @GuillaumeGomez, @jyn514

@rustbot modify labels: +regression-from-stable-to-nightly

[jyn514]

This is a problem with rustdoc's imprecise parsing (#84478). Before, rustdoc hid your code, but rejected the following valid code:

# [inline]
fn main() {}

Now it accepts # [inline], but at the cost of breaking your code. I guess you could argue since both are wrong, it's better to stick with the one that's been around longer? It seems a shame to give up being able to hide #xxx.

[jyn514]

Well, I guess fixing this doesn't actually require reverting all of #84445, it just requires special casing # again and only stripping whitespace after #[ . Happy to take a PR for that but I will likely not have time in the next few days.

[SergioBenitez]

This is a breaking change, regardless of why, incompatible with rustdoc and cargo's semantic versioning and edition policy. The previous behavior must be restored. Changes must be additive.

Now it accepts # [inline], but at the cost of breaking your code.

And it's not just my code. See dtolnay/seq-macro#10. I imagine when this eventually hits stable, you'll find a lot more code broken, too.

I guess you could argue since both are wrong, it's better to stick with the one that's been around longer? It seems a shame to give up being able to hide #xxx.

I'm not sure what "both" refers to here. I expect the code I had to work - a line that starts with # that isn't otherwise a valid attribute to be stripped. If you'd like to change this behavior, I suggest that you keep the current behavior but 1) attempt to parse a line that starts with # as one or more attributes, 2) if it parses but would be stripped by the current implementation, issue a warning, 3) in the far far future, don't strip that line.

Well, I guess fixing this doesn't actually require reverting all of #84445, it just requires special casing # again and only stripping whitespace after #[ . Happy to take a PR for that but I will likely not have time in the next few days.

Please don't add more special cases.

[davidhewitt]

Can confirm that this has also broken rustdoc tests in PyO3 on our nightly CI job: https://github.com/PyO3/pyo3/pull/1128/checks?check_run_id=2425867141

[davidhewitt]

Well, I guess fixing this doesn't actually require reverting all of #84445, it just requires special casing # again and only stripping whitespace after #[ . Happy to take a PR for that but I will likely not have time in the next few days.

I just took a look whether I could implement such a PR, but then got uneasy because that code looks like it's going to grow a long list of special cases.

As we already have three examples of #84445 breaking code in the wild, I would suggest it's safer to just revert.

If we try to add new special cases to fix those reported here, I'm worried when #84445 finally hits stable we'll get a bunch of regressions reported in cases which people never tested on nightly.

We know that the proper fix is #84478, and it's unfortunate reverting means #83284 will remain broken for longer. This just seems like the safer option to me.

[davidhewitt]

Btw, this is a related minimal repro I was able to demonstrate:

/// ```
/// # assert_eq!(
/// #     playground::foo(r"
/// # foo
/// #   bar
/// # "
/// #     ),
/// #     "\nfoo\n  bar\n"
/// # );
/// ```
pub fn foo(s: &str) -> &str {
    s
}

Passes on stable, fails on nightly 2021-04-23

[jyn514]

This was reverted in #84511.