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

Add lint for intra link resolution failure #51382

Merged
merged 8 commits into from
Jun 17, 2018
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/librustc/lint/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,12 @@ declare_lint! {
"detects duplicate macro exports"
}

declare_lint! {
pub INTRA_DOC_LINK_RESOLUTION_FAILURE,
Warn,
"warn about documentation intra links resolution failure"
Copy link
Member

Choose a reason for hiding this comment

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

I've tried to move away from calling this feature "intra links" because it doesn't mean much on its own. Instead, i've started opting for "type links". I feel like that says a bit more about the feature. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

As you prefer, I don't have a strong opinion on the naming.

Copy link
Member

@killercup killercup Jun 12, 2018

Choose a reason for hiding this comment

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

intra links […] doesn't mean much on its own

What is this "intra links" thing you are talking about? Did you mean "intra doc links" which has actual context for what the links are between/within and for which we also have an RFC? :trollface:

(And don't get me started on how these can also link to items that are, strictly speaking, not types!)

Copy link
Member

Choose a reason for hiding this comment

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

@QuietMisdreavus Why "type links"? It also works for functions and macros etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Soooooo, should I rename it? If so, how? :)

Copy link
Member

Choose a reason for hiding this comment

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

  • I have never, since the RFC was accepted, referred to this feature with its full title in internal discussions. The shortened title has been used so much that the full one has fallen out of our collective memory, as evidenced in this lint title, this PR title, and my comment here.
  • Calling it "type links" implies that it can link to "things that rustc cares about", even if the name is overly restrictive. Yes, i'm aware that the links can go to more than just "types".
  • INTRA_DOC_LINK_RESOLUTION_FAILURES is exceedingly verbose, but a quick look at rustc +nightly -W help says it's in good company, so use that.

}

/// Does nothing as a lint pass, but registers some `Lint`s
/// which are used by other parts of the compiler.
#[derive(Copy, Clone)]
Expand Down Expand Up @@ -351,6 +357,7 @@ impl LintPass for HardwiredLints {
UNSTABLE_NAME_COLLISIONS,
DUPLICATE_ASSOCIATED_TYPE_BINDINGS,
DUPLICATE_MACRO_EXPORTS,
INTRA_DOC_LINK_RESOLUTION_FAILURE,
)
}
}
Expand Down
33 changes: 33 additions & 0 deletions src/librustc_lint/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1665,3 +1665,36 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for TrivialConstraints {
}
}
}

/// Does nothing as a lint pass, but registers some `Lint`s
/// which are used by other parts of the compiler.
#[derive(Copy, Clone)]
pub struct SoftLints;

impl LintPass for SoftLints {
fn get_lints(&self) -> LintArray {
lint_array!(
WHILE_TRUE,
BOX_POINTERS,
NON_SHORTHAND_FIELD_PATTERNS,
Copy link
Member

Choose a reason for hiding this comment

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

Where did this list of lints come from? I see a similar list in register_builtins farther down, but this one seems to be missing some of those.

Copy link
Member Author

Choose a reason for hiding this comment

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

From this file. :)

Copy link
Member

Choose a reason for hiding this comment

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

Aha, now i see it. Thanks for pointing that out.

UNSAFE_CODE,
MISSING_DOCS,
MISSING_COPY_IMPLEMENTATIONS,
MISSING_DEBUG_IMPLEMENTATIONS,
ANONYMOUS_PARAMETERS,
UNUSED_DOC_COMMENTS,
UNCONDITIONAL_RECURSION,
PLUGIN_AS_LIBRARY,
PRIVATE_NO_MANGLE_FNS,
PRIVATE_NO_MANGLE_STATICS,
NO_MANGLE_CONST_ITEMS,
NO_MANGLE_GENERIC_ITEMS,
MUTABLE_TRANSMUTES,
UNSTABLE_FEATURES,
UNIONS_WITH_DROP_FIELDS,
UNREACHABLE_PUB,
TYPE_ALIAS_BOUNDS,
TRIVIAL_BOUNDS,
)
}
}
3 changes: 3 additions & 0 deletions src/librustc_lint/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,9 @@ use builtin::*;
use types::*;
use unused::*;

