-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Add warning if a resolution failed #49542
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
Changes from all commits
035ec5b
a6fefde
b2192ae
16a3938
a3ed2ab
05275da
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -514,6 +514,41 @@ impl Step for RustdocJS { | |
} | ||
} | ||
|
||
#[derive(Debug, Copy, Clone, Hash, PartialEq, Eq)] | ||
pub struct RustdocUi { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This struct should be replaceable with a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried to at first to avoid writing code by hand, but failed... I can't only rely on UI or rustdoc test suite, I need a mix of the two. |
||
pub host: Interned<String>, | ||
pub target: Interned<String>, | ||
pub compiler: Compiler, | ||
} | ||
|
||
impl Step for RustdocUi { | ||
type Output = (); | ||
const DEFAULT: bool = true; | ||
const ONLY_HOSTS: bool = true; | ||
|
||
fn should_run(run: ShouldRun) -> ShouldRun { | ||
run.path("src/test/rustdoc-ui") | ||
} | ||
|
||
fn make_run(run: RunConfig) { | ||
let compiler = run.builder.compiler(run.builder.top_stage, run.host); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You need to make this depend on rustdoc. Otherwise rustdoc might not be available when running the tests. A call to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Apparently it's done correctly (or at least that's what I understood from @Mark-Simulacrum's comment). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is fine as is, we depend on rustdoc below when we pass it's path. |
||
run.builder.ensure(RustdocUi { | ||
host: run.host, | ||
target: run.target, | ||
compiler, | ||
}); | ||
} | ||
|
||
fn run(self, builder: &Builder) { | ||
builder.ensure(Compiletest { | ||
compiler: self.compiler, | ||
target: self.target, | ||
mode: "ui", | ||
suite: "rustdoc-ui", | ||
}) | ||
} | ||
} | ||
|
||
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] | ||
pub struct Tidy; | ||
|
||
|
@@ -851,8 +886,12 @@ impl Step for Compiletest { | |
cmd.arg("--run-lib-path").arg(builder.sysroot_libdir(compiler, target)); | ||
cmd.arg("--rustc-path").arg(builder.rustc(compiler)); | ||
|
||
let is_rustdoc_ui = suite.ends_with("rustdoc-ui"); | ||
|
||
// Avoid depending on rustdoc when we don't need it. | ||
if mode == "rustdoc" || (mode == "run-make" && suite.ends_with("fulldeps")) { | ||
if mode == "rustdoc" || | ||
(mode == "run-make" && suite.ends_with("fulldeps")) || | ||
(mode == "ui" && is_rustdoc_ui) { | ||
cmd.arg("--rustdoc-path").arg(builder.rustdoc(compiler.host)); | ||
} | ||
|
||
|
@@ -868,12 +907,18 @@ impl Step for Compiletest { | |
cmd.arg("--nodejs").arg(nodejs); | ||
} | ||
|
||
let mut flags = vec!["-Crpath".to_string()]; | ||
if build.config.rust_optimize_tests { | ||
flags.push("-O".to_string()); | ||
} | ||
if build.config.rust_debuginfo_tests { | ||
flags.push("-g".to_string()); | ||
let mut flags = if is_rustdoc_ui { | ||
Vec::new() | ||
} else { | ||
vec!["-Crpath".to_string()] | ||
}; | ||
if !is_rustdoc_ui { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Merge the if below into this one |
||
if build.config.rust_optimize_tests { | ||
flags.push("-O".to_string()); | ||
} | ||
if build.config.rust_debuginfo_tests { | ||
flags.push("-g".to_string()); | ||
} | ||
} | ||
flags.push("-Zunstable-options".to_string()); | ||
flags.push(build.config.cmd.rustc_args().join(" ")); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1178,6 +1178,10 @@ enum PathKind { | |
Type, | ||
} | ||
|
||
fn resolution_failure(cx: &DocContext, path_str: &str) { | ||
cx.sess().warn(&format!("[{}] cannot be resolved, ignoring it...", path_str)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are there spans available that we can use to report the warnings? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It'll be part of another pr. I think this one is already big enough. |
||
} | ||
|
||
impl Clean<Attributes> for [ast::Attribute] { | ||
fn clean(&self, cx: &DocContext) -> Attributes { | ||
let mut attrs = Attributes::from_ast(cx.sess().diagnostic(), self); | ||
|
@@ -1228,6 +1232,7 @@ impl Clean<Attributes> for [ast::Attribute] { | |
if let Ok(def) = resolve(cx, path_str, true) { | ||
def | ||
} else { | ||
resolution_failure(cx, path_str); | ||
// this could just be a normal link or a broken link | ||
// we could potentially check if something is | ||
// "intra-doc-link-like" and warn in that case | ||
|
@@ -1238,6 +1243,7 @@ impl Clean<Attributes> for [ast::Attribute] { | |
if let Ok(def) = resolve(cx, path_str, false) { | ||
def | ||
} else { | ||
resolution_failure(cx, path_str); | ||
// this could just be a normal link | ||
continue; | ||
} | ||
|
@@ -1282,6 +1288,7 @@ impl Clean<Attributes> for [ast::Attribute] { | |
} else if let Ok(value_def) = resolve(cx, path_str, true) { | ||
value_def | ||
} else { | ||
resolution_failure(cx, path_str); | ||
// this could just be a normal link | ||
continue; | ||
} | ||
|
@@ -1290,6 +1297,7 @@ impl Clean<Attributes> for [ast::Attribute] { | |
if let Some(def) = macro_resolve(cx, path_str) { | ||
(def, None) | ||
} else { | ||
resolution_failure(cx, path_str); | ||
continue | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ use rustc::middle::privacy::AccessLevels; | |
use rustc::ty::{self, TyCtxt, AllArenas}; | ||
use rustc::hir::map as hir_map; | ||
use rustc::lint; | ||
use rustc::session::config::ErrorOutputType; | ||
use rustc::util::nodemap::{FxHashMap, FxHashSet}; | ||
use rustc_resolve as resolve; | ||
use rustc_metadata::creader::CrateLoader; | ||
|
@@ -28,8 +29,9 @@ use syntax::ast::NodeId; | |
use syntax::codemap; | ||
use syntax::edition::Edition; | ||
use syntax::feature_gate::UnstableFeatures; | ||
use syntax::json::JsonEmitter; | ||
use errors; | ||
use errors::emitter::ColorConfig; | ||
use errors::emitter::{Emitter, EmitterWriter}; | ||
|
||
use std::cell::{RefCell, Cell}; | ||
use std::mem; | ||
|
@@ -115,7 +117,6 @@ impl DocAccessLevels for AccessLevels<DefId> { | |
} | ||
} | ||
|
||
|
||
pub fn run_core(search_paths: SearchPaths, | ||
cfgs: Vec<String>, | ||
externs: config::Externs, | ||
|
@@ -126,7 +127,8 @@ pub fn run_core(search_paths: SearchPaths, | |
crate_name: Option<String>, | ||
force_unstable_if_unmarked: bool, | ||
edition: Edition, | ||
cg: CodegenOptions) -> (clean::Crate, RenderInfo) | ||
cg: CodegenOptions, | ||
error_format: ErrorOutputType) -> (clean::Crate, RenderInfo) | ||
{ | ||
// Parse, resolve, and typecheck the given crate. | ||
|
||
|
@@ -138,6 +140,7 @@ pub fn run_core(search_paths: SearchPaths, | |
let warning_lint = lint::builtin::WARNINGS.name_lower(); | ||
|
||
let host_triple = TargetTriple::from_triple(config::host_triple()); | ||
// plays with error output here! | ||
let sessopts = config::Options { | ||
maybe_sysroot, | ||
search_paths, | ||
|
@@ -155,14 +158,42 @@ pub fn run_core(search_paths: SearchPaths, | |
edition, | ||
..config::basic_debugging_options() | ||
}, | ||
error_format, | ||
..config::basic_options().clone() | ||
}; | ||
|
||
let codemap = Lrc::new(codemap::CodeMap::new(sessopts.file_path_mapping())); | ||
let diagnostic_handler = errors::Handler::with_tty_emitter(ColorConfig::Auto, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the purpose of these changes? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Allow rustdoc output to be checked through UI test suite. |
||
true, | ||
false, | ||
Some(codemap.clone())); | ||
let emitter: Box<dyn Emitter> = match error_format { | ||
ErrorOutputType::HumanReadable(color_config) => Box::new( | ||
EmitterWriter::stderr( | ||
color_config, | ||
Some(codemap.clone()), | ||
false, | ||
sessopts.debugging_opts.teach, | ||
).ui_testing(sessopts.debugging_opts.ui_testing) | ||
), | ||
ErrorOutputType::Json(pretty) => Box::new( | ||
JsonEmitter::stderr( | ||
None, | ||
codemap.clone(), | ||
pretty, | ||
sessopts.debugging_opts.approximate_suggestions, | ||
).ui_testing(sessopts.debugging_opts.ui_testing) | ||
), | ||
ErrorOutputType::Short(color_config) => Box::new( | ||
EmitterWriter::stderr(color_config, Some(codemap.clone()), true, false) | ||
), | ||
}; | ||
|
||
let diagnostic_handler = errors::Handler::with_emitter_and_flags( | ||
emitter, | ||
errors::HandlerFlags { | ||
can_emit_warnings: true, | ||
treat_err_as_bug: false, | ||
external_macro_backtrace: false, | ||
..Default::default() | ||
}, | ||
); | ||
|
||
let mut sess = session::build_session_( | ||
sessopts, cpath, diagnostic_handler, codemap, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a lot of code for very few tests. I was hoping we could somehow express this in the regular ui tests by inserting some flags via comments in the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As would i, but (as far as i know) there's no facility in compiletest to swap out which binary is called for a specific test - only which arguments are passed. In addition, we would have to tell bootstrap to have rustdoc ready for the UI tests, even if someone is trying to run a specific non-rustdoc test.
Plus, we wanted to introduce UI tests for rustdoc at some point anyway.
>_>
Having them in a separate test directory will help us when we add more over time.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to have a struct directly to bind the test folder path.