From 0e0ae47af9843cddb5f67cc060cc163871da6943 Mon Sep 17 00:00:00 2001 From: Alexis Bourget Date: Tue, 15 Dec 2020 19:22:23 +0100 Subject: [PATCH 01/22] Add a new lint to rustdoc for invalid codeblocks This will make rustdoc behave properly when -Dwarnings is given --- compiler/rustc_lint/src/lib.rs | 3 +- compiler/rustc_lint_defs/src/builtin.rs | 13 ++++ src/doc/rustdoc/src/lints.md | 35 +++++++++ src/librustdoc/core.rs | 2 + .../passes/check_code_block_syntax.rs | 71 +++++++++++++------ src/test/rustdoc-ui/invalid-syntax.stderr | 1 + 6 files changed, 102 insertions(+), 23 deletions(-) diff --git a/compiler/rustc_lint/src/lib.rs b/compiler/rustc_lint/src/lib.rs index 80ef855c3859e..d1a9d518fae87 100644 --- a/compiler/rustc_lint/src/lib.rs +++ b/compiler/rustc_lint/src/lib.rs @@ -70,7 +70,7 @@ use rustc_middle::ty::TyCtxt; use rustc_session::lint::builtin::{ BARE_TRAIT_OBJECTS, BROKEN_INTRA_DOC_LINKS, ELIDED_LIFETIMES_IN_PATHS, EXPLICIT_OUTLIVES_REQUIREMENTS, INVALID_CODEBLOCK_ATTRIBUTES, INVALID_HTML_TAGS, - MISSING_DOC_CODE_EXAMPLES, NON_AUTOLINKS, PRIVATE_DOC_TESTS, + INVALID_RUST_CODEBLOCK, MISSING_DOC_CODE_EXAMPLES, NON_AUTOLINKS, PRIVATE_DOC_TESTS, }; use rustc_span::symbol::{Ident, Symbol}; use rustc_span::Span; @@ -320,6 +320,7 @@ fn register_builtins(store: &mut LintStore, no_interleave_lints: bool) { BROKEN_INTRA_DOC_LINKS, PRIVATE_INTRA_DOC_LINKS, INVALID_CODEBLOCK_ATTRIBUTES, + INVALID_RUST_CODEBLOCK, MISSING_DOC_CODE_EXAMPLES, PRIVATE_DOC_TESTS, INVALID_HTML_TAGS diff --git a/compiler/rustc_lint_defs/src/builtin.rs b/compiler/rustc_lint_defs/src/builtin.rs index fa82dce0ae2ed..90c5348c6ea6e 100644 --- a/compiler/rustc_lint_defs/src/builtin.rs +++ b/compiler/rustc_lint_defs/src/builtin.rs @@ -1847,6 +1847,18 @@ declare_lint! { "codeblock attribute looks a lot like a known one" } +declare_lint! { + /// The `invalid_rust_codeblock` lint detects Rust code blocks in + /// documentation examples that are invalid (e.g. empty, not parsable as + /// Rust code). This is a `rustdoc` only lint, see the documentation in the + /// [rustdoc book]. + /// + /// [rustdoc book]: ../../../rustdoc/lints.html#invalid_rust_codeblock + pub INVALID_RUST_CODEBLOCK, + Warn, + "codeblock could not be parsed as valid Rust or is empty" +} + declare_lint! { /// The `missing_crate_level_docs` lint detects if documentation is /// missing at the crate root. This is a `rustdoc` only lint, see the @@ -2803,6 +2815,7 @@ declare_lint_pass! { BROKEN_INTRA_DOC_LINKS, PRIVATE_INTRA_DOC_LINKS, INVALID_CODEBLOCK_ATTRIBUTES, + INVALID_RUST_CODEBLOCK, MISSING_CRATE_LEVEL_DOCS, MISSING_DOC_CODE_EXAMPLES, INVALID_HTML_TAGS, diff --git a/src/doc/rustdoc/src/lints.md b/src/doc/rustdoc/src/lints.md index 41292b3d83841..94e0cbe223a95 100644 --- a/src/doc/rustdoc/src/lints.md +++ b/src/doc/rustdoc/src/lints.md @@ -285,6 +285,41 @@ warning: unclosed HTML tag `h1` warning: 2 warnings emitted ``` + +## invalid_rust_codeblock + +This lint **warns by default**. It detects Rust code blocks in documentation +examples that are invalid (e.g. empty, not parsable as Rust). For example: + +```rust +/// Empty code block, with and without Rust: +/// +/// ```rust +/// ``` +/// +/// Unclosed code block: +/// +/// ```rust +fn main() {} +``` + +Which will give: + +```text +warning: Rust code block is empty + --> src/lib.rs:3:5 + | +3 | /// ```rust + | _____^ +4 | | /// ``` + | |_______^ + +warning: Rust code block is empty + --> src/lib.rs:8:5 + | +8 | /// ```rust + | ^^^^^^^ +``` ## non_autolinks diff --git a/src/librustdoc/core.rs b/src/librustdoc/core.rs index 9c693c290e7f4..438ac63f0da41 100644 --- a/src/librustdoc/core.rs +++ b/src/librustdoc/core.rs @@ -322,6 +322,7 @@ crate fn run_core( let private_doc_tests = rustc_lint::builtin::PRIVATE_DOC_TESTS.name; let no_crate_level_docs = rustc_lint::builtin::MISSING_CRATE_LEVEL_DOCS.name; let invalid_codeblock_attributes_name = rustc_lint::builtin::INVALID_CODEBLOCK_ATTRIBUTES.name; + let invalid_rust_codeblock = rustc_lint::builtin::INVALID_RUST_CODEBLOCK.name; let invalid_html_tags = rustc_lint::builtin::INVALID_HTML_TAGS.name; let renamed_and_removed_lints = rustc_lint::builtin::RENAMED_AND_REMOVED_LINTS.name; let non_autolinks = rustc_lint::builtin::NON_AUTOLINKS.name; @@ -337,6 +338,7 @@ crate fn run_core( private_doc_tests.to_owned(), no_crate_level_docs.to_owned(), invalid_codeblock_attributes_name.to_owned(), + invalid_rust_codeblock.to_owned(), invalid_html_tags.to_owned(), renamed_and_removed_lints.to_owned(), unknown_lints.to_owned(), diff --git a/src/librustdoc/passes/check_code_block_syntax.rs b/src/librustdoc/passes/check_code_block_syntax.rs index 0c76dc571beee..323b140847148 100644 --- a/src/librustdoc/passes/check_code_block_syntax.rs +++ b/src/librustdoc/passes/check_code_block_syntax.rs @@ -1,6 +1,8 @@ use rustc_data_structures::sync::{Lock, Lrc}; use rustc_errors::{emitter::Emitter, Applicability, Diagnostic, Handler}; +use rustc_middle::lint::LintDiagnosticBuilder; use rustc_parse::parse_stream_from_source_str; +use rustc_session::lint; use rustc_session::parse::ParseSess; use rustc_span::source_map::{FilePathMapping, SourceMap}; use rustc_span::{FileName, InnerSpan}; @@ -47,51 +49,76 @@ impl<'a, 'tcx> SyntaxChecker<'a, 'tcx> { .unwrap_or(false); let buffer = buffer.borrow(); - if buffer.has_errors || is_empty { - let mut diag = if let Some(sp) = - super::source_span_for_markdown_range(self.cx, &dox, &code_block.range, &item.attrs) - { - let warning_message = if buffer.has_errors { + if !(buffer.has_errors || is_empty) { + // No errors in a non-empty program. + return; + } + + let local_id = match item.def_id.as_local() { + Some(id) => id, + // We don't need to check the syntax for other crates so returning + // without doing anything should not be a problem. + None => return, + }; + + let hir_id = self.cx.tcx.hir().local_def_id_to_hir_id(local_id); + let mark_with_text = code_block.syntax.is_none() && code_block.is_fenced; + + // The span and whether it is precise or not. + let (sp, precise_span) = match super::source_span_for_markdown_range( + self.cx, + &dox, + &code_block.range, + &item.attrs, + ) { + Some(sp) => (sp, true), + None => (super::span_of_attrs(&item.attrs).unwrap_or(item.source.span()), false), + }; + + // lambda that will use the lint to start a new diagnostic and add + // a suggestion to it when needed. + let diag_builder = |lint: LintDiagnosticBuilder<'_>| { + let mut diag = if precise_span { + let msg = if buffer.has_errors { "could not parse code block as Rust code" } else { "Rust code block is empty" }; - let mut diag = self.cx.sess().struct_span_warn(sp, warning_message); - - if code_block.syntax.is_none() && code_block.is_fenced { - let sp = sp.from_inner(InnerSpan::new(0, 3)); + let mut diag = lint.build(msg); + if mark_with_text { diag.span_suggestion( - sp, + sp.from_inner(InnerSpan::new(0, 3)), "mark blocks that do not contain Rust code as text", String::from("```text"), Applicability::MachineApplicable, ); } - diag } else { - // We couldn't calculate the span of the markdown block that had the error, so our - // diagnostics are going to be a bit lacking. - let mut diag = self.cx.sess().struct_span_warn( - super::span_of_attrs(&item.attrs).unwrap_or(item.source.span()), - "doc comment contains an invalid Rust code block", - ); - - if code_block.syntax.is_none() && code_block.is_fenced { + let mut diag = lint.build("doc comment contains an invalid Rust code block"); + if mark_with_text { diag.help("mark blocks that do not contain Rust code as text: ```text"); } diag }; - // FIXME(#67563): Provide more context for these errors by displaying the spans inline. for message in buffer.messages.iter() { diag.note(&message); } - diag.emit(); - } + }; + + // Finally build and emit the completed diagnostic. + // All points of divergence have been handled earlier so this can be + // done the same way whether the span is precise or not. + self.cx.tcx.struct_span_lint_hir( + lint::builtin::INVALID_RUST_CODEBLOCK, + hir_id, + sp, + diag_builder, + ); } } diff --git a/src/test/rustdoc-ui/invalid-syntax.stderr b/src/test/rustdoc-ui/invalid-syntax.stderr index 9a7a4d2101362..a55a0b1d90e24 100644 --- a/src/test/rustdoc-ui/invalid-syntax.stderr +++ b/src/test/rustdoc-ui/invalid-syntax.stderr @@ -7,6 +7,7 @@ LL | | /// \__________pkt->size___________/ \_result->size_/ \__pkt->si LL | | /// ``` | |_______^ | + = note: `#[warn(invalid_rust_codeblock)]` on by default = note: error from rustc: unknown start of token: \ = note: error from rustc: unknown start of token: \ = note: error from rustc: unknown start of token: \ From b11b705e52c534300565fa9a44fa4630a896ae5b Mon Sep 17 00:00:00 2001 From: Nixon Enraght-Moony Date: Thu, 17 Dec 2020 22:06:00 +0000 Subject: [PATCH 02/22] Add test for issue #74824 --- .../generic-associated-types/issue-74824.rs | 27 +++++++++++++++++++ .../issue-74824.stderr | 27 +++++++++++++++++++ 2 files changed, 54 insertions(+) create mode 100644 src/test/ui/generic-associated-types/issue-74824.rs create mode 100644 src/test/ui/generic-associated-types/issue-74824.stderr diff --git a/src/test/ui/generic-associated-types/issue-74824.rs b/src/test/ui/generic-associated-types/issue-74824.rs new file mode 100644 index 0000000000000..00761a97d00c7 --- /dev/null +++ b/src/test/ui/generic-associated-types/issue-74824.rs @@ -0,0 +1,27 @@ +#![feature(generic_associated_types)] +#![feature(associated_type_defaults)] +#![allow(incomplete_features)] + +use std::ops::Deref; + +trait UnsafeCopy { + type Copy: Copy = Box; + //~^ ERROR the trait bound `Box: Copy` is not satisfied + //~^^ ERROR the trait bound `T: Clone` is not satisfied + fn copy(x: &Self::Copy) -> Self::Copy { + *x + } +} + +impl UnsafeCopy for T {} + +fn main() { + let b = Box::new(42usize); + let copy = <()>::copy(&b); + + let raw_b = Box::deref(&b) as *const _; + let raw_copy = Box::deref(©) as *const _; + + // assert the addresses. + assert_eq!(raw_b, raw_copy); +} diff --git a/src/test/ui/generic-associated-types/issue-74824.stderr b/src/test/ui/generic-associated-types/issue-74824.stderr new file mode 100644 index 0000000000000..34a2c1932ebcc --- /dev/null +++ b/src/test/ui/generic-associated-types/issue-74824.stderr @@ -0,0 +1,27 @@ +error[E0277]: the trait bound `Box: Copy` is not satisfied + --> $DIR/issue-74824.rs:8:5 + | +LL | type Copy: Copy = Box; + | ^^^^^^^^^^^^^^----^^^^^^^^^^ + | | | + | | required by this bound in `UnsafeCopy::Copy` + | the trait `Copy` is not implemented for `Box` + +error[E0277]: the trait bound `T: Clone` is not satisfied + --> $DIR/issue-74824.rs:8:5 + | +LL | type Copy: Copy = Box; + | ^^^^^^^^^^^^^^----^^^^^^^^^^ + | | | + | | required by this bound in `UnsafeCopy::Copy` + | the trait `Clone` is not implemented for `T` + | + = note: required because of the requirements on the impl of `Clone` for `Box` +help: consider restricting type parameter `T` + | +LL | type Copy: Copy = Box; + | ^^^^^^^ + +error: aborting due to 2 previous errors + +For more information about this error, try `rustc --explain E0277`. From 830ceaa41908bd428e36b1a804dd93c9a257aea8 Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Sun, 6 Dec 2020 13:57:37 +0100 Subject: [PATCH 03/22] Remap instrument-coverage line numbers in doctests This uses the `SourceMap::doctest_offset_line` method to re-map line numbers from doctests. Remapping columns is not yet done. Part of issue #79417. --- .../rustc_mir/src/transform/coverage/mod.rs | 6 +- compiler/rustc_span/src/lib.rs | 2 +- src/librustdoc/doctest.rs | 75 +++++--- src/librustdoc/doctest/tests.rs | 69 +++++-- src/librustdoc/html/markdown.rs | 2 +- .../coverage-reports/Makefile | 22 ++- .../expected_show_coverage.doctest.txt | 79 ++++++++ .../expected_show_coverage.uses_crate.txt | 4 +- .../coverage-reports/normalize_paths.py | 10 + ...est.main.-------.InstrumentCoverage.0.html | 127 +++++++++++++ ...doctests.-------.InstrumentCoverage.0.html | 173 ++++++++++++++++++ .../coverage/compiletest-ignore-dir | 4 +- .../coverage/coverage_tools.mk | 4 +- .../run-make-fulldeps/coverage/doctest.rs | 66 +++++++ .../coverage/lib/doctest_crate.rs | 9 + 15 files changed, 596 insertions(+), 56 deletions(-) create mode 100644 src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.doctest.txt create mode 100644 src/test/run-make-fulldeps/coverage-reports/normalize_paths.py create mode 100644 src/test/run-make-fulldeps/coverage-spanview/expected_mir_dump.doctest/doctest.main.-------.InstrumentCoverage.0.html create mode 100644 src/test/run-make-fulldeps/coverage-spanview/expected_mir_dump.doctest_crate/doctest_crate.fn_run_in_doctests.-------.InstrumentCoverage.0.html create mode 100644 src/test/run-make-fulldeps/coverage/doctest.rs create mode 100644 src/test/run-make-fulldeps/coverage/lib/doctest_crate.rs diff --git a/compiler/rustc_mir/src/transform/coverage/mod.rs b/compiler/rustc_mir/src/transform/coverage/mod.rs index 4590d37c182e4..93133e9b7f063 100644 --- a/compiler/rustc_mir/src/transform/coverage/mod.rs +++ b/compiler/rustc_mir/src/transform/coverage/mod.rs @@ -30,6 +30,7 @@ use rustc_middle::mir::{ }; use rustc_middle::ty::TyCtxt; use rustc_span::def_id::DefId; +use rustc_span::source_map::SourceMap; use rustc_span::{CharPos, Pos, SourceFile, Span, Symbol}; /// A simple error message wrapper for `coverage::Error`s. @@ -311,7 +312,7 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> { self.mir_body, counter_kind, self.bcb_leader_bb(bcb), - Some(make_code_region(file_name, &self.source_file, span, body_span)), + Some(make_code_region(source_map, file_name, &self.source_file, span, body_span)), ); } } @@ -489,6 +490,7 @@ fn inject_intermediate_expression(mir_body: &mut mir::Body<'tcx>, expression: Co /// Convert the Span into its file name, start line and column, and end line and column fn make_code_region( + source_map: &SourceMap, file_name: Symbol, source_file: &Lrc, span: Span, @@ -508,6 +510,8 @@ fn make_code_region( } else { source_file.lookup_file_pos(span.hi()) }; + let start_line = source_map.doctest_offset_line(&source_file.name, start_line); + let end_line = source_map.doctest_offset_line(&source_file.name, end_line); CodeRegion { file_name, start_line: start_line as u32, diff --git a/compiler/rustc_span/src/lib.rs b/compiler/rustc_span/src/lib.rs index f63a73acbf4ba..fbef4d06709ec 100644 --- a/compiler/rustc_span/src/lib.rs +++ b/compiler/rustc_span/src/lib.rs @@ -182,7 +182,7 @@ impl std::fmt::Display for FileName { use FileName::*; match *self { Real(RealFileName::Named(ref path)) => write!(fmt, "{}", path.display()), - // FIXME: might be nice to display both compoments of Devirtualized. + // FIXME: might be nice to display both components of Devirtualized. // But for now (to backport fix for issue #70924), best to not // perturb diagnostics so its obvious test suite still works. Real(RealFileName::Devirtualized { ref local_path, virtual_name: _ }) => { diff --git a/src/librustdoc/doctest.rs b/src/librustdoc/doctest.rs index 37fe13c32ce74..a08ded926402f 100644 --- a/src/librustdoc/doctest.rs +++ b/src/librustdoc/doctest.rs @@ -247,9 +247,10 @@ fn run_test( edition: Edition, outdir: DirState, path: PathBuf, + test_id: &str, ) -> Result<(), TestFailure> { let (test, line_offset, supports_color) = - make_test(test, Some(cratename), as_test_harness, opts, edition); + make_test(test, Some(cratename), as_test_harness, opts, edition, Some(test_id)); let output_file = outdir.path().join("rust_out"); @@ -387,6 +388,7 @@ crate fn make_test( dont_insert_main: bool, opts: &TestOptions, edition: Edition, + test_id: Option<&str>, ) -> (String, usize, bool) { let (crate_attrs, everything_else, crates) = partition_source(s); let everything_else = everything_else.trim(); @@ -542,16 +544,40 @@ crate fn make_test( prog.push_str(everything_else); } else { let returns_result = everything_else.trim_end().ends_with("(())"); + // Give each doctest main function a unique name. + // This is for example needed for the tooling around `-Z instrument-coverage`. + let inner_fn_name = if let Some(test_id) = test_id { + format!("_doctest_main_{}", test_id) + } else { + "_inner".into() + }; let (main_pre, main_post) = if returns_result { ( - "fn main() { fn _inner() -> Result<(), impl core::fmt::Debug> {", - "}\n_inner().unwrap() }", + format!( + "fn main() {{ fn {}() -> Result<(), impl core::fmt::Debug> {{\n", + inner_fn_name + ), + format!("\n}}; {}().unwrap() }}", inner_fn_name), + ) + } else if test_id.is_some() { + ( + format!("fn main() {{ fn {}() {{\n", inner_fn_name), + format!("\n}}; {}() }}", inner_fn_name), ) } else { - ("fn main() {\n", "\n}") + ("fn main() {\n".into(), "\n}".into()) }; - prog.extend([main_pre, everything_else, main_post].iter().cloned()); + // Note on newlines: We insert a line/newline *before*, and *after* + // the doctest and adjust the `line_offset` accordingly. + // In the case of `-Z instrument-coverage`, this means that the generated + // inner `main` function spans from the doctest opening codeblock to the + // closing one. For example + // /// ``` <- start of the inner main + // /// <- code under doctest + // /// ``` <- end of the inner main line_offset += 1; + + prog.extend([&main_pre, everything_else, &main_post].iter().cloned()); } debug!("final doctest:\n{}", prog); @@ -749,28 +775,24 @@ impl Tester for Collector { _ => PathBuf::from(r"doctest.rs"), }; + // For example `module/file.rs` would become `module_file_rs` + let file = filename + .to_string() + .chars() + .map(|c| if c.is_ascii_alphanumeric() { c } else { '_' }) + .collect::(); + let test_id = format!( + "{file}_{line}_{number}", + file = file, + line = line, + number = { + // Increases the current test number, if this file already + // exists or it creates a new entry with a test number of 0. + self.visited_tests.entry((file.clone(), line)).and_modify(|v| *v += 1).or_insert(0) + }, + ); let outdir = if let Some(mut path) = options.persist_doctests.clone() { - // For example `module/file.rs` would become `module_file_rs` - let folder_name = filename - .to_string() - .chars() - .map(|c| if c == '\\' || c == '/' || c == '.' { '_' } else { c }) - .collect::(); - - path.push(format!( - "{krate}_{file}_{line}_{number}", - krate = cratename, - file = folder_name, - line = line, - number = { - // Increases the current test number, if this file already - // exists or it creates a new entry with a test number of 0. - self.visited_tests - .entry((folder_name.clone(), line)) - .and_modify(|v| *v += 1) - .or_insert(0) - }, - )); + path.push(&test_id); std::fs::create_dir_all(&path) .expect("Couldn't create directory for doctest executables"); @@ -817,6 +839,7 @@ impl Tester for Collector { edition, outdir, path, + &test_id, ); if let Err(err) = res { diff --git a/src/librustdoc/doctest/tests.rs b/src/librustdoc/doctest/tests.rs index a024e9c72a43e..7c0df673c1b9e 100644 --- a/src/librustdoc/doctest/tests.rs +++ b/src/librustdoc/doctest/tests.rs @@ -11,7 +11,7 @@ fn main() { assert_eq!(2+2, 4); }" .to_string(); - let (output, len, _) = make_test(input, None, false, &opts, DEFAULT_EDITION); + let (output, len, _) = make_test(input, None, false, &opts, DEFAULT_EDITION, None); assert_eq!((output, len), (expected, 2)); } @@ -26,7 +26,7 @@ fn main() { assert_eq!(2+2, 4); }" .to_string(); - let (output, len, _) = make_test(input, Some("asdf"), false, &opts, DEFAULT_EDITION); + let (output, len, _) = make_test(input, Some("asdf"), false, &opts, DEFAULT_EDITION, None); assert_eq!((output, len), (expected, 2)); } @@ -44,7 +44,7 @@ use asdf::qwop; assert_eq!(2+2, 4); }" .to_string(); - let (output, len, _) = make_test(input, Some("asdf"), false, &opts, DEFAULT_EDITION); + let (output, len, _) = make_test(input, Some("asdf"), false, &opts, DEFAULT_EDITION, None); assert_eq!((output, len), (expected, 3)); } @@ -61,7 +61,7 @@ use asdf::qwop; assert_eq!(2+2, 4); }" .to_string(); - let (output, len, _) = make_test(input, Some("asdf"), false, &opts, DEFAULT_EDITION); + let (output, len, _) = make_test(input, Some("asdf"), false, &opts, DEFAULT_EDITION, None); assert_eq!((output, len), (expected, 2)); } @@ -79,7 +79,7 @@ use std::*; assert_eq!(2+2, 4); }" .to_string(); - let (output, len, _) = make_test(input, Some("std"), false, &opts, DEFAULT_EDITION); + let (output, len, _) = make_test(input, Some("std"), false, &opts, DEFAULT_EDITION, None); assert_eq!((output, len), (expected, 2)); } @@ -98,7 +98,7 @@ use asdf::qwop; assert_eq!(2+2, 4); }" .to_string(); - let (output, len, _) = make_test(input, Some("asdf"), false, &opts, DEFAULT_EDITION); + let (output, len, _) = make_test(input, Some("asdf"), false, &opts, DEFAULT_EDITION, None); assert_eq!((output, len), (expected, 2)); } @@ -115,7 +115,7 @@ use asdf::qwop; assert_eq!(2+2, 4); }" .to_string(); - let (output, len, _) = make_test(input, Some("asdf"), false, &opts, DEFAULT_EDITION); + let (output, len, _) = make_test(input, Some("asdf"), false, &opts, DEFAULT_EDITION, None); assert_eq!((output, len), (expected, 2)); } @@ -134,7 +134,7 @@ use asdf::qwop; assert_eq!(2+2, 4); }" .to_string(); - let (output, len, _) = make_test(input, Some("asdf"), false, &opts, DEFAULT_EDITION); + let (output, len, _) = make_test(input, Some("asdf"), false, &opts, DEFAULT_EDITION, None); assert_eq!((output, len), (expected, 3)); // Adding more will also bump the returned line offset. @@ -147,7 +147,7 @@ use asdf::qwop; assert_eq!(2+2, 4); }" .to_string(); - let (output, len, _) = make_test(input, Some("asdf"), false, &opts, DEFAULT_EDITION); + let (output, len, _) = make_test(input, Some("asdf"), false, &opts, DEFAULT_EDITION, None); assert_eq!((output, len), (expected, 4)); } @@ -164,7 +164,7 @@ fn main() { assert_eq!(2+2, 4); }" .to_string(); - let (output, len, _) = make_test(input, None, false, &opts, DEFAULT_EDITION); + let (output, len, _) = make_test(input, None, false, &opts, DEFAULT_EDITION, None); assert_eq!((output, len), (expected, 2)); } @@ -180,7 +180,7 @@ fn main() { assert_eq!(2+2, 4); }" .to_string(); - let (output, len, _) = make_test(input, None, false, &opts, DEFAULT_EDITION); + let (output, len, _) = make_test(input, None, false, &opts, DEFAULT_EDITION, None); assert_eq!((output, len), (expected, 1)); } @@ -196,7 +196,7 @@ fn main() { assert_eq!(2+2, 4); }" .to_string(); - let (output, len, _) = make_test(input, None, false, &opts, DEFAULT_EDITION); + let (output, len, _) = make_test(input, None, false, &opts, DEFAULT_EDITION, None); assert_eq!((output, len), (expected, 2)); } @@ -210,7 +210,7 @@ assert_eq!(2+2, 4);"; //Ceci n'est pas une `fn main` assert_eq!(2+2, 4);" .to_string(); - let (output, len, _) = make_test(input, None, true, &opts, DEFAULT_EDITION); + let (output, len, _) = make_test(input, None, true, &opts, DEFAULT_EDITION, None); assert_eq!((output, len), (expected, 1)); } @@ -224,7 +224,7 @@ fn make_test_display_warnings() { assert_eq!(2+2, 4); }" .to_string(); - let (output, len, _) = make_test(input, None, false, &opts, DEFAULT_EDITION); + let (output, len, _) = make_test(input, None, false, &opts, DEFAULT_EDITION, None); assert_eq!((output, len), (expected, 1)); } @@ -242,7 +242,7 @@ assert_eq!(2+2, 4); }" .to_string(); - let (output, len, _) = make_test(input, None, false, &opts, DEFAULT_EDITION); + let (output, len, _) = make_test(input, None, false, &opts, DEFAULT_EDITION, None); assert_eq!((output, len), (expected, 2)); let input = "extern crate hella_qwop; @@ -256,7 +256,7 @@ assert_eq!(asdf::foo, 4); }" .to_string(); - let (output, len, _) = make_test(input, Some("asdf"), false, &opts, DEFAULT_EDITION); + let (output, len, _) = make_test(input, Some("asdf"), false, &opts, DEFAULT_EDITION, None); assert_eq!((output, len), (expected, 3)); } @@ -274,6 +274,41 @@ test_wrapper! { }" .to_string(); - let (output, len, _) = make_test(input, Some("my_crate"), false, &opts, DEFAULT_EDITION); + let (output, len, _) = make_test(input, Some("my_crate"), false, &opts, DEFAULT_EDITION, None); assert_eq!((output, len), (expected, 1)); } + +#[test] +fn make_test_returns_result() { + // creates an inner function and unwraps it + let opts = TestOptions::default(); + let input = "use std::io; +let mut input = String::new(); +io::stdin().read_line(&mut input)?; +Ok::<(), io:Error>(())"; + let expected = "#![allow(unused)] +fn main() { fn _inner() -> Result<(), impl core::fmt::Debug> { +use std::io; +let mut input = String::new(); +io::stdin().read_line(&mut input)?; +Ok::<(), io:Error>(()) +}; _inner().unwrap() }" + .to_string(); + let (output, len, _) = make_test(input, None, false, &opts, DEFAULT_EDITION, None); + assert_eq!((output, len), (expected, 2)); +} + +#[test] +fn make_test_named_wrapper() { + // creates an inner function with a specific name + let opts = TestOptions::default(); + let input = "assert_eq!(2+2, 4);"; + let expected = "#![allow(unused)] +fn main() { fn _doctest_main_some_unique_name() { +assert_eq!(2+2, 4); +}; _doctest_main_some_unique_name() }" + .to_string(); + let (output, len, _) = + make_test(input, None, false, &opts, DEFAULT_EDITION, Some("some_unique_name")); + assert_eq!((output, len), (expected, 2)); +} diff --git a/src/librustdoc/html/markdown.rs b/src/librustdoc/html/markdown.rs index 22096203d4ce6..f911a2ce3fc78 100644 --- a/src/librustdoc/html/markdown.rs +++ b/src/librustdoc/html/markdown.rs @@ -248,7 +248,7 @@ impl<'a, I: Iterator>> Iterator for CodeBlocks<'_, 'a, I> { .join("\n"); let krate = krate.as_ref().map(|s| &**s); let (test, _, _) = - doctest::make_test(&test, krate, false, &Default::default(), edition); + doctest::make_test(&test, krate, false, &Default::default(), edition, None); let channel = if test.contains("#![feature(") { "&version=nightly" } else { "" }; let edition_string = format!("&edition={}", edition); diff --git a/src/test/run-make-fulldeps/coverage-reports/Makefile b/src/test/run-make-fulldeps/coverage-reports/Makefile index 5c24f909130c0..c4700b317efa0 100644 --- a/src/test/run-make-fulldeps/coverage-reports/Makefile +++ b/src/test/run-make-fulldeps/coverage-reports/Makefile @@ -98,7 +98,7 @@ endif # Run it in order to generate some profiling data, # with `LLVM_PROFILE_FILE=` environment variable set to # output the coverage stats for this run. - LLVM_PROFILE_FILE="$(TMPDIR)"/$@.profraw \ + LLVM_PROFILE_FILE="$(TMPDIR)"/$@-%p.profraw \ $(call RUN,$@) || \ ( \ status=$$?; \ @@ -108,9 +108,16 @@ endif ) \ ) + # Run it through rustdoc as well to cover doctests + LLVM_PROFILE_FILE="$(TMPDIR)"/$@-%p.profraw \ + $(RUSTDOC) --crate-name workaround_for_79771 --test $(SOURCEDIR)/$@.rs \ + $$( grep -q '^\/\/ require-rust-edition-2018' $(SOURCEDIR)/$@.rs && echo "--edition=2018" ) \ + -L "$(TMPDIR)" -Zinstrument-coverage \ + -Z unstable-options --persist-doctests=$(TMPDIR)/rustdoc-$@ + # Postprocess the profiling data so it can be used by the llvm-cov tool "$(LLVM_BIN_DIR)"/llvm-profdata merge --sparse \ - "$(TMPDIR)"/$@.profraw \ + "$(TMPDIR)"/$@-*.profraw \ -o "$(TMPDIR)"/$@.profdata # Generate a coverage report using `llvm-cov show`. @@ -121,8 +128,15 @@ endif --show-line-counts-or-regions \ --instr-profile="$(TMPDIR)"/$@.profdata \ $(call BIN,"$(TMPDIR)"/$@) \ - > "$(TMPDIR)"/actual_show_coverage.$@.txt \ - 2> "$(TMPDIR)"/show_coverage_stderr.$@.txt || \ + $$( \ + for file in $(TMPDIR)/rustdoc-$@/*/rust_out; \ + do \ + [[ -x $$file ]] && printf "%s %s " -object $$file; \ + done \ + ) \ + 2> "$(TMPDIR)"/show_coverage_stderr.$@.txt \ + | "$(PYTHON)" $(BASEDIR)/normalize_paths.py \ + > "$(TMPDIR)"/actual_show_coverage.$@.txt || \ ( status=$$? ; \ >&2 cat "$(TMPDIR)"/show_coverage_stderr.$@.txt ; \ exit $$status \ diff --git a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.doctest.txt b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.doctest.txt new file mode 100644 index 0000000000000..e1731c7223c5d --- /dev/null +++ b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.doctest.txt @@ -0,0 +1,79 @@ +../coverage/doctest.rs: + 1| |//! This test ensures that code from doctests is properly re-mapped. + 2| |//! See for more info. + 3| |//! + 4| |//! Just some random code: + 5| 1|//! ``` + 6| 1|//! if true { + 7| |//! // this is executed! + 8| 1|//! assert_eq!(1, 1); + 9| |//! } else { + 10| |//! // this is not! + 11| |//! assert_eq!(1, 2); + 12| |//! } + 13| 1|//! ``` + 14| |//! + 15| |//! doctest testing external code: + 16| |//! ``` + 17| 1|//! extern crate doctest_crate; + 18| 1|//! doctest_crate::fn_run_in_doctests(1); + 19| 1|//! ``` + 20| |//! + 21| |//! doctest returning a result: + 22| 1|//! ``` + 23| 1|//! #[derive(Debug)] + 24| 1|//! struct SomeError; + 25| 1|//! let mut res = Err(SomeError); + 26| 1|//! if res.is_ok() { + 27| 0|//! res?; + 28| 1|//! } else { + 29| 1|//! res = Ok(0); + 30| 1|//! } + 31| |//! // need to be explicit because rustdoc cant infer the return type + 32| 1|//! Ok::<(), SomeError>(()) + 33| 1|//! ``` + 34| |//! + 35| |//! doctest with custom main: + 36| |//! ``` + 37| |//! #[derive(Debug)] + 38| |//! struct SomeError; + 39| |//! + 40| |//! extern crate doctest_crate; + 41| |//! + 42| 1|//! fn doctest_main() -> Result<(), SomeError> { + 43| 1|//! doctest_crate::fn_run_in_doctests(2); + 44| 1|//! Ok(()) + 45| 1|//! } + 46| |//! + 47| |//! // this `main` is not shown as covered, as it clashes with all the other + 48| |//! // `main` functions that were automatically generated for doctests + 49| |//! fn main() -> Result<(), SomeError> { + 50| |//! doctest_main() + 51| |//! } + 52| |//! ``` + 53| | + 54| |/// doctest attached to fn testing external code: + 55| |/// ``` + 56| 1|/// extern crate doctest_crate; + 57| 1|/// doctest_crate::fn_run_in_doctests(3); + 58| 1|/// ``` + 59| |/// + 60| 1|fn main() { + 61| 1| if true { + 62| 1| assert_eq!(1, 1); + 63| | } else { + 64| | assert_eq!(1, 2); + 65| | } + 66| 1|} + +../coverage/lib/doctest_crate.rs: + 1| |/// A function run only from within doctests + 2| 3|pub fn fn_run_in_doctests(conditional: usize) { + 3| 3| match conditional { + 4| 1| 1 => assert_eq!(1, 1), // this is run, + 5| 1| 2 => assert_eq!(1, 1), // this, + 6| 1| 3 => assert_eq!(1, 1), // and this too + 7| 0| _ => assert_eq!(1, 2), // however this is not + 8| | } + 9| 3|} + diff --git a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_crate.txt b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_crate.txt index e14e733fff6d4..4c03e950af029 100644 --- a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_crate.txt +++ b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_crate.txt @@ -19,12 +19,12 @@ 18| 2| println!("used_only_from_bin_crate_generic_function with {:?}", arg); 19| 2|} ------------------ - | used_crate::used_only_from_bin_crate_generic_function::<&str>: + | used_crate::used_only_from_bin_crate_generic_function::<&alloc::vec::Vec>: | 17| 1|pub fn used_only_from_bin_crate_generic_function(arg: T) { | 18| 1| println!("used_only_from_bin_crate_generic_function with {:?}", arg); | 19| 1|} ------------------ - | used_crate::used_only_from_bin_crate_generic_function::<&alloc::vec::Vec>: + | used_crate::used_only_from_bin_crate_generic_function::<&str>: | 17| 1|pub fn used_only_from_bin_crate_generic_function(arg: T) { | 18| 1| println!("used_only_from_bin_crate_generic_function with {:?}", arg); | 19| 1|} diff --git a/src/test/run-make-fulldeps/coverage-reports/normalize_paths.py b/src/test/run-make-fulldeps/coverage-reports/normalize_paths.py new file mode 100644 index 0000000000000..05fb412cdb635 --- /dev/null +++ b/src/test/run-make-fulldeps/coverage-reports/normalize_paths.py @@ -0,0 +1,10 @@ +#!/usr/bin/env python + +import sys + +# Normalize file paths in output +for line in sys.stdin: + if line.startswith("..") and line.rstrip().endswith(".rs:"): + print(line.replace("\\", "/"), end='') + else: + print(line, end='') diff --git a/src/test/run-make-fulldeps/coverage-spanview/expected_mir_dump.doctest/doctest.main.-------.InstrumentCoverage.0.html b/src/test/run-make-fulldeps/coverage-spanview/expected_mir_dump.doctest/doctest.main.-------.InstrumentCoverage.0.html new file mode 100644 index 0000000000000..8d074558aae20 --- /dev/null +++ b/src/test/run-make-fulldeps/coverage-spanview/expected_mir_dump.doctest/doctest.main.-------.InstrumentCoverage.0.html @@ -0,0 +1,127 @@ + + + + +doctest.main - Coverage Spans + + + +
@0⦊fn main() ⦉@0{ + if @0⦊true⦉@0 { + @5⦊@4,6,7,8,9⦊assert_eq!(1, 1);⦉@4,6,7,8,9⦉@5 + } else { + @11⦊@10,12,13,14,15⦊assert_eq!(1, 2);⦉@10,12,13,14,15⦉@11 + } +}@16⦊‸⦉@16
+ + diff --git a/src/test/run-make-fulldeps/coverage-spanview/expected_mir_dump.doctest_crate/doctest_crate.fn_run_in_doctests.-------.InstrumentCoverage.0.html b/src/test/run-make-fulldeps/coverage-spanview/expected_mir_dump.doctest_crate/doctest_crate.fn_run_in_doctests.-------.InstrumentCoverage.0.html new file mode 100644 index 0000000000000..ae119d9ca9f2e --- /dev/null +++ b/src/test/run-make-fulldeps/coverage-spanview/expected_mir_dump.doctest_crate/doctest_crate.fn_run_in_doctests.-------.InstrumentCoverage.0.html @@ -0,0 +1,173 @@ + + + + +doctest_crate.fn_run_in_doctests - Coverage Spans + + + +
@0⦊pub fn fn_run_in_doctests(conditional: usize) ⦉@0{ + match @0⦊conditional⦉@0 { + 1 => @7⦊@6,8,9,10,11⦊assert_eq!(1, 1)⦉@6,8,9,10,11⦉@7, // this is run, + 2 => @14⦊@13,15,16,17,18⦊assert_eq!(1, 1)⦉@13,15,16,17,18⦉@14, // this, + 3 => @21⦊@20,22,23,24,25⦊assert_eq!(1, 1)⦉@20,22,23,24,25⦉@21, // and this too + _ => @27⦊@26,28,29,30,31⦊assert_eq!(1, 2)⦉@26,28,29,30,31⦉@27, // however this is not + } +}@32⦊‸⦉@32
+ + diff --git a/src/test/run-make-fulldeps/coverage/compiletest-ignore-dir b/src/test/run-make-fulldeps/coverage/compiletest-ignore-dir index abf8df8fdc9e6..d1824d189e382 100644 --- a/src/test/run-make-fulldeps/coverage/compiletest-ignore-dir +++ b/src/test/run-make-fulldeps/coverage/compiletest-ignore-dir @@ -1,3 +1,3 @@ -# Directory "instrument-coverage" supports the tests at prefix ../instrument-coverage-* +# Directory "coverage" supports the tests at prefix ../coverage-* -# Use ./x.py [options] test src/test/run-make-fulldeps/instrument-coverage to run all related tests. +# Use ./x.py [options] test src/test/run-make-fulldeps/coverage to run all related tests. diff --git a/src/test/run-make-fulldeps/coverage/coverage_tools.mk b/src/test/run-make-fulldeps/coverage/coverage_tools.mk index 7dc485cd94d66..4d340d4b1dadd 100644 --- a/src/test/run-make-fulldeps/coverage/coverage_tools.mk +++ b/src/test/run-make-fulldeps/coverage/coverage_tools.mk @@ -1,7 +1,7 @@ -# Common Makefile include for Rust `run-make-fulldeps/instrument-coverage-* tests. Include this +# Common Makefile include for Rust `run-make-fulldeps/coverage-* tests. Include this # file with the line: # -# -include ../instrument-coverage/coverage_tools.mk +# -include ../coverage/coverage_tools.mk -include ../tools.mk diff --git a/src/test/run-make-fulldeps/coverage/doctest.rs b/src/test/run-make-fulldeps/coverage/doctest.rs new file mode 100644 index 0000000000000..e41d669bf0c76 --- /dev/null +++ b/src/test/run-make-fulldeps/coverage/doctest.rs @@ -0,0 +1,66 @@ +//! This test ensures that code from doctests is properly re-mapped. +//! See for more info. +//! +//! Just some random code: +//! ``` +//! if true { +//! // this is executed! +//! assert_eq!(1, 1); +//! } else { +//! // this is not! +//! assert_eq!(1, 2); +//! } +//! ``` +//! +//! doctest testing external code: +//! ``` +//! extern crate doctest_crate; +//! doctest_crate::fn_run_in_doctests(1); +//! ``` +//! +//! doctest returning a result: +//! ``` +//! #[derive(Debug)] +//! struct SomeError; +//! let mut res = Err(SomeError); +//! if res.is_ok() { +//! res?; +//! } else { +//! res = Ok(0); +//! } +//! // need to be explicit because rustdoc cant infer the return type +//! Ok::<(), SomeError>(()) +//! ``` +//! +//! doctest with custom main: +//! ``` +//! #[derive(Debug)] +//! struct SomeError; +//! +//! extern crate doctest_crate; +//! +//! fn doctest_main() -> Result<(), SomeError> { +//! doctest_crate::fn_run_in_doctests(2); +//! Ok(()) +//! } +//! +//! // this `main` is not shown as covered, as it clashes with all the other +//! // `main` functions that were automatically generated for doctests +//! fn main() -> Result<(), SomeError> { +//! doctest_main() +//! } +//! ``` + +/// doctest attached to fn testing external code: +/// ``` +/// extern crate doctest_crate; +/// doctest_crate::fn_run_in_doctests(3); +/// ``` +/// +fn main() { + if true { + assert_eq!(1, 1); + } else { + assert_eq!(1, 2); + } +} diff --git a/src/test/run-make-fulldeps/coverage/lib/doctest_crate.rs b/src/test/run-make-fulldeps/coverage/lib/doctest_crate.rs new file mode 100644 index 0000000000000..c3210146d69b0 --- /dev/null +++ b/src/test/run-make-fulldeps/coverage/lib/doctest_crate.rs @@ -0,0 +1,9 @@ +/// A function run only from within doctests +pub fn fn_run_in_doctests(conditional: usize) { + match conditional { + 1 => assert_eq!(1, 1), // this is run, + 2 => assert_eq!(1, 1), // this, + 3 => assert_eq!(1, 1), // and this too + _ => assert_eq!(1, 2), // however this is not + } +} From 52b717f8264baf1044a001035d20c77f545b6abf Mon Sep 17 00:00:00 2001 From: pierwill Date: Sat, 19 Dec 2020 14:08:41 -0800 Subject: [PATCH 04/22] Edit rustc_middle::lint::LintSource docs Edit punctuation in doc comment for rustc_middle::lint::LintSource::CommandLine. --- compiler/rustc_middle/src/lint.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_middle/src/lint.rs b/compiler/rustc_middle/src/lint.rs index a61d37cc90eba..0724d50340785 100644 --- a/compiler/rustc_middle/src/lint.rs +++ b/compiler/rustc_middle/src/lint.rs @@ -22,8 +22,8 @@ pub enum LintSource { Node(Symbol, Span, Option /* RFC 2383 reason */), /// Lint level was set by a command-line flag. - /// The provided `Level` is the level specified on the command line - - /// the actual level may be lower due to `--cap-lints` + /// The provided `Level` is the level specified on the command line. + /// (The actual level may be lower due to `--cap-lints`.) CommandLine(Symbol, Level), } From 4fffa742d70c81f7414d02f55808d22eeeb77bb2 Mon Sep 17 00:00:00 2001 From: pierwill Date: Sat, 19 Dec 2020 14:25:24 -0800 Subject: [PATCH 05/22] docs: Edit rustc_middle::ty::query::on_disk_cache Expand abbreviations for "incremental compliation". Also added the word "to" to the description of CacheEncoder. --- compiler/rustc_middle/src/ty/query/on_disk_cache.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_middle/src/ty/query/on_disk_cache.rs b/compiler/rustc_middle/src/ty/query/on_disk_cache.rs index e006dfeb66336..98fc32f34c888 100644 --- a/compiler/rustc_middle/src/ty/query/on_disk_cache.rs +++ b/compiler/rustc_middle/src/ty/query/on_disk_cache.rs @@ -648,7 +648,7 @@ impl<'sess> OnDiskCache<'sess> { //- DECODING ------------------------------------------------------------------- -/// A decoder that can read from the incr. comp. cache. It is similar to the one +/// A decoder that can read from the incremental compilation cache. It is similar to the one /// we use for crate metadata decoding in that it can rebase spans and eventually /// will also handle things that contain `Ty` instances. crate struct CacheDecoder<'a, 'tcx> { @@ -936,7 +936,7 @@ impl<'a, 'tcx> Decodable> for &'tcx [Span] { //- ENCODING ------------------------------------------------------------------- -/// An encoder that can write the incr. comp. cache. +/// An encoder that can write to the incremental compilation cache. struct CacheEncoder<'a, 'tcx, E: OpaqueEncoder> { tcx: TyCtxt<'tcx>, encoder: &'a mut E, From 41c43300820615aa377b9e6f5711ff0ba1c74fab Mon Sep 17 00:00:00 2001 From: Alexis Bourget Date: Sun, 20 Dec 2020 00:34:49 +0100 Subject: [PATCH 06/22] Make doc-test pass with the new rustdoc --- compiler/rustc_trait_selection/src/opaque_types.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/compiler/rustc_trait_selection/src/opaque_types.rs b/compiler/rustc_trait_selection/src/opaque_types.rs index f5bc90e6f9621..9cce863ac73ea 100644 --- a/compiler/rustc_trait_selection/src/opaque_types.rs +++ b/compiler/rustc_trait_selection/src/opaque_types.rs @@ -46,6 +46,7 @@ pub struct OpaqueTypeDecl<'tcx> { /// type Foo = impl Baz; /// fn bar() -> Foo { /// // ^^^ This is the span we are looking for! + /// # } /// ``` /// /// In cases where the fn returns `(impl Trait, impl Trait)` or From c127530be76bd8aebc7b61f3b4a54f1be577f74c Mon Sep 17 00:00:00 2001 From: Camelid Date: Sat, 19 Dec 2020 20:47:57 -0800 Subject: [PATCH 07/22] Fix labels for 'Library Tracking Issue' template Each label needs to be separated by a comma (see the ICE issue template for an example of correct usage). --- .github/ISSUE_TEMPLATE/library_tracking_issue.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/ISSUE_TEMPLATE/library_tracking_issue.md b/.github/ISSUE_TEMPLATE/library_tracking_issue.md index b8544e6a4e0fd..3e42594c8280d 100644 --- a/.github/ISSUE_TEMPLATE/library_tracking_issue.md +++ b/.github/ISSUE_TEMPLATE/library_tracking_issue.md @@ -2,7 +2,7 @@ name: Library Tracking Issue about: A tracking issue for an unstable library feature. title: Tracking Issue for XXX -labels: C-tracking-issue T-libs +labels: C-tracking-issue, T-libs --- $DIR/incorrect-move-async-order-issue-79694.rs:7:13 + | +LL | let _ = move async { }; + | ^^^^^^^^^^ + | +help: try switching the order + | +LL | let _ = async move { }; + | ^^^^^^^^^^ + +error: aborting due to previous error + From 3abba5e21f362e325d52d922676ef26513a668e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Linus=20F=C3=A4rnstrand?= Date: Fri, 20 Nov 2020 21:45:51 +0100 Subject: [PATCH 12/22] Deprecate compare_and_swap on all atomic types --- library/core/src/sync/atomic.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/library/core/src/sync/atomic.rs b/library/core/src/sync/atomic.rs index a96da9aa6dc73..8de7d53f30250 100644 --- a/library/core/src/sync/atomic.rs +++ b/library/core/src/sync/atomic.rs @@ -479,6 +479,10 @@ impl AtomicBool { /// ``` #[inline] #[stable(feature = "rust1", since = "1.0.0")] + #[rustc_deprecated( + since = "1.50.0", + reason = "Use `compare_exchange` or `compare_exchange_weak` instead" + )] #[cfg(target_has_atomic = "8")] pub fn compare_and_swap(&self, current: bool, new: bool, order: Ordering) -> bool { match self.compare_exchange(current, new, order, strongest_failure_ordering(order)) { @@ -1080,6 +1084,10 @@ impl AtomicPtr { /// ``` #[inline] #[stable(feature = "rust1", since = "1.0.0")] + #[rustc_deprecated( + since = "1.50.0", + reason = "Use `compare_exchange` or `compare_exchange_weak` instead" + )] #[cfg(target_has_atomic = "ptr")] pub fn compare_and_swap(&self, current: *mut T, new: *mut T, order: Ordering) -> *mut T { match self.compare_exchange(current, new, order, strongest_failure_ordering(order)) { @@ -1619,6 +1627,10 @@ assert_eq!(some_var.load(Ordering::Relaxed), 10); ```"), #[inline] #[$stable] + #[rustc_deprecated( + since = "1.50.0", + reason = "Use `compare_exchange` or `compare_exchange_weak` instead") + ] #[$cfg_cas] pub fn compare_and_swap(&self, current: $int_type, From 4252e482569f00612e768811cbe0295562095343 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Linus=20F=C3=A4rnstrand?= Date: Fri, 20 Nov 2020 22:16:15 +0100 Subject: [PATCH 13/22] Add documentation on migrating away from compare_and_swap --- library/core/src/sync/atomic.rs | 51 +++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/library/core/src/sync/atomic.rs b/library/core/src/sync/atomic.rs index 8de7d53f30250..4fc455d06edea 100644 --- a/library/core/src/sync/atomic.rs +++ b/library/core/src/sync/atomic.rs @@ -464,6 +464,23 @@ impl AtomicBool { /// **Note:** This method is only available on platforms that support atomic /// operations on `u8`. /// + /// # Migrating to `compare_exchange` and `compare_exchange_weak` + /// + /// `compare_and_swap` is equivalent to `compare_exchange` with the following mapping for + /// memory orderings: + /// + /// Original | Success | Failure + /// -------- | ------- | ------- + /// Relaxed | Relaxed | Relaxed + /// Acquire | Acquire | Acquire + /// Release | Release | Relaxed + /// AcqRel | AcqRel | Acquire + /// SeqCst | SeqCst | SeqCst + /// + /// `compare_exchange_weak` is allowed to fail spuriously even when the comparison succeeds, + /// which allows the compiler to generate better assembly code when the compare and swap + /// is used in a loop. + /// /// # Examples /// /// ``` @@ -1070,6 +1087,23 @@ impl AtomicPtr { /// **Note:** This method is only available on platforms that support atomic /// operations on pointers. /// + /// # Migrating to `compare_exchange` and `compare_exchange_weak` + /// + /// `compare_and_swap` is equivalent to `compare_exchange` with the following mapping for + /// memory orderings: + /// + /// Original | Success | Failure + /// -------- | ------- | ------- + /// Relaxed | Relaxed | Relaxed + /// Acquire | Acquire | Acquire + /// Release | Release | Relaxed + /// AcqRel | AcqRel | Acquire + /// SeqCst | SeqCst | SeqCst + /// + /// `compare_exchange_weak` is allowed to fail spuriously even when the comparison succeeds, + /// which allows the compiler to generate better assembly code when the compare and swap + /// is used in a loop. + /// /// # Examples /// /// ``` @@ -1612,6 +1646,23 @@ happens, and using [`Release`] makes the load part [`Relaxed`]. **Note**: This method is only available on platforms that support atomic operations on [`", $s_int_type, "`](", $int_ref, "). +# Migrating to `compare_exchange` and `compare_exchange_weak` + +`compare_and_swap` is equivalent to `compare_exchange` with the following mapping for +memory orderings: + +Original | Success | Failure +-------- | ------- | ------- +Relaxed | Relaxed | Relaxed +Acquire | Acquire | Acquire +Release | Release | Relaxed +AcqRel | AcqRel | Acquire +SeqCst | SeqCst | SeqCst + +`compare_exchange_weak` is allowed to fail spuriously even when the comparison succeeds, +which allows the compiler to generate better assembly code when the compare and swap +is used in a loop. + # Examples ``` From 828d4ace4dee856b376fa44cc095d490ee799c30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Linus=20F=C3=A4rnstrand?= Date: Fri, 20 Nov 2020 22:27:50 +0100 Subject: [PATCH 14/22] Migrate standard library away from compare_and_swap --- library/core/tests/atomic.rs | 6 +++--- library/std/src/sync/mpsc/blocking.rs | 6 +++++- library/std/src/sync/mpsc/oneshot.rs | 14 +++++++++++--- library/std/src/sync/mpsc/shared.rs | 11 +++++++++-- library/std/src/sync/mpsc/stream.rs | 9 ++++++--- library/std/src/sync/once.rs | 16 +++++++++++----- library/std/src/sys/sgx/abi/mod.rs | 8 ++++---- library/std/src/sys/sgx/waitqueue/spin_mutex.rs | 2 +- library/std/src/sys/windows/mutex.rs | 6 +++--- library/std/src/sys_common/condvar/check.rs | 6 +++--- library/std/src/sys_common/thread_local_key.rs | 8 ++++---- .../std/src/sys_common/thread_parker/futex.rs | 2 +- .../ui/array-slice-vec/box-of-array-of-drop-1.rs | 7 ++++++- .../ui/array-slice-vec/box-of-array-of-drop-2.rs | 7 ++++++- src/test/ui/array-slice-vec/nested-vec-3.rs | 7 ++++++- 15 files changed, 79 insertions(+), 36 deletions(-) diff --git a/library/core/tests/atomic.rs b/library/core/tests/atomic.rs index 75528ebb54eaf..2d1e4496aeef7 100644 --- a/library/core/tests/atomic.rs +++ b/library/core/tests/atomic.rs @@ -4,11 +4,11 @@ use core::sync::atomic::*; #[test] fn bool_() { let a = AtomicBool::new(false); - assert_eq!(a.compare_and_swap(false, true, SeqCst), false); - assert_eq!(a.compare_and_swap(false, true, SeqCst), true); + assert_eq!(a.compare_exchange(false, true, SeqCst, SeqCst), Ok(false)); + assert_eq!(a.compare_exchange(false, true, SeqCst, SeqCst), Err(true)); a.store(false, SeqCst); - assert_eq!(a.compare_and_swap(false, true, SeqCst), false); + assert_eq!(a.compare_exchange(false, true, SeqCst, SeqCst), Ok(false)); } #[test] diff --git a/library/std/src/sync/mpsc/blocking.rs b/library/std/src/sync/mpsc/blocking.rs index d34de6a4fac3e..4c852b8ee812f 100644 --- a/library/std/src/sync/mpsc/blocking.rs +++ b/library/std/src/sync/mpsc/blocking.rs @@ -36,7 +36,11 @@ pub fn tokens() -> (WaitToken, SignalToken) { impl SignalToken { pub fn signal(&self) -> bool { - let wake = !self.inner.woken.compare_and_swap(false, true, Ordering::SeqCst); + let wake = self + .inner + .woken + .compare_exchange(false, true, Ordering::SeqCst, Ordering::SeqCst) + .is_ok(); if wake { self.inner.thread.unpark(); } diff --git a/library/std/src/sync/mpsc/oneshot.rs b/library/std/src/sync/mpsc/oneshot.rs index 75f5621fa127e..3dcf03f579a0f 100644 --- a/library/std/src/sync/mpsc/oneshot.rs +++ b/library/std/src/sync/mpsc/oneshot.rs @@ -129,7 +129,7 @@ impl Packet { let ptr = unsafe { signal_token.cast_to_usize() }; // race with senders to enter the blocking state - if self.state.compare_and_swap(EMPTY, ptr, Ordering::SeqCst) == EMPTY { + if self.state.compare_exchange(EMPTY, ptr, Ordering::SeqCst, Ordering::SeqCst).is_ok() { if let Some(deadline) = deadline { let timed_out = !wait_token.wait_max_until(deadline); // Try to reset the state @@ -161,7 +161,12 @@ impl Packet { // the state changes under our feet we'd rather just see that state // change. DATA => { - self.state.compare_and_swap(DATA, EMPTY, Ordering::SeqCst); + let _ = self.state.compare_exchange( + DATA, + EMPTY, + Ordering::SeqCst, + Ordering::SeqCst, + ); match (&mut *self.data.get()).take() { Some(data) => Ok(data), None => unreachable!(), @@ -264,7 +269,10 @@ impl Packet { // If we've got a blocked thread, then use an atomic to gain ownership // of it (may fail) - ptr => self.state.compare_and_swap(ptr, EMPTY, Ordering::SeqCst), + ptr => self + .state + .compare_exchange(ptr, EMPTY, Ordering::SeqCst, Ordering::SeqCst) + .unwrap_or_else(|x| x), }; // Now that we've got ownership of our state, figure out what to do diff --git a/library/std/src/sync/mpsc/shared.rs b/library/std/src/sync/mpsc/shared.rs index 898654f21f2ea..0c32e636a5633 100644 --- a/library/std/src/sync/mpsc/shared.rs +++ b/library/std/src/sync/mpsc/shared.rs @@ -385,8 +385,15 @@ impl Packet { self.port_dropped.store(true, Ordering::SeqCst); let mut steals = unsafe { *self.steals.get() }; while { - let cnt = self.cnt.compare_and_swap(steals, DISCONNECTED, Ordering::SeqCst); - cnt != DISCONNECTED && cnt != steals + match self.cnt.compare_exchange( + steals, + DISCONNECTED, + Ordering::SeqCst, + Ordering::SeqCst, + ) { + Ok(_) => false, + Err(old) => old != DISCONNECTED, + } } { // See the discussion in 'try_recv' for why we yield // control of this thread. diff --git a/library/std/src/sync/mpsc/stream.rs b/library/std/src/sync/mpsc/stream.rs index 9f7c1af895199..a652f24c58a19 100644 --- a/library/std/src/sync/mpsc/stream.rs +++ b/library/std/src/sync/mpsc/stream.rs @@ -322,12 +322,15 @@ impl Packet { // (because there is a bounded number of senders). let mut steals = unsafe { *self.queue.consumer_addition().steals.get() }; while { - let cnt = self.queue.producer_addition().cnt.compare_and_swap( + match self.queue.producer_addition().cnt.compare_exchange( steals, DISCONNECTED, Ordering::SeqCst, - ); - cnt != DISCONNECTED && cnt != steals + Ordering::SeqCst, + ) { + Ok(_) => false, + Err(old) => old != DISCONNECTED, + } } { while self.queue.pop().is_some() { steals += 1; diff --git a/library/std/src/sync/once.rs b/library/std/src/sync/once.rs index de5ddf1daf27b..9a17d121db19e 100644 --- a/library/std/src/sync/once.rs +++ b/library/std/src/sync/once.rs @@ -65,7 +65,7 @@ // must do so with Release ordering to make the result available. // - `wait` inserts `Waiter` nodes as a pointer in `state_and_queue`, and // needs to make the nodes available with Release ordering. The load in -// its `compare_and_swap` can be Relaxed because it only has to compare +// its `compare_exchange` can be Relaxed because it only has to compare // the atomic, not to read other data. // - `WaiterQueue::Drop` must see the `Waiter` nodes, so it must load // `state_and_queue` with Acquire ordering. @@ -395,12 +395,13 @@ impl Once { } POISONED | INCOMPLETE => { // Try to register this thread as the one RUNNING. - let old = self.state_and_queue.compare_and_swap( + let exchange_result = self.state_and_queue.compare_exchange( state_and_queue, RUNNING, Ordering::Acquire, + Ordering::Acquire, ); - if old != state_and_queue { + if let Err(old) = exchange_result { state_and_queue = old; continue; } @@ -452,8 +453,13 @@ fn wait(state_and_queue: &AtomicUsize, mut current_state: usize) { // Try to slide in the node at the head of the linked list, making sure // that another thread didn't just replace the head of the linked list. - let old = state_and_queue.compare_and_swap(current_state, me | RUNNING, Ordering::Release); - if old != current_state { + let exchange_result = state_and_queue.compare_exchange( + current_state, + me | RUNNING, + Ordering::Release, + Ordering::Relaxed, + ); + if let Err(old) = exchange_result { current_state = old; continue; } diff --git a/library/std/src/sys/sgx/abi/mod.rs b/library/std/src/sys/sgx/abi/mod.rs index a0eb12c3d154a..a5e453034762c 100644 --- a/library/std/src/sys/sgx/abi/mod.rs +++ b/library/std/src/sys/sgx/abi/mod.rs @@ -36,20 +36,20 @@ unsafe extern "C" fn tcs_init(secondary: bool) { } // Try to atomically swap UNINIT with BUSY. The returned state can be: - match RELOC_STATE.compare_and_swap(UNINIT, BUSY, Ordering::Acquire) { + match RELOC_STATE.compare_exchange(UNINIT, BUSY, Ordering::Acquire, Ordering::Acquire) { // This thread just obtained the lock and other threads will observe BUSY - UNINIT => { + Ok(_) => { reloc::relocate_elf_rela(); RELOC_STATE.store(DONE, Ordering::Release); } // We need to wait until the initialization is done. - BUSY => { + Err(BUSY) => { while RELOC_STATE.load(Ordering::Acquire) == BUSY { core::hint::spin_loop(); } } // Initialization is done. - DONE => {} + Err(DONE) => {} _ => unreachable!(), } } diff --git a/library/std/src/sys/sgx/waitqueue/spin_mutex.rs b/library/std/src/sys/sgx/waitqueue/spin_mutex.rs index d99ce895da594..9140041c58414 100644 --- a/library/std/src/sys/sgx/waitqueue/spin_mutex.rs +++ b/library/std/src/sys/sgx/waitqueue/spin_mutex.rs @@ -42,7 +42,7 @@ impl SpinMutex { #[inline(always)] pub fn try_lock(&self) -> Option> { - if !self.lock.compare_and_swap(false, true, Ordering::Acquire) { + if self.lock.compare_exchange(false, true, Ordering::Acquire, Ordering::Acquire).is_ok() { Some(SpinMutexGuard { mutex: self }) } else { None diff --git a/library/std/src/sys/windows/mutex.rs b/library/std/src/sys/windows/mutex.rs index fa51b006c346f..d4cc56d4cb3ef 100644 --- a/library/std/src/sys/windows/mutex.rs +++ b/library/std/src/sys/windows/mutex.rs @@ -123,9 +123,9 @@ impl Mutex { let inner = box Inner { remutex: ReentrantMutex::uninitialized(), held: Cell::new(false) }; inner.remutex.init(); let inner = Box::into_raw(inner); - match self.lock.compare_and_swap(0, inner as usize, Ordering::SeqCst) { - 0 => inner, - n => { + match self.lock.compare_exchange(0, inner as usize, Ordering::SeqCst, Ordering::SeqCst) { + Ok(_) => inner, + Err(n) => { Box::from_raw(inner).remutex.destroy(); n as *const _ } diff --git a/library/std/src/sys_common/condvar/check.rs b/library/std/src/sys_common/condvar/check.rs index fecb732b910ce..1578a2de60cef 100644 --- a/library/std/src/sys_common/condvar/check.rs +++ b/library/std/src/sys_common/condvar/check.rs @@ -23,9 +23,9 @@ impl SameMutexCheck { } pub fn verify(&self, mutex: &MovableMutex) { let addr = mutex.raw() as *const mutex_imp::Mutex as usize; - match self.addr.compare_and_swap(0, addr, Ordering::SeqCst) { - 0 => {} // Stored the address - n if n == addr => {} // Lost a race to store the same address + match self.addr.compare_exchange(0, addr, Ordering::SeqCst, Ordering::SeqCst) { + Ok(_) => {} // Stored the address + Err(n) if n == addr => {} // Lost a race to store the same address _ => panic!("attempted to use a condition variable with two mutexes"), } } diff --git a/library/std/src/sys_common/thread_local_key.rs b/library/std/src/sys_common/thread_local_key.rs index dbcb7b36265f5..32cd56416655f 100644 --- a/library/std/src/sys_common/thread_local_key.rs +++ b/library/std/src/sys_common/thread_local_key.rs @@ -168,7 +168,7 @@ impl StaticKey { return key; } - // POSIX allows the key created here to be 0, but the compare_and_swap + // POSIX allows the key created here to be 0, but the compare_exchange // below relies on using 0 as a sentinel value to check who won the // race to set the shared TLS key. As far as I know, there is no // guaranteed value that cannot be returned as a posix_key_create key, @@ -186,11 +186,11 @@ impl StaticKey { key2 }; rtassert!(key != 0); - match self.key.compare_and_swap(0, key as usize, Ordering::SeqCst) { + match self.key.compare_exchange(0, key as usize, Ordering::SeqCst, Ordering::SeqCst) { // The CAS succeeded, so we've created the actual key - 0 => key as usize, + Ok(_) => key as usize, // If someone beat us to the punch, use their key instead - n => { + Err(n) => { imp::destroy(key); n } diff --git a/library/std/src/sys_common/thread_parker/futex.rs b/library/std/src/sys_common/thread_parker/futex.rs index a5d4927dcc5ca..0132743b24404 100644 --- a/library/std/src/sys_common/thread_parker/futex.rs +++ b/library/std/src/sys_common/thread_parker/futex.rs @@ -49,7 +49,7 @@ impl Parker { // Wait for something to happen, assuming it's still set to PARKED. futex_wait(&self.state, PARKED, None); // Change NOTIFIED=>EMPTY and return in that case. - if self.state.compare_and_swap(NOTIFIED, EMPTY, Acquire) == NOTIFIED { + if self.state.compare_exchange(NOTIFIED, EMPTY, Acquire, Acquire).is_ok() { return; } else { // Spurious wake up. We loop to try again. diff --git a/src/test/ui/array-slice-vec/box-of-array-of-drop-1.rs b/src/test/ui/array-slice-vec/box-of-array-of-drop-1.rs index d485893281562..c8559d2472824 100644 --- a/src/test/ui/array-slice-vec/box-of-array-of-drop-1.rs +++ b/src/test/ui/array-slice-vec/box-of-array-of-drop-1.rs @@ -17,7 +17,12 @@ impl Drop for D { fn drop(&mut self) { println!("Dropping {}", self.0); let old = LOG.load(Ordering::SeqCst); - LOG.compare_and_swap(old, old << 4 | self.0 as usize, Ordering::SeqCst); + let _ = LOG.compare_exchange( + old, + old << 4 | self.0 as usize, + Ordering::SeqCst, + Ordering::SeqCst + ); } } diff --git a/src/test/ui/array-slice-vec/box-of-array-of-drop-2.rs b/src/test/ui/array-slice-vec/box-of-array-of-drop-2.rs index e8a5b00a55b9c..e75051caabcc3 100644 --- a/src/test/ui/array-slice-vec/box-of-array-of-drop-2.rs +++ b/src/test/ui/array-slice-vec/box-of-array-of-drop-2.rs @@ -17,7 +17,12 @@ impl Drop for D { fn drop(&mut self) { println!("Dropping {}", self.0); let old = LOG.load(Ordering::SeqCst); - LOG.compare_and_swap(old, old << 4 | self.0 as usize, Ordering::SeqCst); + let _ = LOG.compare_exchange( + old, + old << 4 | self.0 as usize, + Ordering::SeqCst, + Ordering::SeqCst + ); } } diff --git a/src/test/ui/array-slice-vec/nested-vec-3.rs b/src/test/ui/array-slice-vec/nested-vec-3.rs index 52b892dbcdfaf..96497a53d308e 100644 --- a/src/test/ui/array-slice-vec/nested-vec-3.rs +++ b/src/test/ui/array-slice-vec/nested-vec-3.rs @@ -18,7 +18,12 @@ impl Drop for D { fn drop(&mut self) { println!("Dropping {}", self.0); let old = LOG.load(Ordering::SeqCst); - LOG.compare_and_swap(old, old << 4 | self.0 as usize, Ordering::SeqCst); + let _ = LOG.compare_exchange( + old, + old << 4 | self.0 as usize, + Ordering::SeqCst, + Ordering::SeqCst, + ); } } From 7f35e2d5737a73e86c08316aca9c9cd90ddde3cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Linus=20F=C3=A4rnstrand?= Date: Sun, 22 Nov 2020 18:56:47 +0100 Subject: [PATCH 15/22] Add doc aliases to compare_exchange[_weak] --- library/core/src/sync/atomic.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/library/core/src/sync/atomic.rs b/library/core/src/sync/atomic.rs index 4fc455d06edea..668b0dc088602 100644 --- a/library/core/src/sync/atomic.rs +++ b/library/core/src/sync/atomic.rs @@ -546,6 +546,7 @@ impl AtomicBool { /// ``` #[inline] #[stable(feature = "extended_compare_and_swap", since = "1.10.0")] + #[doc(alias = "compare_and_swap")] #[cfg(target_has_atomic = "8")] pub fn compare_exchange( &self, @@ -599,6 +600,7 @@ impl AtomicBool { /// ``` #[inline] #[stable(feature = "extended_compare_and_swap", since = "1.10.0")] + #[doc(alias = "compare_and_swap")] #[cfg(target_has_atomic = "8")] pub fn compare_exchange_weak( &self, From 427996a2868ec958dc1a5bee564fb8f377241255 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Linus=20F=C3=A4rnstrand?= Date: Sun, 22 Nov 2020 20:26:36 +0100 Subject: [PATCH 16/22] Fix documentation typo --- library/std/src/sync/once.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/std/src/sync/once.rs b/library/std/src/sync/once.rs index 9a17d121db19e..6a330834489df 100644 --- a/library/std/src/sync/once.rs +++ b/library/std/src/sync/once.rs @@ -110,7 +110,7 @@ use crate::thread::{self, Thread}; /// ``` #[stable(feature = "rust1", since = "1.0.0")] pub struct Once { - // `state_and_queue` is actually an a pointer to a `Waiter` with extra state + // `state_and_queue` is actually a pointer to a `Waiter` with extra state // bits, so we add the `PhantomData` appropriately. state_and_queue: AtomicUsize, _marker: marker::PhantomData<*const Waiter>, From 3eef20ffa0887a88a15c29eb17de5e20a5c99363 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Linus=20F=C3=A4rnstrand?= Date: Sun, 22 Nov 2020 20:36:29 +0100 Subject: [PATCH 17/22] Improve documentation on `success` and `failure` arguments --- library/core/src/sync/atomic.rs | 42 +++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 18 deletions(-) diff --git a/library/core/src/sync/atomic.rs b/library/core/src/sync/atomic.rs index 668b0dc088602..36857979af8c1 100644 --- a/library/core/src/sync/atomic.rs +++ b/library/core/src/sync/atomic.rs @@ -514,9 +514,10 @@ impl AtomicBool { /// the previous value. On success this value is guaranteed to be equal to `current`. /// /// `compare_exchange` takes two [`Ordering`] arguments to describe the memory - /// ordering of this operation. The first describes the required ordering if the - /// operation succeeds while the second describes the required ordering when the - /// operation fails. Using [`Acquire`] as success ordering makes the store part + /// ordering of this operation. `success` describes the required ordering for the + /// read-modify-write operation that takes place if the comparison with `current` succeeds. + /// `failure` describes the required ordering for the load operation that takes place when + /// the comparison fails. Using [`Acquire`] as success ordering makes the store part /// of this operation [`Relaxed`], and using [`Release`] makes the successful load /// [`Relaxed`]. The failure ordering can only be [`SeqCst`], [`Acquire`] or [`Relaxed`] /// and must be equivalent to or weaker than the success ordering. @@ -572,9 +573,10 @@ impl AtomicBool { /// previous value. /// /// `compare_exchange_weak` takes two [`Ordering`] arguments to describe the memory - /// ordering of this operation. The first describes the required ordering if the - /// operation succeeds while the second describes the required ordering when the - /// operation fails. Using [`Acquire`] as success ordering makes the store part + /// ordering of this operation. `success` describes the required ordering for the + /// read-modify-write operation that takes place if the comparison with `current` succeeds. + /// `failure` describes the required ordering for the load operation that takes place when + /// the comparison fails. Using [`Acquire`] as success ordering makes the store part /// of this operation [`Relaxed`], and using [`Release`] makes the successful load /// [`Relaxed`]. The failure ordering can only be [`SeqCst`], [`Acquire`] or [`Relaxed`] /// and must be equivalent to or weaker than the success ordering. @@ -1138,9 +1140,10 @@ impl AtomicPtr { /// the previous value. On success this value is guaranteed to be equal to `current`. /// /// `compare_exchange` takes two [`Ordering`] arguments to describe the memory - /// ordering of this operation. The first describes the required ordering if the - /// operation succeeds while the second describes the required ordering when the - /// operation fails. Using [`Acquire`] as success ordering makes the store part + /// ordering of this operation. `success` describes the required ordering for the + /// read-modify-write operation that takes place if the comparison with `current` succeeds. + /// `failure` describes the required ordering for the load operation that takes place when + /// the comparison fails. Using [`Acquire`] as success ordering makes the store part /// of this operation [`Relaxed`], and using [`Release`] makes the successful load /// [`Relaxed`]. The failure ordering can only be [`SeqCst`], [`Acquire`] or [`Relaxed`] /// and must be equivalent to or weaker than the success ordering. @@ -1201,9 +1204,10 @@ impl AtomicPtr { /// previous value. /// /// `compare_exchange_weak` takes two [`Ordering`] arguments to describe the memory - /// ordering of this operation. The first describes the required ordering if the - /// operation succeeds while the second describes the required ordering when the - /// operation fails. Using [`Acquire`] as success ordering makes the store part + /// ordering of this operation. `success` describes the required ordering for the + /// read-modify-write operation that takes place if the comparison with `current` succeeds. + /// `failure` describes the required ordering for the load operation that takes place when + /// the comparison fails. Using [`Acquire`] as success ordering makes the store part /// of this operation [`Relaxed`], and using [`Release`] makes the successful load /// [`Relaxed`]. The failure ordering can only be [`SeqCst`], [`Acquire`] or [`Relaxed`] /// and must be equivalent to or weaker than the success ordering. @@ -1708,9 +1712,10 @@ containing the previous value. On success this value is guaranteed to be equal t `current`. `compare_exchange` takes two [`Ordering`] arguments to describe the memory -ordering of this operation. The first describes the required ordering if the -operation succeeds while the second describes the required ordering when the -operation fails. Using [`Acquire`] as success ordering makes the store part +ordering of this operation. `success` describes the required ordering for the +read-modify-write operation that takes place if the comparison with `current` succeeds. +`failure` describes the required ordering for the load operation that takes place when +the comparison fails. Using [`Acquire`] as success ordering makes the store part of this operation [`Relaxed`], and using [`Release`] makes the successful load [`Relaxed`]. The failure ordering can only be [`SeqCst`], [`Acquire`] or [`Relaxed`] and must be equivalent to or weaker than the success ordering. @@ -1760,9 +1765,10 @@ platforms. The return value is a result indicating whether the new value was written and containing the previous value. `compare_exchange_weak` takes two [`Ordering`] arguments to describe the memory -ordering of this operation. The first describes the required ordering if the -operation succeeds while the second describes the required ordering when the -operation fails. Using [`Acquire`] as success ordering makes the store part +ordering of this operation. `success` describes the required ordering for the +read-modify-write operation that takes place if the comparison with `current` succeeds. +`failure` describes the required ordering for the load operation that takes place when +the comparison fails. Using [`Acquire`] as success ordering makes the store part of this operation [`Relaxed`], and using [`Release`] makes the successful load [`Relaxed`]. The failure ordering can only be [`SeqCst`], [`Acquire`] or [`Relaxed`] and must be equivalent to or weaker than the success ordering. From 865e4797df83a702982186bbbd6bc70268cd2664 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Linus=20F=C3=A4rnstrand?= Date: Tue, 22 Dec 2020 12:24:17 +0100 Subject: [PATCH 18/22] Fix compare_and_swap in Windows thread_parker --- library/std/src/sys/windows/thread_parker.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/std/src/sys/windows/thread_parker.rs b/library/std/src/sys/windows/thread_parker.rs index 701c6e2e9bec7..e3585d4cb37a4 100644 --- a/library/std/src/sys/windows/thread_parker.rs +++ b/library/std/src/sys/windows/thread_parker.rs @@ -113,7 +113,7 @@ impl Parker { // Wait for something to happen, assuming it's still set to PARKED. c::WaitOnAddress(self.ptr(), &PARKED as *const _ as c::LPVOID, 1, c::INFINITE); // Change NOTIFIED=>EMPTY but leave PARKED alone. - if self.state.compare_and_swap(NOTIFIED, EMPTY, Acquire) == NOTIFIED { + if self.state.compare_exchange(NOTIFIED, EMPTY, Acquire, Acquire) == NOTIFIED { // Actually woken up by unpark(). return; } else { From 454f3ed9029dc407b59b714cb60c00036a253a2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Linus=20F=C3=A4rnstrand?= Date: Tue, 22 Dec 2020 12:33:11 +0100 Subject: [PATCH 19/22] Update library/std/src/sys/windows/thread_parker.rs Co-authored-by: Mara Bos --- library/std/src/sys/windows/thread_parker.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/std/src/sys/windows/thread_parker.rs b/library/std/src/sys/windows/thread_parker.rs index e3585d4cb37a4..9e4c9aa0a512c 100644 --- a/library/std/src/sys/windows/thread_parker.rs +++ b/library/std/src/sys/windows/thread_parker.rs @@ -113,7 +113,7 @@ impl Parker { // Wait for something to happen, assuming it's still set to PARKED. c::WaitOnAddress(self.ptr(), &PARKED as *const _ as c::LPVOID, 1, c::INFINITE); // Change NOTIFIED=>EMPTY but leave PARKED alone. - if self.state.compare_exchange(NOTIFIED, EMPTY, Acquire, Acquire) == NOTIFIED { + if self.state.compare_exchange(NOTIFIED, EMPTY, Acquire, Acquire).is_ok() { // Actually woken up by unpark(). return; } else { From 9414f0b833734c344e795d590e9a845ce437c908 Mon Sep 17 00:00:00 2001 From: Wesley Wiser Date: Sat, 19 Dec 2020 16:29:38 -0500 Subject: [PATCH 20/22] Revert "Remove missing_fragment_specifier lint" This reverts commit 5ba961018c482e050af908de60e4f8bd1a00f0ae. --- compiler/rustc_lint_defs/src/builtin.rs | 17 +++++++++++++++++ .../rustc/src/lints/listing/deny-by-default.md | 3 --- 2 files changed, 17 insertions(+), 3 deletions(-) delete mode 100644 src/doc/rustc/src/lints/listing/deny-by-default.md diff --git a/compiler/rustc_lint_defs/src/builtin.rs b/compiler/rustc_lint_defs/src/builtin.rs index 6a48b8f4dfbb2..de5253a84cc06 100644 --- a/compiler/rustc_lint_defs/src/builtin.rs +++ b/compiler/rustc_lint_defs/src/builtin.rs @@ -1227,6 +1227,22 @@ declare_lint! { }; } +declare_lint! { + /// The missing_fragment_specifier warning is issued when an unused pattern in a + /// `macro_rules!` macro definition has a meta-variable (e.g. `$e`) that is not + /// followed by a fragment specifier (e.g. `:expr`). + /// + /// This warning can always be fixed by removing the unused pattern in the + /// `macro_rules!` macro definition. + pub MISSING_FRAGMENT_SPECIFIER, + Deny, + "detects missing fragment specifiers in unused `macro_rules!` patterns", + @future_incompatible = FutureIncompatibleInfo { + reference: "issue #40107 ", + edition: None, + }; +} + declare_lint! { /// The `late_bound_lifetime_arguments` lint detects generic lifetime /// arguments in path segments with late bound lifetime parameters. @@ -2827,6 +2843,7 @@ declare_lint_pass! { CONST_ITEM_MUTATION, SAFE_PACKED_BORROWS, PATTERNS_IN_FNS_WITHOUT_BODY, + MISSING_FRAGMENT_SPECIFIER, LATE_BOUND_LIFETIME_ARGUMENTS, ORDER_DEPENDENT_TRAIT_OBJECTS, COHERENCE_LEAK_CHECK, diff --git a/src/doc/rustc/src/lints/listing/deny-by-default.md b/src/doc/rustc/src/lints/listing/deny-by-default.md deleted file mode 100644 index 3c1452d64676c..0000000000000 --- a/src/doc/rustc/src/lints/listing/deny-by-default.md +++ /dev/null @@ -1,3 +0,0 @@ -# Deny-by-default lints - -This file is auto-generated by the lint-docs script. From f1eb88b28a84b0533ddd036a60bb5b8acbffabb2 Mon Sep 17 00:00:00 2001 From: Wesley Wiser Date: Sat, 19 Dec 2020 16:30:56 -0500 Subject: [PATCH 21/22] Revert "Promote missing_fragment_specifier to hard error" This reverts commit 02eae432e7476a0686633a8c2b7cb1d5aab1bd2c. --- compiler/rustc_expand/src/mbe.rs | 2 +- compiler/rustc_expand/src/mbe/macro_parser.rs | 20 ++++++++++++++--- compiler/rustc_expand/src/mbe/macro_rules.rs | 15 +++++++------ compiler/rustc_expand/src/mbe/quoted.rs | 11 ++++++---- compiler/rustc_interface/src/passes.rs | 19 +++++++++++++++- compiler/rustc_session/src/parse.rs | 2 ++ src/test/ui/lint/expansion-time.rs | 4 ++++ src/test/ui/lint/expansion-time.stderr | 22 +++++++++++++++---- src/test/ui/macros/issue-39404.rs | 1 + src/test/ui/macros/issue-39404.stderr | 4 ++++ src/test/ui/macros/macro-match-nonterminal.rs | 1 + .../ui/macros/macro-match-nonterminal.stderr | 4 ++++ src/test/ui/parser/macro/issue-33569.rs | 1 - src/test/ui/parser/macro/issue-33569.stderr | 16 +++++--------- 14 files changed, 90 insertions(+), 32 deletions(-) diff --git a/compiler/rustc_expand/src/mbe.rs b/compiler/rustc_expand/src/mbe.rs index eb4aab116f00f..cbc4d14a65a1b 100644 --- a/compiler/rustc_expand/src/mbe.rs +++ b/compiler/rustc_expand/src/mbe.rs @@ -84,7 +84,7 @@ enum TokenTree { /// e.g., `$var` MetaVar(Span, Ident), /// e.g., `$var:expr`. This is only used in the left hand side of MBE macros. - MetaVarDecl(Span, Ident /* name to bind */, NonterminalKind), + MetaVarDecl(Span, Ident /* name to bind */, Option), } impl TokenTree { diff --git a/compiler/rustc_expand/src/mbe/macro_parser.rs b/compiler/rustc_expand/src/mbe/macro_parser.rs index 3cf2d8f8ac1ef..0c44f5fe9e10a 100644 --- a/compiler/rustc_expand/src/mbe/macro_parser.rs +++ b/compiler/rustc_expand/src/mbe/macro_parser.rs @@ -378,6 +378,11 @@ fn nameize>( n_rec(sess, next_m, res.by_ref(), ret_val)?; } } + TokenTree::MetaVarDecl(span, _, None) => { + if sess.missing_fragment_specifiers.borrow_mut().remove(&span).is_some() { + return Err((span, "missing fragment specifier".to_string())); + } + } TokenTree::MetaVarDecl(sp, bind_name, _) => match ret_val .entry(MacroRulesNormalizedIdent::new(bind_name)) { @@ -446,6 +451,7 @@ fn or_pat_mode(edition: Edition) -> OrPatNonterminalMode { /// /// A `ParseResult`. Note that matches are kept track of through the items generated. fn inner_parse_loop<'root, 'tt>( + sess: &ParseSess, cur_items: &mut SmallVec<[MatcherPosHandle<'root, 'tt>; 1]>, next_items: &mut Vec>, eof_items: &mut SmallVec<[MatcherPosHandle<'root, 'tt>; 1]>, @@ -563,9 +569,16 @@ fn inner_parse_loop<'root, 'tt>( }))); } + // We need to match a metavar (but the identifier is invalid)... this is an error + TokenTree::MetaVarDecl(span, _, None) => { + if sess.missing_fragment_specifiers.borrow_mut().remove(&span).is_some() { + return Error(span, "missing fragment specifier".to_string()); + } + } + // We need to match a metavar with a valid ident... call out to the black-box // parser by adding an item to `bb_items`. - TokenTree::MetaVarDecl(span, _, kind) => { + TokenTree::MetaVarDecl(span, _, Some(kind)) => { // Built-in nonterminals never start with these tokens, so we can eliminate // them from consideration. // @@ -640,6 +653,7 @@ pub(super) fn parse_tt(parser: &mut Cow<'_, Parser<'_>>, ms: &[TokenTree]) -> Na // parsing from the black-box parser done. The result is that `next_items` will contain a // bunch of possible next matcher positions in `next_items`. match inner_parse_loop( + parser.sess, &mut cur_items, &mut next_items, &mut eof_items, @@ -701,7 +715,7 @@ pub(super) fn parse_tt(parser: &mut Cow<'_, Parser<'_>>, ms: &[TokenTree]) -> Na let nts = bb_items .iter() .map(|item| match item.top_elts.get_tt(item.idx) { - TokenTree::MetaVarDecl(_, bind, kind) => format!("{} ('{}')", kind, bind), + TokenTree::MetaVarDecl(_, bind, Some(kind)) => format!("{} ('{}')", kind, bind), _ => panic!(), }) .collect::>() @@ -731,7 +745,7 @@ pub(super) fn parse_tt(parser: &mut Cow<'_, Parser<'_>>, ms: &[TokenTree]) -> Na assert_eq!(bb_items.len(), 1); let mut item = bb_items.pop().unwrap(); - if let TokenTree::MetaVarDecl(span, _, kind) = item.top_elts.get_tt(item.idx) { + if let TokenTree::MetaVarDecl(span, _, Some(kind)) = item.top_elts.get_tt(item.idx) { let match_cur = item.match_cur; // We use the span of the metavariable declaration to determine any // edition-specific matching behavior for non-terminals. diff --git a/compiler/rustc_expand/src/mbe/macro_rules.rs b/compiler/rustc_expand/src/mbe/macro_rules.rs index 66463eeb90713..89d375b257da5 100644 --- a/compiler/rustc_expand/src/mbe/macro_rules.rs +++ b/compiler/rustc_expand/src/mbe/macro_rules.rs @@ -401,7 +401,7 @@ pub fn compile_declarative_macro( let diag = &sess.parse_sess.span_diagnostic; let lhs_nm = Ident::new(sym::lhs, def.span); let rhs_nm = Ident::new(sym::rhs, def.span); - let tt_spec = NonterminalKind::TT; + let tt_spec = Some(NonterminalKind::TT); // Parse the macro_rules! invocation let (macro_rules, body) = match &def.kind { @@ -578,7 +578,7 @@ fn check_lhs_no_empty_seq(sess: &ParseSess, tts: &[mbe::TokenTree]) -> bool { TokenTree::Sequence(span, ref seq) => { if seq.separator.is_none() && seq.tts.iter().all(|seq_tt| match *seq_tt { - TokenTree::MetaVarDecl(_, _, NonterminalKind::Vis) => true, + TokenTree::MetaVarDecl(_, _, Some(NonterminalKind::Vis)) => true, TokenTree::Sequence(_, ref sub_seq) => { sub_seq.kleene.op == mbe::KleeneOp::ZeroOrMore || sub_seq.kleene.op == mbe::KleeneOp::ZeroOrOne @@ -961,7 +961,7 @@ fn check_matcher_core( // Now `last` holds the complete set of NT tokens that could // end the sequence before SUFFIX. Check that every one works with `suffix`. for token in &last.tokens { - if let TokenTree::MetaVarDecl(_, name, kind) = *token { + if let TokenTree::MetaVarDecl(_, name, Some(kind)) = *token { for next_token in &suffix_first.tokens { match is_in_follow(next_token, kind) { IsInFollow::Yes => {} @@ -1019,7 +1019,7 @@ fn check_matcher_core( } fn token_can_be_followed_by_any(tok: &mbe::TokenTree) -> bool { - if let mbe::TokenTree::MetaVarDecl(_, _, kind) = *tok { + if let mbe::TokenTree::MetaVarDecl(_, _, Some(kind)) = *tok { frag_can_be_followed_by_any(kind) } else { // (Non NT's can always be followed by anything in matchers.) @@ -1123,7 +1123,7 @@ fn is_in_follow(tok: &mbe::TokenTree, kind: NonterminalKind) -> IsInFollow { } _ => IsInFollow::No(TOKENS), }, - TokenTree::MetaVarDecl(_, _, NonterminalKind::Block) => IsInFollow::Yes, + TokenTree::MetaVarDecl(_, _, Some(NonterminalKind::Block)) => IsInFollow::Yes, _ => IsInFollow::No(TOKENS), } } @@ -1158,7 +1158,7 @@ fn is_in_follow(tok: &mbe::TokenTree, kind: NonterminalKind) -> IsInFollow { TokenTree::MetaVarDecl( _, _, - NonterminalKind::Ident | NonterminalKind::Ty | NonterminalKind::Path, + Some(NonterminalKind::Ident | NonterminalKind::Ty | NonterminalKind::Path), ) => IsInFollow::Yes, _ => IsInFollow::No(TOKENS), } @@ -1171,7 +1171,8 @@ fn quoted_tt_to_string(tt: &mbe::TokenTree) -> String { match *tt { mbe::TokenTree::Token(ref token) => pprust::token_to_string(&token), mbe::TokenTree::MetaVar(_, name) => format!("${}", name), - mbe::TokenTree::MetaVarDecl(_, name, kind) => format!("${}:{}", name, kind), + mbe::TokenTree::MetaVarDecl(_, name, Some(kind)) => format!("${}:{}", name, kind), + mbe::TokenTree::MetaVarDecl(_, name, None) => format!("${}:", name), _ => panic!( "{}", "unexpected mbe::TokenTree::{Sequence or Delimited} \ diff --git a/compiler/rustc_expand/src/mbe/quoted.rs b/compiler/rustc_expand/src/mbe/quoted.rs index 48db532c78f30..01b11bb979d68 100644 --- a/compiler/rustc_expand/src/mbe/quoted.rs +++ b/compiler/rustc_expand/src/mbe/quoted.rs @@ -3,7 +3,7 @@ use crate::mbe::{Delimited, KleeneOp, KleeneToken, SequenceRepetition, TokenTree use rustc_ast::token::{self, Token}; use rustc_ast::tokenstream; -use rustc_ast::NodeId; +use rustc_ast::{NodeId, DUMMY_NODE_ID}; use rustc_ast_pretty::pprust; use rustc_session::parse::ParseSess; use rustc_span::symbol::{kw, Ident}; @@ -73,7 +73,7 @@ pub(super) fn parse( .emit(); token::NonterminalKind::Ident }); - result.push(TokenTree::MetaVarDecl(span, ident, kind)); + result.push(TokenTree::MetaVarDecl(span, ident, Some(kind))); continue; } _ => token.span, @@ -83,8 +83,11 @@ pub(super) fn parse( } tree => tree.as_ref().map(tokenstream::TokenTree::span).unwrap_or(start_sp), }; - sess.span_diagnostic.struct_span_err(span, "missing fragment specifier").emit(); - continue; + if node_id != DUMMY_NODE_ID { + // Macros loaded from other crates have dummy node ids. + sess.missing_fragment_specifiers.borrow_mut().insert(span, node_id); + } + result.push(TokenTree::MetaVarDecl(span, ident, None)); } // Not a metavar or no matchers allowed, so just return the tree diff --git a/compiler/rustc_interface/src/passes.rs b/compiler/rustc_interface/src/passes.rs index c13d26c79d738..a8c8690b9e7fa 100644 --- a/compiler/rustc_interface/src/passes.rs +++ b/compiler/rustc_interface/src/passes.rs @@ -29,6 +29,7 @@ use rustc_passes::{self, hir_stats, layout_test}; use rustc_plugin_impl as plugin; use rustc_resolve::{Resolver, ResolverArenas}; use rustc_session::config::{CrateType, Input, OutputFilenames, OutputType, PpMode, PpSourceMode}; +use rustc_session::lint; use rustc_session::output::{filename_for_input, filename_for_metadata}; use rustc_session::search_paths::PathKind; use rustc_session::Session; @@ -306,11 +307,27 @@ fn configure_and_expand_inner<'a>( ecx.check_unused_macros(); }); + let mut missing_fragment_specifiers: Vec<_> = ecx + .sess + .parse_sess + .missing_fragment_specifiers + .borrow() + .iter() + .map(|(span, node_id)| (*span, *node_id)) + .collect(); + missing_fragment_specifiers.sort_unstable_by_key(|(span, _)| *span); + + let recursion_limit_hit = ecx.reduced_recursion_limit.is_some(); + + for (span, node_id) in missing_fragment_specifiers { + let lint = lint::builtin::MISSING_FRAGMENT_SPECIFIER; + let msg = "missing fragment specifier"; + resolver.lint_buffer().buffer_lint(lint, node_id, span, msg); + } if cfg!(windows) { env::set_var("PATH", &old_path); } - let recursion_limit_hit = ecx.reduced_recursion_limit.is_some(); if recursion_limit_hit { // If we hit a recursion limit, exit early to avoid later passes getting overwhelmed // with a large AST diff --git a/compiler/rustc_session/src/parse.rs b/compiler/rustc_session/src/parse.rs index 66c3738fb5b5a..b1a4834241730 100644 --- a/compiler/rustc_session/src/parse.rs +++ b/compiler/rustc_session/src/parse.rs @@ -119,6 +119,7 @@ pub struct ParseSess { pub unstable_features: UnstableFeatures, pub config: CrateConfig, pub edition: Edition, + pub missing_fragment_specifiers: Lock>, /// Places where raw identifiers were used. This is used for feature-gating raw identifiers. pub raw_identifier_spans: Lock>, /// Used to determine and report recursive module inclusions. @@ -152,6 +153,7 @@ impl ParseSess { unstable_features: UnstableFeatures::from_environment(None), config: FxHashSet::default(), edition: ExpnId::root().expn_data().edition, + missing_fragment_specifiers: Default::default(), raw_identifier_spans: Lock::new(Vec::new()), included_mod_stack: Lock::new(vec![]), source_map, diff --git a/src/test/ui/lint/expansion-time.rs b/src/test/ui/lint/expansion-time.rs index a9c7ac363b0b3..f23c7cb0dca14 100644 --- a/src/test/ui/lint/expansion-time.rs +++ b/src/test/ui/lint/expansion-time.rs @@ -5,6 +5,10 @@ macro_rules! foo { ( $($i:ident)* ) => { $($i)+ }; //~ WARN meta-variable repeats with different Kleene operator } +#[warn(missing_fragment_specifier)] +macro_rules! m { ($i) => {} } //~ WARN missing fragment specifier + //~| WARN this was previously accepted + #[warn(soft_unstable)] mod benches { #[bench] //~ WARN use of unstable library feature 'test' diff --git a/src/test/ui/lint/expansion-time.stderr b/src/test/ui/lint/expansion-time.stderr index 24e2733064e48..b0fc1f8e5eec7 100644 --- a/src/test/ui/lint/expansion-time.stderr +++ b/src/test/ui/lint/expansion-time.stderr @@ -12,14 +12,28 @@ note: the lint level is defined here LL | #[warn(meta_variable_misuse)] | ^^^^^^^^^^^^^^^^^^^^ +warning: missing fragment specifier + --> $DIR/expansion-time.rs:9:19 + | +LL | macro_rules! m { ($i) => {} } + | ^^ + | +note: the lint level is defined here + --> $DIR/expansion-time.rs:8:8 + | +LL | #[warn(missing_fragment_specifier)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + = 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 #40107 + warning: use of unstable library feature 'test': `bench` is a part of custom test frameworks which are unstable - --> $DIR/expansion-time.rs:10:7 + --> $DIR/expansion-time.rs:14:7 | LL | #[bench] | ^^^^^ | note: the lint level is defined here - --> $DIR/expansion-time.rs:8:8 + --> $DIR/expansion-time.rs:12:8 | LL | #[warn(soft_unstable)] | ^^^^^^^^^^^^^ @@ -33,10 +47,10 @@ LL | 2 | ^ | note: the lint level is defined here - --> $DIR/expansion-time.rs:25:8 + --> $DIR/expansion-time.rs:29:8 | LL | #[warn(incomplete_include)] | ^^^^^^^^^^^^^^^^^^ -warning: 3 warnings emitted +warning: 4 warnings emitted diff --git a/src/test/ui/macros/issue-39404.rs b/src/test/ui/macros/issue-39404.rs index 054958ba00b8d..2229f2c3900c3 100644 --- a/src/test/ui/macros/issue-39404.rs +++ b/src/test/ui/macros/issue-39404.rs @@ -2,5 +2,6 @@ macro_rules! m { ($i) => {} } //~^ ERROR missing fragment specifier +//~| WARN previously accepted fn main() {} diff --git a/src/test/ui/macros/issue-39404.stderr b/src/test/ui/macros/issue-39404.stderr index 645f06e59d817..d2f2a823c2a6b 100644 --- a/src/test/ui/macros/issue-39404.stderr +++ b/src/test/ui/macros/issue-39404.stderr @@ -3,6 +3,10 @@ error: missing fragment specifier | LL | macro_rules! m { ($i) => {} } | ^^ + | + = note: `#[deny(missing_fragment_specifier)]` 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 #40107 error: aborting due to previous error diff --git a/src/test/ui/macros/macro-match-nonterminal.rs b/src/test/ui/macros/macro-match-nonterminal.rs index 6b023e4137274..b23e5c71c03f0 100644 --- a/src/test/ui/macros/macro-match-nonterminal.rs +++ b/src/test/ui/macros/macro-match-nonterminal.rs @@ -2,6 +2,7 @@ macro_rules! test { ($a, $b) => { //~^ ERROR missing fragment //~| ERROR missing fragment + //~| WARN this was previously accepted () }; } diff --git a/src/test/ui/macros/macro-match-nonterminal.stderr b/src/test/ui/macros/macro-match-nonterminal.stderr index 334d62812cdab..674ce3434aac6 100644 --- a/src/test/ui/macros/macro-match-nonterminal.stderr +++ b/src/test/ui/macros/macro-match-nonterminal.stderr @@ -9,6 +9,10 @@ error: missing fragment specifier | LL | ($a, $b) => { | ^^ + | + = note: `#[deny(missing_fragment_specifier)]` 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 #40107 error: aborting due to 2 previous errors diff --git a/src/test/ui/parser/macro/issue-33569.rs b/src/test/ui/parser/macro/issue-33569.rs index cf81f0480a2a7..80e2d7c6545ba 100644 --- a/src/test/ui/parser/macro/issue-33569.rs +++ b/src/test/ui/parser/macro/issue-33569.rs @@ -2,7 +2,6 @@ macro_rules! foo { { $+ } => { //~ ERROR expected identifier, found `+` //~^ ERROR missing fragment specifier $(x)(y) //~ ERROR expected one of: `*`, `+`, or `?` - //~^ ERROR attempted to repeat an expression containing no syntax variables } } diff --git a/src/test/ui/parser/macro/issue-33569.stderr b/src/test/ui/parser/macro/issue-33569.stderr index f54efaa6996f2..b4d38d3ce4806 100644 --- a/src/test/ui/parser/macro/issue-33569.stderr +++ b/src/test/ui/parser/macro/issue-33569.stderr @@ -4,23 +4,17 @@ error: expected identifier, found `+` LL | { $+ } => { | ^ -error: missing fragment specifier - --> $DIR/issue-33569.rs:2:8 - | -LL | { $+ } => { - | ^ - error: expected one of: `*`, `+`, or `?` --> $DIR/issue-33569.rs:4:13 | LL | $(x)(y) | ^^^ -error: attempted to repeat an expression containing no syntax variables matched as repeating at this depth - --> $DIR/issue-33569.rs:4:10 +error: missing fragment specifier + --> $DIR/issue-33569.rs:2:8 | -LL | $(x)(y) - | ^^^ +LL | { $+ } => { + | ^ -error: aborting due to 4 previous errors +error: aborting due to 3 previous errors From 56154a11473da93da0f5d57f4692991ae4972695 Mon Sep 17 00:00:00 2001 From: Wesley Wiser Date: Sun, 20 Dec 2020 21:44:27 -0500 Subject: [PATCH 22/22] Add example to lint docs --- compiler/rustc_lint_defs/src/builtin.rs | 30 ++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/compiler/rustc_lint_defs/src/builtin.rs b/compiler/rustc_lint_defs/src/builtin.rs index de5253a84cc06..1c692d4f20762 100644 --- a/compiler/rustc_lint_defs/src/builtin.rs +++ b/compiler/rustc_lint_defs/src/builtin.rs @@ -1228,12 +1228,40 @@ declare_lint! { } declare_lint! { - /// The missing_fragment_specifier warning is issued when an unused pattern in a + /// The `missing_fragment_specifier` lint is issued when an unused pattern in a /// `macro_rules!` macro definition has a meta-variable (e.g. `$e`) that is not /// followed by a fragment specifier (e.g. `:expr`). /// /// This warning can always be fixed by removing the unused pattern in the /// `macro_rules!` macro definition. + /// + /// ### Example + /// + /// ```rust,compile_fail + /// macro_rules! foo { + /// () => {}; + /// ($name) => { }; + /// } + /// + /// fn main() { + /// foo!(); + /// } + /// ``` + /// + /// {{produces}} + /// + /// ### Explanation + /// + /// To fix this, remove the unused pattern from the `macro_rules!` macro definition: + /// + /// ```rust + /// macro_rules! foo { + /// () => {}; + /// } + /// fn main() { + /// foo!(); + /// } + /// ``` pub MISSING_FRAGMENT_SPECIFIER, Deny, "detects missing fragment specifiers in unused `macro_rules!` patterns",