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

Deprecate doc(include) #82539

Closed
wants to merge 4 commits into from
Closed

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Feb 26, 2021

Why deprecate doc(include)?

This nightly feature is a strictly less general version of
extended_key_value_attributes (#78835), which looks like this:

#[doc = include_str!("docs.md")]

In particular, that feature allows manipulating the filename and
included docs in a way that doc(include) cannot, due to eager
evaluation:

#[doc = concat!(
   "These are my docs: ",
   include_str!(concat!(env!("OUT_DIR"), "/generated_docs.md"))
 )]

For more about eager evaluation and how it impacts macro parsing,
see petrochenkov's excellent writeup: https://internals.rust-lang.org/t/macro-expansion-points-in-attributes/11455

Given that #[doc(include)] offers no features that
extended_key_value_attributes cannot, it makes no sense for the
language and rustdoc to maintain it indefinitely. This deprecates
doc(include) and adds a structured suggestion to switch to switch to
doc = include_str! instead.

Implementation

This now adds a DOC_INCLUDE lint, which is marked as future_incompatible. To make that possible, it also extends declare_tool_lint! and declare_rustdoc_lint! to allow future-incompat lints. Finally, it changes Attributes::from_ast to require an Option<HirId> instead of just whether the item is local or not.

cc #44732. I plan to remove the feature gate altogether eventually, but I want to have at least a month or two when it's deprecated so people have time to switch.

r? @GuillaumeGomez cc @petrochenkov

@jyn514 jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. F-external_doc `#![feature(external_doc)]` labels Feb 26, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 26, 2021
@rust-log-analyzer

This comment has been minimized.

@camelid
Copy link
Member

camelid commented Feb 26, 2021

Should this be T-lang as well, or not necessary?

@jyn514
Copy link
Member Author

jyn514 commented Feb 26, 2021

Should this be T-lang as well, or not necessary?

rust-lang/rfcs#1990 (comment):

Conclusion from the language team: generally quite favorable, but this should fall entirely within the purview of the doc and/or dev-tools teams, rather than lang.

@jyn514
Copy link
Member Author

jyn514 commented Feb 26, 2021

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Feb 26, 2021

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

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Feb 26, 2021
@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Feb 26, 2021
@rfcbot
Copy link

rfcbot commented Feb 26, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@rust-log-analyzer

This comment has been minimized.

@camelid

This comment has been minimized.

@daxpedda
Copy link
Contributor

daxpedda commented Mar 4, 2021

I've found that:

#![cfg_attr(
    feature = "nightly",
    feature(external_doc),
    doc(include = "../README.md")
)]

can be compiled if the crate feature nightly isn't active.

But this can't:

#![cfg_attr(
    feature = "nightly",
    feature(extended_key_value_attributes),
    doc = include_str!("../README.md")
)]
error[E0658]: arbitrary expressions in key-value attributes are unstable
 --> src/lib.rs:4:11
  |
4 |     doc = include_str!("../README.md")
  |           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: see issue #78835 <https://github.com/rust-lang/rust/issues/78835> for more information
  = help: add `#![feature(extended_key_value_attributes)]` to the crate attributes to enable

I would prefer if doc(include) doesn't get deprecated before this is "fixed".

@jyn514
Copy link
Member Author

jyn514 commented Mar 4, 2021

Personally, I would rather that be fixed by stabilizing extended_key_value_attributes; I do think it makes sense to keep doc(include) available until that's done, but I don't know if it should block deprecating it at all.

@bors

This comment has been minimized.

@camelid
Copy link
Member

camelid commented Mar 6, 2021

@rfcbot concern doctest-span

How will this interact with #81070? Specifically, will it be feasible to use the source span in the included file for doc=include_str!? (cc https://rust-lang.zulipchat.com/#narrow/stream/266220-rustdoc/topic/nominated.3A.20.2381070)

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Mar 6, 2021
@camelid
Copy link
Member

camelid commented Mar 6, 2021

Oops, didn't realize that would cancel the FCP...

 ## Why deprecate doc(include)?

This nightly feature is a strictly less general version of
`extended_key_value_attributes`, which looks like this:

 ```
 #[doc = include_str!("docs.md")]
 ```

In particular, that feature allows manipulating the filename and
included docs in a way that `doc(include)` cannot, due to eager
evaluation:

 ```
 #[doc = concat!(
    "These are my docs: ",
    include_str!(concat!(env!("OUT_DIR"), "generated_docs.md"))
  )]
 ```

For more about eager evaluation and how it impacts macro parsing,
see petrochenkov's excellent writeup: https://internals.rust-lang.org/t/macro-expansion-points-in-attributes/11455

