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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 15 additions & 3 deletions compiler/rustc_lint_defs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -446,18 +446,26 @@ macro_rules! declare_lint {
macro_rules! declare_tool_lint {
(
$(#[$attr:meta])* $vis:vis $tool:ident ::$NAME:ident, $Level: ident, $desc: expr
$(, @future_incompatible = FutureIncompatibleInfo { $($field:ident : $val:expr),* $(,)* }; )?
) => (
$crate::declare_tool_lint!{$(#[$attr])* $vis $tool::$NAME, $Level, $desc, false}
$crate::declare_tool_lint!{
$(#[$attr])* $vis $tool::$NAME, $Level, $desc, false
$(, @future_incompatible = FutureIncompatibleInfo { $($field : $val),* };)?
}
);
(
$(#[$attr:meta])* $vis:vis $tool:ident ::$NAME:ident, $Level:ident, $desc:expr,
report_in_external_macro: $rep:expr
$(, @future_incompatible = FutureIncompatibleInfo { $($field:ident : $val:expr),* $(,)* }; )?
) => (
$crate::declare_tool_lint!{$(#[$attr])* $vis $tool::$NAME, $Level, $desc, $rep}
$crate::declare_tool_lint!{$(#[$attr])* $vis $tool::$NAME, $Level, $desc, $rep
$(, @future_incompatible = FutureIncompatibleInfo { $($field:ident : $val:expr),* $(,)* }; )?
}
);
(
$(#[$attr:meta])* $vis:vis $tool:ident ::$NAME:ident, $Level:ident, $desc:expr,
$external:expr
$(, @future_incompatible = FutureIncompatibleInfo { $($field:ident : $val:expr),* $(,)* }; )?
) => (
$(#[$attr])*
$vis static $NAME: &$crate::Lint = &$crate::Lint {
Expand All @@ -466,10 +474,14 @@ macro_rules! declare_tool_lint {
desc: $desc,
edition_lint_opts: None,
report_in_external_macro: $external,
future_incompatible: None,
$(future_incompatible: Some($crate::FutureIncompatibleInfo {
$($field: $val,)*
..$crate::FutureIncompatibleInfo::default_fields_for_macro()
}),)?
is_plugin: true,
feature_gate: None,
crate_level_only: false,
..$crate::Lint::default_fields_for_macro()
};
);
}
Expand Down
28 changes: 28 additions & 0 deletions src/doc/rustdoc/src/lints.md
Original file line number Diff line number Diff line change
Expand Up @@ -331,3 +331,31 @@ warning: this URL is not a hyperlink
3 | /// [http://example.net]
| ^^^^^^^^^^^^^^^^^^ help: use an automatic link instead: `<http://example.net>`
```

## external_doc

This lint is **nightly-only** and **warns by default**. It detects when
`#![feature(external_doc)]` is used. This feature is scheduled for removal and will give a hard
error in a future release.

```rust
#![feature(external_doc)]
#![doc(include = "./external-doc.rs")]
```

Which will give:

```text
warning: `#[doc(include)]` is deprecated and will be removed in a future release
--> external-doc.rs:2:1
|
2 | #![doc(include = "./external-doc.rs")]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: `#[warn(rustdoc::doc_include)]` on by default
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #44732 <https://github.com/rust-lang/rust/issues/44732>
help: use `#![feature(extended_key_value_attributes)]` instead
|
2 | #![doc = include_str!("./external-doc.rs")]
```
17 changes: 10 additions & 7 deletions src/librustdoc/clean/inline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use rustc_data_structures::fx::FxHashSet;
use rustc_hir as hir;
use rustc_hir::def::{DefKind, Res};
use rustc_hir::def_id::{DefId, CRATE_DEF_INDEX};
use rustc_hir::Mutability;
use rustc_hir::{HirId, Mutability};
use rustc_metadata::creader::LoadedMacro;
use rustc_middle::ty::{self, TyCtxt};
use rustc_mir::const_eval::is_min_const_fn;
Expand All @@ -19,6 +19,7 @@ use crate::clean::{self, Attributes, GetDefId, ToSource, TypeKind};
use crate::core::DocContext;
use crate::formats::item_type::ItemType;

use super::clean_attrs;
use super::Clean;

type Attrs<'hir> = rustc_middle::ty::Attributes<'hir>;
Expand Down Expand Up @@ -121,7 +122,8 @@ crate fn try_inline(
};

let target_attrs = load_attrs(cx, did);
let attrs = box merge_attrs(cx, Some(parent_module), target_attrs, attrs_clone);
let local_item = DocContext::as_local_hir_id(cx.tcx, did);
let attrs = box merge_attrs(cx, Some(parent_module), target_attrs, attrs_clone, local_item);

cx.inlined.insert(did);
let what_rustc_thinks = clean::Item::from_def_id_and_parts(did, Some(name), kind, cx);
Expand Down Expand Up @@ -289,22 +291,22 @@ fn merge_attrs(
parent_module: Option<DefId>,
old_attrs: Attrs<'_>,
new_attrs: Option<Attrs<'_>>,
item: Option<HirId>,
) -> clean::Attributes {
// NOTE: If we have additional attributes (from a re-export),
// always insert them first. This ensure that re-export
// doc comments show up before the original doc comments
// when we render them.
if let Some(inner) = new_attrs {
if let Some(new_id) = parent_module {
let diag = cx.sess().diagnostic();
Attributes::from_ast(diag, old_attrs, Some((inner, new_id)))
Attributes::from_ast(cx.tcx, old_attrs, Some((inner, new_id)), item)
} else {
let mut both = inner.to_vec();
both.extend_from_slice(old_attrs);
both.clean(cx)
clean_attrs(&both, item, cx)
}
} else {
old_attrs.clean(cx)
clean_attrs(old_attrs, item, cx)
}
}

Expand Down Expand Up @@ -415,7 +417,8 @@ crate fn build_impl(

debug!("build_impl: impl {:?} for {:?}", trait_.def_id(), for_.def_id());

let attrs = box merge_attrs(cx, parent_module.into(), load_attrs(cx, did), attrs);
let local_item = DocContext::as_local_hir_id(tcx, did);
let attrs = box merge_attrs(cx, parent_module.into(), load_attrs(cx, did), attrs, local_item);
debug!("merged_attrs={:?}", attrs);

ret.push(clean::Item::from_def_id_and_attrs_and_parts(
Expand Down
22 changes: 14 additions & 8 deletions src/librustdoc/clean/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_hir as hir;
use rustc_hir::def::{CtorKind, DefKind, Res};
use rustc_hir::def_id::{CrateNum, DefId, CRATE_DEF_INDEX, LOCAL_CRATE};
use rustc_hir::HirId;
use rustc_index::vec::{Idx, IndexVec};
use rustc_infer::infer::region_constraints::{Constraint, RegionConstraintData};
use rustc_middle::bug;
Expand Down Expand Up @@ -108,7 +109,9 @@ impl Clean<ExternalCrate> for CrateNum {
// rendering by delegating everything to a hash map.
let mut as_primitive = |res: Res| {
if let Res::Def(DefKind::Mod, def_id) = res {
let attrs = cx.tcx.get_attrs(def_id).clean(cx);
// We already warned about any attributes on the module when cleaning it.
// Don't warn a second time.
let attrs = clean_attrs(cx.tcx.get_attrs(def_id), None, cx);
let mut prim = None;
for attr in attrs.lists(sym::doc) {
if let Some(v) = attr.value_str() {
Expand Down Expand Up @@ -155,7 +158,7 @@ impl Clean<ExternalCrate> for CrateNum {

let mut as_keyword = |res: Res| {
if let Res::Def(DefKind::Mod, def_id) = res {
let attrs = tcx.get_attrs(def_id).clean(cx);
let attrs = clean_attrs(tcx.get_attrs(def_id), None, cx);
let mut keyword = None;
for attr in attrs.lists(sym::doc) {
if attr.has_name(sym::keyword) {
Expand Down Expand Up @@ -197,7 +200,8 @@ impl Clean<ExternalCrate> for CrateNum {
ExternalCrate {
name: tcx.crate_name(*self),
src: krate_src,
attrs: tcx.get_attrs(root).clean(cx),
// The local crate was already cleaned, and all other crates are non-local.
attrs: clean_attrs(tcx.get_attrs(root), None, cx),
primitives,
keywords,
}
Expand Down Expand Up @@ -237,10 +241,12 @@ impl Clean<Item> for doctree::Module<'_> {
}
}

impl Clean<Attributes> for [ast::Attribute] {
fn clean(&self, cx: &mut DocContext<'_>) -> Attributes {
Attributes::from_ast(cx.sess().diagnostic(), self, None)
}
fn clean_attrs(
attrs: &[ast::Attribute],
local: Option<HirId>,
cx: &mut DocContext<'_>,
) -> Attributes {
Attributes::from_ast(cx.tcx, attrs, None, local)
}

impl Clean<GenericBound> for hir::GenericBound<'_> {
Expand Down Expand Up @@ -2124,7 +2130,7 @@ fn clean_extern_crate(
// FIXME: using `from_def_id_and_kind` breaks `rustdoc/masked` for some reason
vec![Item {
name: Some(name),
attrs: box attrs.clean(cx),
attrs: box clean_attrs(attrs, Some(krate.hir_id()), cx),
span: krate.span.clean(cx),
def_id: crate_def_id,
visibility: krate.vis.clean(cx),
Expand Down
32 changes: 26 additions & 6 deletions src/librustdoc/clean/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,12 @@ use rustc_ast::{self as ast, AttrStyle};
use rustc_attr::{ConstStability, Deprecation, Stability, StabilityLevel};
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_data_structures::thin_vec::ThinVec;
use rustc_errors::Applicability;
use rustc_hir as hir;
use rustc_hir::def::{CtorKind, Res};
use rustc_hir::def_id::{CrateNum, DefId, DefIndex};
use rustc_hir::lang_items::LangItem;
use rustc_hir::HirId;
use rustc_hir::{BodyId, Mutability};
use rustc_index::vec::IndexVec;
use rustc_middle::ty::{self, TyCtxt};
Expand All @@ -32,11 +34,10 @@ use rustc_target::abi::VariantIdx;
use rustc_target::spec::abi::Abi;

use crate::clean::cfg::Cfg;
use crate::clean::external_path;
use crate::clean::inline::{self, print_inlined_const};
use crate::clean::types::Type::{QPath, ResolvedPath};
use crate::clean::utils::{is_literal_expr, print_const_expr, print_evaluated_const};
use crate::clean::Clean;
use crate::clean::{clean_attrs, external_path, inline};
use crate::core::DocContext;
use crate::formats::cache::Cache;
use crate::formats::item_type::ItemType;
Expand Down Expand Up @@ -150,11 +151,12 @@ impl Item {
kind: ItemKind,
cx: &mut DocContext<'_>,
) -> Item {
let local_item = DocContext::as_local_hir_id(cx.tcx, def_id);
Self::from_def_id_and_attrs_and_parts(
def_id,
name,
kind,
box cx.tcx.get_attrs(def_id).clean(cx),
box clean_attrs(cx.tcx.get_attrs(def_id), local_item, cx),
cx,
)
}
Expand Down Expand Up @@ -739,9 +741,10 @@ impl Attributes {
}

crate fn from_ast(
diagnostic: &::rustc_errors::Handler,
tcx: TyCtxt<'_>,
attrs: &[ast::Attribute],
additional_attrs: Option<(&[ast::Attribute], DefId)>,
item: Option<HirId>,
) -> Attributes {
let mut doc_strings: Vec<DocFragment> = vec![];
let mut sp = None;
Expand Down Expand Up @@ -800,10 +803,27 @@ impl Attributes {
// Extracted #[doc(cfg(...))]
match Cfg::parse(cfg_mi) {
Ok(new_cfg) => cfg &= new_cfg,
Err(e) => diagnostic.span_err(e.span, e.msg),
Err(e) => tcx.sess.span_err(e.span, e.msg),
}
} else if let Some((filename, contents)) = Attributes::extract_include(&mi)
{
if let Some(hir_id) = item {
tcx.struct_span_lint_hir(crate::lint::DOC_INCLUDE, hir_id, attr.span, |lint_builder| {
let mut diag = lint_builder.build("`#[doc(include)]` is deprecated and will be removed in a future release");
diag.span_suggestion_verbose(
attr.span,
"use `#![feature(extended_key_value_attributes)]` instead",
format!(
"#{}[doc = include_str!(\"{}\")]",
if attr.style == AttrStyle::Inner { "!" } else { "" },
filename,
),
Applicability::MaybeIncorrect,
);
diag.emit();
});
}

let line = doc_line;
doc_line += contents.as_str().lines().count();
let frag = DocFragment {
Expand Down Expand Up @@ -2001,7 +2021,7 @@ impl Constant {
crate fn expr(&self, tcx: TyCtxt<'_>) -> String {
match self.kind {
ConstantKind::TyConst { ref expr } => expr.clone(),
ConstantKind::Extern { def_id } => print_inlined_const(tcx, def_id),
ConstantKind::Extern { def_id } => inline::print_inlined_const(tcx, def_id),
ConstantKind::Local { body, .. } | ConstantKind::Anonymous { body } => {
print_const_expr(tcx, body)
}
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/doctest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1093,7 +1093,7 @@ impl<'a, 'hir, 'tcx> HirCollector<'a, 'hir, 'tcx> {
nested: F,
) {
let attrs = self.tcx.hir().attrs(hir_id);
let mut attrs = Attributes::from_ast(self.sess.diagnostic(), attrs, None);
let mut attrs = Attributes::from_ast(self.tcx, attrs, None, Some(hir_id));
if let Some(ref cfg) = attrs.cfg {
if !cfg.matches(&self.sess.parse_sess, Some(&self.sess.features_untracked())) {
return;
Expand Down
21 changes: 20 additions & 1 deletion src/librustdoc/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,12 @@ where
}

macro_rules! declare_rustdoc_lint {
($(#[$attr:meta])* $name: ident, $level: ident, $descr: literal $(,)?) => {
($(#[$attr:meta])* $name: ident, $level: ident, $descr: literal
$(, @future_incompatible = FutureIncompatibleInfo { $($field:ident : $val:expr),* $(,)* }; )?
$(,)?) => {
declare_tool_lint! {
$(#[$attr])* pub rustdoc::$name, $level, $descr
$(, @future_incompatible = FutureIncompatibleInfo { $($field : $val),* };)?
}
}
}
Expand Down Expand Up @@ -158,6 +161,22 @@ declare_rustdoc_lint! {
"detects URLs that could be written using only angle brackets"
}

declare_rustdoc_lint! {
/// The `doc_include` lint detects when `#[doc(include = ...)]` is used.
/// This feature is scheduled for removal and will give a hard error in a future release.
///
/// 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

DOC_INCLUDE,
Warn,
"detects using `#[doc(include = ...)]`",
@future_incompatible = FutureIncompatibleInfo {
reference: "issue #44732 <https://github.com/rust-lang/rust/issues/44732>",
edition: None,
};
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is such an overkill.
doc(include) is an unstable feature, it can be simply removed at any moment and that's all.

If you need to guide people to #[doc = include_str!(...)], then message in rustc_feature/src/removed.rs gives opportunity to do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

doc(include) is an unstable feature, it can be simply removed at any moment and that's all.

This will break half of docs.rs, because it uses nightly and a lot of people don't test their documentation on nightly before publishing. If you have a suggestion other than a new lint I'm happy to hear it but I really don't think a hard error is the way to go.

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, those users that don't test their documentation on nightly are unlikely to see this warning; while they are likely to notice that their documentation fails to build on docs.rs and fix it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I grepped for doc(include in all the crates on crates.io (with criner and find criner.db/assets | grep '\.crate$' | xargs zgrep -a 'doc(include' > doc-include.txt). I came up with 6702 hits among 2618 crates; there are a total of 354798 crates.io releases, so that's almost a full percent of all crates that exist. I am not personally comfortable breaking that many crates.

For comparison, 348 of the latest versions use doc(include) and there are 58871 latest versions overall.

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jyn514
When do you plan to remove doc(include) from the compiler?
Unless you suggest to wait for years, only ~10-20 from these crates will notice the warning before the removal, we've seen similar numbers from the previous deprecation warnings, most of crates are not actively maintained and do not use deny(warnings) and can live with warnings for years.

I still suggest simply removing doc(include) immediately after extended_key_value_attributes are stabilized, more people will notice that (maybe ~20-40 crates are somewhat maintained and will update).

(FWIW, the numbers from criner look too large, it would be good to manually verify what those 348 uses are.)

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @petrochenkov here (the important point here being that extended_key_value_attributes will be stabilized).


crate static RUSTDOC_LINTS: Lazy<Vec<&'static Lint>> = Lazy::new(|| {
vec![
BROKEN_INTRA_DOC_LINKS,
Expand Down
5 changes: 5 additions & 0 deletions src/test/rustdoc-ui/auxiliary/doc-include-deprecated.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#![crate_name = "inner"]
#![feature(external_doc)]
#[doc(include = "docs.md")]
pub struct HasDocInclude {}
pub struct NoDocs {}
1 change: 1 addition & 0 deletions src/test/rustdoc-ui/auxiliary/docs.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Here have some docs
20 changes: 20 additions & 0 deletions src/test/rustdoc-ui/doc-include-deprecated.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// aux-build:doc-include-deprecated.rs
// check-pass
#![feature(external_doc)]
#![doc(include = "auxiliary/docs.md")]
//~^ WARNING deprecated
//~| WARNING hard error in a future release

extern crate inner;

pub use inner::HasDocInclude;

#[doc(include = "auxiliary/docs.md")]
//~^ WARNING deprecated
//~| WARNING hard error in a future release
pub use inner::HasDocInclude as _;

#[doc(include = "auxiliary/docs.md")]
//~^ WARNING deprecated
//~| WARNING hard error in a future release
pub use inner::NoDocs;