/// Useful for other parts of the compiler.
pub use builtin::SoftLints;

/// Tell the `LintStore` about all the built-in lints (the ones
/// defined in this crate and the ones defined in
/// `rustc::lint::builtin`).
Expand Down
22 changes: 17 additions & 5 deletions src/librustdoc/clean/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ pub use self::Visibility::{Public, Inherited};

use syntax;
use rustc_target::spec::abi::Abi;
use syntax::ast::{self, AttrStyle, Ident};
use syntax::ast::{self, AttrStyle, NodeId, Ident};
use syntax::attr;
use syntax::codemap::{dummy_spanned, Spanned};
use syntax::feature_gate::UnstableFeatures;
Expand All @@ -46,9 +46,10 @@ use rustc::middle::stability;
use rustc::util::nodemap::{FxHashMap, FxHashSet};
use rustc_typeck::hir_ty_to_ty;
use rustc::infer::region_constraints::{RegionConstraintData, Constraint};
use rustc::lint as lint;

use std::collections::hash_map::Entry;
use std::fmt;

use std::default::Default;
use std::{mem, slice, vec};
use std::iter::{FromIterator, once};
Expand Down Expand Up @@ -1283,10 +1284,16 @@ fn resolution_failure(
link_range.end + code_dox_len,
);

diag = cx.sess().struct_span_warn(sp, &msg);
diag = cx.tcx.struct_span_lint_node(lint::builtin::INTRA_DOC_LINK_RESOLUTION_FAILURE,
NodeId::new(0),
sp,
&msg);
diag.span_label(sp, "cannot be resolved, ignoring");
} else {
diag = cx.sess().struct_span_warn(sp, &msg);
diag = cx.tcx.struct_span_lint_node(lint::builtin::INTRA_DOC_LINK_RESOLUTION_FAILURE,
NodeId::new(0),
sp,
&msg);

let last_new_line_offset = dox[..link_range.start].rfind('\n').map_or(0, |n| n + 1);
let line = dox[last_new_line_offset..].lines().next().unwrap_or("");
Expand All @@ -1303,8 +1310,13 @@ fn resolution_failure(
}
diag
} else {
cx.sess().struct_span_warn(sp, &msg)
cx.tcx.struct_span_lint_node(lint::builtin::INTRA_DOC_LINK_RESOLUTION_FAILURE,
NodeId::new(0),
sp,
&msg)
};
diag.help("to escape `[` and `]` characters, just add '\\' before them like \
`\\[` or `\\]`");
Copy link
Member

Choose a reason for hiding this comment

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

I think this should better use span_suggestion here.

Copy link
Member Author

Choose a reason for hiding this comment

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

In many cases, it'll really be a link resolution failure. Not sure it'd be a nice idea to suggest that to remove the warning, they should not use auto-linking. :p

diag.emit();
}

Expand Down
25 changes: 21 additions & 4 deletions src/librustdoc/core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use rustc::middle::cstore::CrateStore;
use rustc::middle::privacy::AccessLevels;
use rustc::ty::{self, TyCtxt, AllArenas};
use rustc::hir::map as hir_map;
use rustc::lint;
use rustc::lint::{self, LintPass};
use rustc::session::config::ErrorOutputType;
use rustc::util::nodemap::{FxHashMap, FxHashSet};
use rustc_resolve as resolve;
Expand Down Expand Up @@ -187,16 +187,33 @@ pub fn run_core(search_paths: SearchPaths,
_ => None
};