Given that `#[doc(include)]` offers no features that
`extended_key_value_attributes` cannot, it makes no sense for the
language and rustdoc to maintain it indefinitely. This deprecates
`doc(include)` and adds a structured suggestion to switch to switch to
`doc = include_str!` instead.

 ## Implementation

Notably, this changes attribute cleaning to pass in whether an item is
local or not. This is necessary to avoid warning about `doc(include)` in
dependencies (see rust-lang#82284 for the
sort of trouble that can cause).
This allows calling `tcx.struct_span_lint_hir` in the next commit. It
also slightly shortens the code.
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-10 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
.................................................................................................... 9400/11744
.................................................................................................... 9500/11744
.................................................................................i......i........... 9600/11744
.................................................................................................... 9700/11744
...........................iiiiiii..iiiiii.i........................................................ 9800/11744
.................................................................................................... 10000/11744
.................................................................................................... 10100/11744
.................................................................................................... 10200/11744
.................................................................................................... 10300/11744
---
Suite("src/test/assembly") not skipped for "bootstrap::test::Assembly" -- not in ["src/tools/tidy"]
Check compiletest suite=assembly mode=assembly (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)

running 33 tests
iiiiiiiiiiiiiiiiiiiiiiiiiiiiiii..

 finished in 0.109 seconds
Suite("src/test/incremental") not skipped for "bootstrap::test::Incremental" -- not in ["src/tools/tidy"]
Check compiletest suite=incremental mode=incremental (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
---
Suite("src/test/debuginfo") not skipped for "bootstrap::test::Debuginfo" -- not in ["src/tools/tidy"]
Check compiletest suite=debuginfo mode=debuginfo (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)

running 116 tests
iiiiiiiiii.i.i..i...i.ii....i.i....ii..........iiii.........i.....i...i.......ii.i.ii.....iiii.....i 100/116
test result: ok. 78 passed; 0 failed; 38 ignored; 0 measured; 0 filtered out; finished in 2.38s

 finished in 2.451 seconds
Suite("src/test/ui-fulldeps") not skipped for "bootstrap::test::UiFullDeps" -- not in ["src/tools/tidy"]
---
...................................................................................................
test result: ok. 299 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.03s

   Doc-tests core
error: `#[doc(include)]` is deprecated and will be removed in a future release
   |
   |
12 | #[doc(include = "core_arch_docs.md")]
   |
   |
   = note: `-D rustdoc::doc-include` implied by `-D warnings`
   = note: for more information, see issue #44732 <https://github.com/rust-lang/rust/issues/44732>
   = note: for more information, see issue #44732 <https://github.com/rust-lang/rust/issues/44732>
help: use `#![feature(extended_key_value_attributes)]` instead
   |
12 | #[doc = include_str!("core_arch_docs.md")]

error: aborting due to previous error

error: test failed, to rerun pass '--doc'
error: test failed, to rerun pass '--doc'


command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace compiler-builtins-c" "--manifest-path" "/checkout/library/test/Cargo.toml" "-p" "core" "--" "--quiet"


failed to run: /checkout/obj/build/bootstrap/debug/bootstrap --stage 2 test --exclude src/tools/tidy
Build completed unsuccessfully in 0:19:49

///
/// This is a `rustdoc` only lint, see the documentation in the [rustdoc book].
///
/// [rustdoc book]: ../../../rustdoc/lints.html#non_autolinks
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This link is incorrect:

Suggested change
/// [rustdoc book]: ../../../rustdoc/lints.html#non_autolinks
/// [rustdoc book]: ../../../rustdoc/lints.html#doc_include

reference: "issue #44732 <https://github.com/rust-lang/rust/issues/44732>",
edition: None,
};
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a compromise would be to add a more general rustdoc::deprecated_features lint or something like that instead? Then we could use it for other features as well.

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 12, 2021
Update stdarch submodule (to before it switched to const generics)

rust-lang#83278 (comment): This unblocks rust-lang#82539.

Major changes:
- More AVX-512 intrinsics.
- More ARM & AArch64 NEON intrinsics.
- Updated unstable WASM intrinsics to latest draft standards.
- std_detect is now a separate crate instead of a submodule of std.

I double-checked and the first use of const generics looks like rust-lang/stdarch@8d50178, which isn't included in this PR.

r? `@Amanieu`
@jyn514 jyn514 removed the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Apr 12, 2021
@jyn514
Copy link
Member Author

jyn514 commented Apr 16, 2021

I don't have the energy to argue about this.

@petrochenkov it's a little frustrating to me that you keep bringing up these issues multiple weeks after the FCP is accepted and the comment period ends. You've done this before too: #77489 (comment)

@jyn514 jyn514 closed this Apr 16, 2021
@jyn514 jyn514 deleted the deprecate-doc-include branch April 16, 2021 05:18
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 18, 2021
…=petrochenkov

Stabilize extended_key_value_attributes

Closes rust-lang#44732. Closes rust-lang#78835. Closes rust-lang#82768 (by making it irrelevant).

 # Stabilization report

 ## Summary

This stabilizes using macro expansion in key-value attributes, like so:

 ```rust
 #[doc = include_str!("my_doc.md")]
 struct S;

 #[path = concat!(env!("OUT_DIR"), "/generated.rs")]
 mod m;
 ```

See Petrochenkov's excellent blog post [on internals](https://internals.rust-lang.org/t/macro-expansion-points-in-attributes/11455)
for alternatives that were considered and rejected ("why accept no more and no less?")

This has been available on nightly since 1.50 with no major issues.

## Notes

### Accepted syntax

The parser accepts arbitrary Rust expressions in this position, but any expression other than a macro invocation will ultimately lead to an error because it is not expected by the built-in expression forms (e.g., `#[doc]`).  Note that decorators and the like may be able to observe other expression forms.

### Expansion ordering

Expansion of macro expressions in "inert" attributes occurs after decorators have executed, analogously to macro expressions appearing in the function body or other parts of decorator input.

There is currently no way for decorators to accept macros in key-value position if macro expansion must be performed before the decorator executes (if the macro can simply be copied into the output for later expansion, that can work).

## Test cases

 - https://github.com/rust-lang/rust/blob/master/src/test/ui/attributes/key-value-expansion-on-mac.rs
 - https://github.com/rust-lang/rust/blob/master/src/test/rustdoc/external-doc.rs

The feature has also been dogfooded extensively in the compiler and
standard library:

- rust-lang#83329
- rust-lang#83230
- rust-lang#82641
- rust-lang#80534

## Implementation history

- Initial proposal: rust-lang#55414 (comment)
- Experiment to see how much code it would break: rust-lang#67121
- Preliminary work to restrict expansion that would conflict with this
feature: rust-lang#77271
- Initial implementation: rust-lang#78837
- Fix for an ICE: rust-lang#80563

## Unresolved Questions

~~rust-lang#83366 (comment) listed some concerns, but they have been resolved as of this final report.~~

 ## Additional Information

 There are two workarounds that have a similar effect for `#[doc]`
attributes on nightly. One is to emulate this behavior by using a limited version of this feature that was stabilized for historical reasons:

```rust
macro_rules! forward_inner_docs {
    ($e:expr => $i:item) => {
        #[doc = $e]
        $i
    };
}

forward_inner_docs!(include_str!("lib.rs") => struct S {});
```

This also works for other attributes (like `#[path = concat!(...)]`).
The other is to use `doc(include)`:

```rust
 #![feature(external_doc)]
 #[doc(include = "lib.rs")]
 struct S {}
```

The first works, but is non-trivial for people to discover, and
difficult to read and maintain. The second is a strange special-case for
a particular use of the macro. This generalizes it to work for any use
case, not just including files.