let warning_lint = lint::builtin::WARNINGS.name_lower();
let intra_link_resolution_failure_name = lint::builtin::INTRA_DOC_LINK_RESOLUTION_FAILURE.name;
let warnings_lint_name = lint::builtin::WARNINGS.name;
let lints = lint::builtin::HardwiredLints.get_lints()
.iter()
.chain(rustc_lint::SoftLints.get_lints())
.filter_map(|lint| {
if lint.name == warnings_lint_name ||
lint.name == intra_link_resolution_failure_name {
None
Copy link
Member

Choose a reason for hiding this comment

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

Do we really want to be excluding warnings in the lints being allowed here? It seems to be doing the opposite of the old code.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's the opposite, we're allowing everything.

Copy link
Member

Choose a reason for hiding this comment

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

Are we sure that allowing a bunch of lints individually will act the same as allowing warnings all at once? Can we be sure that all warnings that can occur during a rustdoc run will always be placed in one of the lists you're chaining here?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we set all warnings at once using this one, then the intra-doc lint stop working.

Copy link
Member

Choose a reason for hiding this comment

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

Ohhhhh, that makes sense, i forgot about that. In that case, this is fine.

} else {
Some((lint.name_lower(), lint::Allow))
}
})
.collect::<Vec<_>>();

let host_triple = TargetTriple::from_triple(config::host_triple());
// plays with error output here!
let sessopts = config::Options {
maybe_sysroot,
search_paths,
crate_types: vec![config::CrateTypeRlib],
lint_opts: if !allow_warnings { vec![(warning_lint, lint::Allow)] } else { vec![] },
lint_cap: Some(lint::Allow),
lint_opts: if !allow_warnings {
lints
} else {
vec![]
},
lint_cap: Some(lint::Forbid),
Copy link
Member

Choose a reason for hiding this comment

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

Observation: lint_cap here is what was suppressing the compilation warnings that i mentioned in #41574 (comment). Changing this probably will make the --display-warnings flag useful on doc builds.

cg,
externs,
target_triple: triple.unwrap_or(host_triple),
Expand Down
1 change: 1 addition & 0 deletions src/libstd/sys/windows/ext/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

#![stable(feature = "rust1", since = "1.0.0")]
#![doc(cfg(windows))]
#![allow(missing_docs)]

pub mod ffi;
pub mod fs;
Expand Down
14 changes: 14 additions & 0 deletions src/test/rustdoc-ui/deny-intra-link-resolution-failure.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Copyright 2015 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![deny(intra_doc_link_resolution_failure)]

/// [v2] //~ ERROR
pub fn foo() {}
13 changes: 13 additions & 0 deletions src/test/rustdoc-ui/deny-intra-link-resolution-failure.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
error: `[v2]` cannot be resolved, ignoring it...
--> $DIR/deny-intra-link-resolution-failure.rs:13:6
|
13 | /// [v2] //~ ERROR
| ^^ cannot be resolved, ignoring
|
note: lint level defined here
--> $DIR/deny-intra-link-resolution-failure.rs:11:9
|
11 | #![deny(intra_doc_link_resolution_failure)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
= help: to escape `[` and `]` characters, just add '/' before them like `/[` or `/]`
Copy link
Member

@QuietMisdreavus QuietMisdreavus Jun 13, 2018

Choose a reason for hiding this comment

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

Is this really how it comes out when the error appears? Or is it an artifact of the UI testing? It would be really worrying if we were accidentally suggesting the wrong thing.

EDIT: I mean it's showing a / when it should be showing a \, based on the help messages in the code above.

Copy link
Member

Choose a reason for hiding this comment

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

@QuietMisdreavus Just an artifact of UI test normalization.

Copy link
Member

Choose a reason for hiding this comment

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

Phew, i was getting nervous there. Imperio said he got the proper output when he ran the test locally, too, so i'll let this slide.


21 changes: 21 additions & 0 deletions src/test/rustdoc-ui/intra-links-warning.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -3,48 +3,65 @@ warning: `[Foo::baz]` cannot be resolved, ignoring it...
|
13 | //! Test with [Foo::baz], [Bar::foo], ...
| ^^^^^^^^ cannot be resolved, ignoring
|
= note: #[warn(intra_doc_link_resolution_failure)] on by default
= help: to escape `[` and `]` characters, just add '/' before them like `/[` or `/]`

warning: `[Bar::foo]` cannot be resolved, ignoring it...
--> $DIR/intra-links-warning.rs:13:35
|
13 | //! Test with [Foo::baz], [Bar::foo], ...
| ^^^^^^^^ cannot be resolved, ignoring
|
= help: to escape `[` and `]` characters, just add '/' before them like `/[` or `/]`

warning: `[Uniooon::X]` cannot be resolved, ignoring it...
--> $DIR/intra-links-warning.rs:14:13
|
14 | //! , [Uniooon::X] and [Qux::Z].
| ^^^^^^^^^^ cannot be resolved, ignoring
|
= help: to escape `[` and `]` characters, just add '/' before them like `/[` or `/]`

warning: `[Qux::Z]` cannot be resolved, ignoring it...
--> $DIR/intra-links-warning.rs:14:30
|
14 | //! , [Uniooon::X] and [Qux::Z].
| ^^^^^^ cannot be resolved, ignoring
|
= help: to escape `[` and `]` characters, just add '/' before them like `/[` or `/]`

warning: `[Uniooon::X]` cannot be resolved, ignoring it...
--> $DIR/intra-links-warning.rs:16:14
|
16 | //! , [Uniooon::X] and [Qux::Z].
| ^^^^^^^^^^ cannot be resolved, ignoring
|
= help: to escape `[` and `]` characters, just add '/' before them like `/[` or `/]`

warning: `[Qux::Z]` cannot be resolved, ignoring it...
--> $DIR/intra-links-warning.rs:16:31
|
16 | //! , [Uniooon::X] and [Qux::Z].
| ^^^^^^ cannot be resolved, ignoring
|
= help: to escape `[` and `]` characters, just add '/' before them like `/[` or `/]`

warning: `[Qux:Y]` cannot be resolved, ignoring it...
--> $DIR/intra-links-warning.rs:18:13
|
18 | /// [Qux:Y]
| ^^^^^ cannot be resolved, ignoring
|
= help: to escape `[` and `]` characters, just add '/' before them like `/[` or `/]`

warning: `[BarA]` cannot be resolved, ignoring it...
--> $DIR/intra-links-warning.rs:24:10
|
24 | /// bar [BarA] bar
| ^^^^ cannot be resolved, ignoring
|
= help: to escape `[` and `]` characters, just add '/' before them like `/[` or `/]`

warning: `[BarB]` cannot be resolved, ignoring it...
--> $DIR/intra-links-warning.rs:28:1
Expand All @@ -60,6 +77,7 @@ warning: `[BarB]` cannot be resolved, ignoring it...

bar [BarB] bar
^^^^
= help: to escape `[` and `]` characters, just add '/' before them like `/[` or `/]`

warning: `[BarC]` cannot be resolved, ignoring it...
--> $DIR/intra-links-warning.rs:35:1
Expand All @@ -77,6 +95,7 @@ warning: `[BarC]` cannot be resolved, ignoring it...

bar [BarC] bar
^^^^
= help: to escape `[` and `]` characters, just add '/' before them like `/[` or `/]`

warning: `[BarD]` cannot be resolved, ignoring it...
--> $DIR/intra-links-warning.rs:48:1
Expand All @@ -88,6 +107,7 @@ warning: `[BarD]` cannot be resolved, ignoring it...

bar [BarD] bar
^^^^
= help: to escape `[` and `]` characters, just add '/' before them like `/[` or `/]`

warning: `[BarF]` cannot be resolved, ignoring it...
--> $DIR/intra-links-warning.rs:53:9
Expand All @@ -102,4 +122,5 @@ warning: `[BarF]` cannot be resolved, ignoring it...

bar [BarF] bar
^^^^
= help: to escape `[` and `]` characters, just add '/' before them like `/[` or `/]`

4 changes: 2 additions & 2 deletions src/test/rustdoc/cap-lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
// therefore should not concern itself with the lints.
#[deny(warnings)]

// @has cap_lints/struct.foo.html //pre '#[must_use]'
// @has cap_lints/struct.Foo.html //pre '#[must_use]'
#[must_use]
pub struct foo {
pub struct Foo {
field: i32,
}