I plan to remove `doc(include)` when this is stabilized
(rust-lang#82539). The `forward_inner_docs`
workaround will still compile without warnings, but I expect it to be
used less once it's no longer necessary.
@jyn514 jyn514 mentioned this pull request May 19, 2021
jackh726 added a commit to jackh726/rust that referenced this pull request May 19, 2021
…=petrochenkov

Stabilize extended_key_value_attributes

Closes rust-lang#44732. Closes rust-lang#78835. Closes rust-lang#82768 (by making it irrelevant).

 # Stabilization report

 ## Summary

This stabilizes using macro expansion in key-value attributes, like so:

 ```rust
 #[doc = include_str!("my_doc.md")]
 struct S;

 #[path = concat!(env!("OUT_DIR"), "/generated.rs")]
 mod m;
 ```

See Petrochenkov's excellent blog post [on internals](https://internals.rust-lang.org/t/macro-expansion-points-in-attributes/11455)
for alternatives that were considered and rejected ("why accept no more and no less?")

This has been available on nightly since 1.50 with no major issues.

## Notes

### Accepted syntax

The parser accepts arbitrary Rust expressions in this position, but any expression other than a macro invocation will ultimately lead to an error because it is not expected by the built-in expression forms (e.g., `#[doc]`).  Note that decorators and the like may be able to observe other expression forms.

### Expansion ordering

Expansion of macro expressions in "inert" attributes occurs after decorators have executed, analogously to macro expressions appearing in the function body or other parts of decorator input.

There is currently no way for decorators to accept macros in key-value position if macro expansion must be performed before the decorator executes (if the macro can simply be copied into the output for later expansion, that can work).

## Test cases

 - https://github.com/rust-lang/rust/blob/master/src/test/ui/attributes/key-value-expansion-on-mac.rs
 - https://github.com/rust-lang/rust/blob/master/src/test/rustdoc/external-doc.rs

The feature has also been dogfooded extensively in the compiler and
standard library:

- rust-lang#83329
- rust-lang#83230
- rust-lang#82641
- rust-lang#80534

## Implementation history

- Initial proposal: rust-lang#55414 (comment)
- Experiment to see how much code it would break: rust-lang#67121
- Preliminary work to restrict expansion that would conflict with this
feature: rust-lang#77271
- Initial implementation: rust-lang#78837
- Fix for an ICE: rust-lang#80563

## Unresolved Questions

~~rust-lang#83366 (comment) listed some concerns, but they have been resolved as of this final report.~~

 ## Additional Information

 There are two workarounds that have a similar effect for `#[doc]`
attributes on nightly. One is to emulate this behavior by using a limited version of this feature that was stabilized for historical reasons:

```rust
macro_rules! forward_inner_docs {
    ($e:expr => $i:item) => {
        #[doc = $e]
        $i
    };
}

forward_inner_docs!(include_str!("lib.rs") => struct S {});
```

This also works for other attributes (like `#[path = concat!(...)]`).
The other is to use `doc(include)`:

```rust
 #![feature(external_doc)]
 #[doc(include = "lib.rs")]
 struct S {}
```

The first works, but is non-trivial for people to discover, and
difficult to read and maintain. The second is a strange special-case for
a particular use of the macro. This generalizes it to work for any use
case, not just including files.

I plan to remove `doc(include)` when this is stabilized
(rust-lang#82539). The `forward_inner_docs`
workaround will still compile without warnings, but I expect it to be
used less once it's no longer necessary.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 5, 2021
…eGomez

Remove `doc(include)`

This nightly feature is redundant now that `extended_key_value_attributes` is stable (rust-lang#83366). `@rust-lang/rustdoc` not sure if you think this needs FCP; there was already an FCP in rust-lang#82539, but technically it was for deprecating, not removing the feature altogether.

This should not be merged before rust-lang#83366.

cc `@petrochenkov`
isislovecruft added a commit to isislovecruft/subtle that referenced this pull request Jul 13, 2021
The latest rust nightlies and beta (at stable version 1.55) include a
change from

    #![cfg_attr(feature = "nightly", feature(external_doc))]
    #![cfg_attr(feature = "nightly", doc(include = "../README.md"))]

to removing `feature(external_doc)` and also changing the syntax of
the second line to

    #![cfg_attr(feature = "nightly", doc = include_str!("../README.md"))]

However.  `include_str!` is stable currently, but the syntax of `doc =
` is expressly disallowed.  This gives me four options:

 1. Don't build documentation (bad)
 2. Copy-pasta README.md into src/lib.rs (also bad)
 3. Support only beta/nightly but not stable (completely untennable
    for a crate with ~14 million downloads)
 4. Support only stable but not beta/nightly (also untennable)

Further, waiting for this to be "fixed" by its inclusion in stable
Rust in about a week means that our MSRV increases from 1.41 to 1.56,
with no changes to actual code (other than how to build documentation)
at all, which seems quite unfriendly to downstream dependents who are
pinning their rust versions for whatever reason.

So, sadly, it seems the most friendly fix is to copy-pasta our
README.md into the codebase.

(cf. rust-lang/rust#44732,
      rust-lang/rust#82539)
isislovecruft added a commit to isislovecruft/subtle that referenced this pull request Jul 13, 2021
The latest rust nightlies and beta (at stable version 1.55) include a
change from

    #![cfg_attr(feature = "nightly", feature(external_doc))]
    #![cfg_attr(feature = "nightly", doc(include = "../README.md"))]

to removing `feature(external_doc)` and also changing the syntax of
the second line to

    #![cfg_attr(feature = "nightly", doc = include_str!("../README.md"))]

However.  `include_str!` is stable currently, but the syntax of `doc =
` is expressly disallowed.  This gives me four options:

 1. Don't build documentation (bad)
 2. Copy-pasta README.md into src/lib.rs (also bad)
 3. Support only beta/nightly but not stable (completely untennable
    for a crate with ~14 million downloads)
 4. Support only stable but not beta/nightly (also untennable)

Further, waiting for this to be "fixed" by its inclusion in stable
Rust in about a week means that our MSRV increases from 1.41 to 1.56,
with no changes to actual code (other than how to build documentation)
at all, which seems quite unfriendly to downstream dependents who are
pinning their rust versions for whatever reason.

So, sadly, it seems the most friendly fix is to copy-pasta our
README.md into the codebase.

(cf. rust-lang/rust#44732,
      rust-lang/rust#82539)
thedodd added a commit to thedodd/ybc that referenced this pull request Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. F-external_doc `#![feature(external_doc)]` finished-final-comment-period The final comment period is finished for this PR / Issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet