From 42e435cbbbac558916dc1b485d2e2b566e17f0e1 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Mon, 7 Oct 2024 15:57:10 -0700 Subject: [PATCH 01/10] Move rules handling to its own module There should otherwise be no code changes here. --- mdbook-spec/src/lib.rs | 39 +------------------------------- mdbook-spec/src/rules.rs | 48 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 38 deletions(-) create mode 100644 mdbook-spec/src/rules.rs diff --git a/mdbook-spec/src/lib.rs b/mdbook-spec/src/lib.rs index 523453131..03b9234ae 100644 --- a/mdbook-spec/src/lib.rs +++ b/mdbook-spec/src/lib.rs @@ -11,11 +11,9 @@ use std::collections::BTreeMap; use std::io; use std::path::PathBuf; +mod rules; mod std_links; -/// The Regex for rules like `r[foo]`. -static RULE_RE: Lazy = Lazy::new(|| Regex::new(r"(?m)^r\[([^]]+)]$").unwrap()); - /// The Regex for the syntax for blockquotes that have a specific CSS class, /// like `> [!WARNING]`. static ADMONITION_RE: Lazy = Lazy::new(|| { @@ -57,41 +55,6 @@ impl Spec { } } - /// Converts lines that start with `r[…]` into a "rule" which has special - /// styling and can be linked to. - fn rule_definitions( - &self, - chapter: &Chapter, - found_rules: &mut BTreeMap, - ) -> String { - let source_path = chapter.source_path.clone().unwrap_or_default(); - let path = chapter.path.clone().unwrap_or_default(); - RULE_RE - .replace_all(&chapter.content, |caps: &Captures<'_>| { - let rule_id = &caps[1]; - if let Some((old, _)) = - found_rules.insert(rule_id.to_string(), (source_path.clone(), path.clone())) - { - let message = format!( - "rule `{rule_id}` defined multiple times\n\ - First location: {old:?}\n\ - Second location: {source_path:?}" - ); - if self.deny_warnings { - panic!("error: {message}"); - } else { - eprintln!("warning: {message}"); - } - } - format!( - "
\ - [{rule_id}]\ -
\n" - ) - }) - .to_string() - } - /// Generates link references to all rules on all pages, so you can easily /// refer to rules anywhere in the book. fn auto_link_references( diff --git a/mdbook-spec/src/rules.rs b/mdbook-spec/src/rules.rs new file mode 100644 index 000000000..65c53c76e --- /dev/null +++ b/mdbook-spec/src/rules.rs @@ -0,0 +1,48 @@ +//! Handling for rule identifiers. + +use crate::Spec; +use mdbook::book::Chapter; +use once_cell::sync::Lazy; +use regex::{Captures, Regex}; +use std::collections::BTreeMap; +use std::path::PathBuf; + +/// The Regex for rules like `r[foo]`. +static RULE_RE: Lazy = Lazy::new(|| Regex::new(r"(?m)^r\[([^]]+)]$").unwrap()); + +impl Spec { + /// Converts lines that start with `r[…]` into a "rule" which has special + /// styling and can be linked to. + pub fn rule_definitions( + &self, + chapter: &Chapter, + found_rules: &mut BTreeMap, + ) -> String { + let source_path = chapter.source_path.clone().unwrap_or_default(); + let path = chapter.path.clone().unwrap_or_default(); + RULE_RE + .replace_all(&chapter.content, |caps: &Captures<'_>| { + let rule_id = &caps[1]; + if let Some((old, _)) = + found_rules.insert(rule_id.to_string(), (source_path.clone(), path.clone())) + { + let message = format!( + "rule `{rule_id}` defined multiple times\n\ + First location: {old:?}\n\ + Second location: {source_path:?}" + ); + if self.deny_warnings { + panic!("error: {message}"); + } else { + eprintln!("warning: {message}"); + } + } + format!( + "
\ + [{rule_id}]\ +
\n" + ) + }) + .to_string() + } +} From f99b45ae7efc149c1464991ab0b55868d6c3c4c3 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Mon, 7 Oct 2024 16:00:44 -0700 Subject: [PATCH 02/10] Move Spec creation out of main This is in preparation for error handling in future commits. --- mdbook-spec/src/lib.rs | 11 ++++++----- mdbook-spec/src/main.rs | 4 +--- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/mdbook-spec/src/lib.rs b/mdbook-spec/src/lib.rs index 03b9234ae..e56943107 100644 --- a/mdbook-spec/src/lib.rs +++ b/mdbook-spec/src/lib.rs @@ -1,5 +1,6 @@ #![deny(rust_2018_idioms, unused_lifetimes)] +use anyhow::Result; use mdbook::book::{Book, Chapter}; use mdbook::errors::Error; use mdbook::preprocess::{CmdPreprocessor, Preprocessor, PreprocessorContext}; @@ -20,7 +21,8 @@ static ADMONITION_RE: Lazy = Lazy::new(|| { Regex::new(r"(?m)^ *> \[!(?[^]]+)\]\n(?
(?: *>.*\n)+)").unwrap() }); -pub fn handle_preprocessing(pre: &dyn Preprocessor) -> Result<(), Error> { +pub fn handle_preprocessing() -> Result<(), Error> { + let pre = Spec::new()?; let (ctx, book) = CmdPreprocessor::parse_input(io::stdin())?; let book_version = Version::parse(&ctx.mdbook_version)?; @@ -49,10 +51,9 @@ pub struct Spec { } impl Spec { - pub fn new() -> Spec { - Spec { - deny_warnings: std::env::var("SPEC_DENY_WARNINGS").as_deref() == Ok("1"), - } + fn new() -> Result { + let deny_warnings = std::env::var("SPEC_DENY_WARNINGS").as_deref() == Ok("1"); + Ok(Spec { deny_warnings }) } /// Generates link references to all rules on all pages, so you can easily diff --git a/mdbook-spec/src/main.rs b/mdbook-spec/src/main.rs index 56e11d760..83ac83046 100644 --- a/mdbook-spec/src/main.rs +++ b/mdbook-spec/src/main.rs @@ -12,9 +12,7 @@ fn main() { None => {} } - let preprocessor = mdbook_spec::Spec::new(); - - if let Err(e) = mdbook_spec::handle_preprocessing(&preprocessor) { + if let Err(e) = mdbook_spec::handle_preprocessing() { eprintln!("{}", e); std::process::exit(1); } From 160bb6a0415fb11f07c5efe40715a58060af387e Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Mon, 7 Oct 2024 16:09:21 -0700 Subject: [PATCH 03/10] Move rule collection into a separate pass with a new structure This adds a "Rules" struct which encapsulates the rules in the document. This also removes some of the mutable variables. This is paving the way for future updates which extends information and processing about rules. --- mdbook-spec/src/lib.rs | 30 +++++---------- mdbook-spec/src/rules.rs | 79 ++++++++++++++++++++++++++++------------ 2 files changed, 65 insertions(+), 44 deletions(-) diff --git a/mdbook-spec/src/lib.rs b/mdbook-spec/src/lib.rs index e56943107..add3b25e8 100644 --- a/mdbook-spec/src/lib.rs +++ b/mdbook-spec/src/lib.rs @@ -1,5 +1,6 @@ #![deny(rust_2018_idioms, unused_lifetimes)] +use crate::rules::Rules; use anyhow::Result; use mdbook::book::{Book, Chapter}; use mdbook::errors::Error; @@ -8,9 +9,7 @@ use mdbook::BookItem; use once_cell::sync::Lazy; use regex::{Captures, Regex}; use semver::{Version, VersionReq}; -use std::collections::BTreeMap; use std::io; -use std::path::PathBuf; mod rules; mod std_links; @@ -58,13 +57,10 @@ impl Spec { /// Generates link references to all rules on all pages, so you can easily /// refer to rules anywhere in the book. - fn auto_link_references( - &self, - chapter: &Chapter, - found_rules: &BTreeMap, - ) -> String { + fn auto_link_references(&self, chapter: &Chapter, rules: &Rules) -> String { let current_path = chapter.path.as_ref().unwrap().parent().unwrap(); - let definitions: String = found_rules + let definitions: String = rules + .def_paths .iter() .map(|(rule_id, (_, path))| { let relative = pathdiff::diff_paths(path, current_path).unwrap(); @@ -125,7 +121,8 @@ impl Preprocessor for Spec { } fn run(&self, _ctx: &PreprocessorContext, mut book: Book) -> Result { - let mut found_rules = BTreeMap::new(); + let rules = self.collect_rules(&book); + book.for_each_mut(|item| { let BookItem::Chapter(ch) = item else { return; @@ -133,20 +130,11 @@ impl Preprocessor for Spec { if ch.is_draft_chapter() { return; } - ch.content = self.rule_definitions(&ch, &mut found_rules); ch.content = self.admonitions(&ch); + ch.content = self.auto_link_references(&ch, &rules); + ch.content = self.render_rule_definitions(&ch.content); }); - // This is a separate pass because it relies on the modifications of - // the previous passes. - book.for_each_mut(|item| { - let BookItem::Chapter(ch) = item else { - return; - }; - if ch.is_draft_chapter() { - return; - } - ch.content = self.auto_link_references(&ch, &found_rules); - }); + // Final pass will resolve everything as a std link (or error if the // link is unknown). std_links::std_links(&mut book); diff --git a/mdbook-spec/src/rules.rs b/mdbook-spec/src/rules.rs index 65c53c76e..fdbd2f76d 100644 --- a/mdbook-spec/src/rules.rs +++ b/mdbook-spec/src/rules.rs @@ -1,7 +1,8 @@ //! Handling for rule identifiers. use crate::Spec; -use mdbook::book::Chapter; +use mdbook::book::Book; +use mdbook::BookItem; use once_cell::sync::Lazy; use regex::{Captures, Regex}; use std::collections::BTreeMap; @@ -10,33 +11,65 @@ use std::path::PathBuf; /// The Regex for rules like `r[foo]`. static RULE_RE: Lazy = Lazy::new(|| Regex::new(r"(?m)^r\[([^]]+)]$").unwrap()); +/// The set of rules defined in the reference. +#[derive(Default)] +pub struct Rules { + /// A mapping from a rule identifier to a tuple of `(source_path, path)`. + /// + /// `source_path` is the path to the markdown source file relative to the + /// `SUMMARY.md`. + /// + /// `path` is the same as `source_path`, except filenames like `README.md` + /// are translated to `index.md`. Which to use depends on if you are + /// trying to access the source files (`source_path`), or creating links + /// in the output (`path`). + pub def_paths: BTreeMap, +} + impl Spec { + /// Collects all rule definitions in the book. + pub fn collect_rules(&self, book: &Book) -> Rules { + let mut rules = Rules::default(); + for item in book.iter() { + let BookItem::Chapter(ch) = item else { + continue; + }; + if ch.is_draft_chapter() { + continue; + } + RULE_RE + .captures_iter(&ch.content) + .for_each(|caps: Captures<'_>| { + let rule_id = &caps[1]; + let source_path = ch.source_path.clone().unwrap_or_default(); + let path = ch.path.clone().unwrap_or_default(); + if let Some((old, _)) = rules + .def_paths + .insert(rule_id.to_string(), (source_path.clone(), path.clone())) + { + let message = format!( + "rule `{rule_id}` defined multiple times\n\ + First location: {old:?}\n\ + Second location: {source_path:?}" + ); + if self.deny_warnings { + panic!("error: {message}"); + } else { + eprintln!("warning: {message}"); + } + } + }); + } + + rules + } + /// Converts lines that start with `r[…]` into a "rule" which has special /// styling and can be linked to. - pub fn rule_definitions( - &self, - chapter: &Chapter, - found_rules: &mut BTreeMap, - ) -> String { - let source_path = chapter.source_path.clone().unwrap_or_default(); - let path = chapter.path.clone().unwrap_or_default(); + pub fn render_rule_definitions(&self, content: &str) -> String { RULE_RE - .replace_all(&chapter.content, |caps: &Captures<'_>| { + .replace_all(content, |caps: &Captures<'_>| { let rule_id = &caps[1]; - if let Some((old, _)) = - found_rules.insert(rule_id.to_string(), (source_path.clone(), path.clone())) - { - let message = format!( - "rule `{rule_id}` defined multiple times\n\ - First location: {old:?}\n\ - Second location: {source_path:?}" - ); - if self.deny_warnings { - panic!("error: {message}"); - } else { - eprintln!("warning: {message}"); - } - } format!( "
\ [{rule_id}]\ From 9165a56a2f94a47300b5f18be06f0110695dfa24 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Mon, 7 Oct 2024 16:15:19 -0700 Subject: [PATCH 04/10] Add tracking of rule prefixes This adds tracking of rule prefixes (like `asm.ts-args` is an interior prefix of `asm.ts-args.syntax`). This will be used for test linking, to check for tests that link to these. I'm not sure what we'll want to do with these long-term, whether we want to allow linking these "uber" rules, or remove them, or just warn about them. --- mdbook-spec/src/rules.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/mdbook-spec/src/rules.rs b/mdbook-spec/src/rules.rs index fdbd2f76d..ac402b880 100644 --- a/mdbook-spec/src/rules.rs +++ b/mdbook-spec/src/rules.rs @@ -5,7 +5,7 @@ use mdbook::book::Book; use mdbook::BookItem; use once_cell::sync::Lazy; use regex::{Captures, Regex}; -use std::collections::BTreeMap; +use std::collections::{BTreeMap, HashSet}; use std::path::PathBuf; /// The Regex for rules like `r[foo]`. @@ -24,6 +24,10 @@ pub struct Rules { /// trying to access the source files (`source_path`), or creating links /// in the output (`path`). pub def_paths: BTreeMap, + /// Set of rule name prefixes that have more specific rules within. + /// + /// For example, `asm.ts-args` is an interior prefix of `asm.ts-args.syntax`. + pub interior_prefixes: HashSet, } impl Spec { @@ -58,6 +62,12 @@ impl Spec { eprintln!("warning: {message}"); } } + let mut parts: Vec<_> = rule_id.split('.').collect(); + while !parts.is_empty() { + parts.pop(); + let prefix = parts.join("."); + rules.interior_prefixes.insert(prefix); + } }); } From c44b031ad864a9e97dff1c5cfb0c17791ca9eaf6 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Mon, 7 Oct 2024 16:26:02 -0700 Subject: [PATCH 05/10] More directly document the SPEC_RELATIVE environment variable --- README.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index a296a3b38..ed97c7ddb 100644 --- a/README.md +++ b/README.md @@ -66,10 +66,14 @@ SPEC_RELATIVE=0 mdbook build --open This will open a browser with a websocket live-link to automatically reload whenever the source is updated. -The `SPEC_RELATIVE=0` environment variable makes links to the standard library go to instead of being relative, which is useful when viewing locally since you normally don't have a copy of the standard library. - You can also use mdbook's live webserver option, which will automatically rebuild the book and reload your web browser whenever a source file is modified: ```sh SPEC_RELATIVE=0 mdbook serve --open ``` + +### `SPEC_RELATIVE` + +The `SPEC_RELATIVE=0` environment variable makes links to the standard library go to instead of being relative, which is useful when viewing locally since you normally don't have a copy of the standard library. + +The published site at (or local docs using `rustup doc`) does not set this, which means it will use relative links which supports offline viewing and links to the correct version (for example, links in will stay within the 1.81.0 directory). From 84414cc50d173fbded7f7b6dba517973bee8f689 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Mon, 7 Oct 2024 16:26:59 -0700 Subject: [PATCH 06/10] Document SPEC_DENY_WARNINGS --- README.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/README.md b/README.md index ed97c7ddb..af6310abc 100644 --- a/README.md +++ b/README.md @@ -77,3 +77,7 @@ SPEC_RELATIVE=0 mdbook serve --open The `SPEC_RELATIVE=0` environment variable makes links to the standard library go to instead of being relative, which is useful when viewing locally since you normally don't have a copy of the standard library. The published site at (or local docs using `rustup doc`) does not set this, which means it will use relative links which supports offline viewing and links to the correct version (for example, links in will stay within the 1.81.0 directory). + +### `SPEC_DENY_WARNINGS` + +The `SPEC_DENY_WARNINGS=1` environment variable will turn all warnings generated by `mdbook-spec` to errors. This is used in CI to ensure that there aren't any problems with the book content. From 9ea5e0e2f37cd8a293b59559c51ee7b694bcf7ae Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Mon, 7 Oct 2024 16:37:35 -0700 Subject: [PATCH 07/10] Add test linking This adds the ability to link rust-lang/rust tests to reference rule annotations. Rules now have a link that pops up a list of tests that are associated with that rule. Additionally, there is a new appendix that lists the number of rules in each chapter, how many tests are associated, and various summaries. This requires a local checkout of rust-lang/rust which is pointed to by the `SPEC_RUST_ROOT` environment variable. --- .github/workflows/main.yml | 14 ++- README.md | 4 + book.toml | 1 + docs/authoring.md | 12 ++ mdbook-spec/Cargo.lock | 31 +++++- mdbook-spec/Cargo.toml | 1 + mdbook-spec/src/lib.rs | 60 +++++++++- mdbook-spec/src/rules.rs | 26 ++++- mdbook-spec/src/test_links.rs | 203 ++++++++++++++++++++++++++++++++++ src/SUMMARY.md | 1 + src/test-summary.md | 5 + theme/reference.css | 30 +++++ theme/reference.js | 24 ++++ 13 files changed, 403 insertions(+), 9 deletions(-) create mode 100644 mdbook-spec/src/test_links.rs create mode 100644 src/test-summary.md create mode 100644 theme/reference.js diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index ccb3d0c6c..0c2b9b884 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -35,6 +35,11 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@master + - name: Checkout rust-lang/rust + uses: actions/checkout@master + with: + repository: rust-lang/rust + path: rust - name: Update rustup run: rustup self update - name: Install Rust @@ -52,16 +57,17 @@ jobs: rustup --version rustc -Vv mdbook --version - - name: Verify the book builds - env: - SPEC_DENY_WARNINGS: 1 - run: mdbook build - name: Style checks working-directory: style-check run: cargo run --locked -- ../src - name: Style fmt working-directory: style-check run: cargo fmt --check + - name: Verify the book builds + env: + SPEC_DENY_WARNINGS: 1 + SPEC_RUST_ROOT: ${{ github.workspace }}/rust + run: mdbook build - name: Check for broken links run: | curl -sSLo linkcheck.sh \ diff --git a/README.md b/README.md index af6310abc..0c7f3c496 100644 --- a/README.md +++ b/README.md @@ -81,3 +81,7 @@ The published site at (or local docs usin ### `SPEC_DENY_WARNINGS` The `SPEC_DENY_WARNINGS=1` environment variable will turn all warnings generated by `mdbook-spec` to errors. This is used in CI to ensure that there aren't any problems with the book content. + +### `SPEC_RUST_ROOT` + +The `SPEC_RUST_ROOT` can be used to point to the directory of a checkout of . This is used by the test-linking feature so that it can find tests linked to reference rules. If this is not set, then the tests won't be linked. diff --git a/book.toml b/book.toml index 404b8cca8..18a99728b 100644 --- a/book.toml +++ b/book.toml @@ -5,6 +5,7 @@ author = "The Rust Project Developers" [output.html] additional-css = ["theme/reference.css"] +additional-js = ["theme/reference.js"] git-repository-url = "https://github.com/rust-lang/reference/" edit-url-template = "https://github.com/rust-lang/reference/edit/master/{path}" smart-punctuation = true diff --git a/docs/authoring.md b/docs/authoring.md index cd5eaa752..fa32fc281 100644 --- a/docs/authoring.md +++ b/docs/authoring.md @@ -99,6 +99,18 @@ When assigning rules to new paragraphs, or when modifying rule names, use the fo * Target specific admonitions should typically be named by the least specific target property to which they apply (e.g. if a rule affects all x86 CPUs, the rule name should include `x86` rather than separately listing `i586`, `i686` and `x86_64`, and if a rule applies to all ELF platforms, it should be named `elf` rather than listing every ELF OS). * Use an appropriately descriptive, but short, name if the language does not provide one. +#### Test rule annotations + +Tests in can be linked to rules in the reference. The rule will include a link to the tests, and there is also an appendix which tracks how the rules are currently linked. + +Tests in the `tests` directory can be annotated with the `//@ reference: x.y.z` header to link it to a rule. The header can be specified multiple times if a single file covers multiple rules. + +You *should* when possible make sure every rule has a test associated with it. This is beneficial for reviewers to see the behavior and readers who may want to see examples of particular behaviors. When adding new rules, you should wait until the reference side is approved before submitting a PR to `rust-lang/rust` (to avoid churn if we decide on different names). + +Prefixed rule names should not be used in tests. That is, do not use something like `asm.rules` when there are specific rules like `asm.rules.reg-not-input`. + +We are not expecting 100% coverage at any time. Although it would be nice, it is unrealistic due to the sequence things are developed, and resources available. + ### Standard library links You should link to the standard library without specifying a URL in a fashion similar to [rustdoc intra-doc links][intra]. Some examples: diff --git a/mdbook-spec/Cargo.lock b/mdbook-spec/Cargo.lock index ff835b409..b101e142f 100644 --- a/mdbook-spec/Cargo.lock +++ b/mdbook-spec/Cargo.lock @@ -1,6 +1,6 @@ # This file is automatically @generated by Cargo. # It is not intended for manual editing. -version = 3 +version = 4 [[package]] name = "aho-corasick" @@ -412,6 +412,7 @@ dependencies = [ "semver", "serde_json", "tempfile", + "walkdir", ] [[package]] @@ -597,6 +598,15 @@ version = "1.0.18" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f3cb5ba0dc43242ce17de99c180e96db90b235b8a9fdc9543c96d2209116bd9f" +[[package]] +name = "same-file" +version = "1.0.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "93fc1dc3aaa9bfed95e02e6eadabb4baf7e3078b0bd1b4d7b6b0b68378900502" +dependencies = [ + "winapi-util", +] + [[package]] name = "semver" version = "1.0.23" @@ -764,6 +774,16 @@ version = "0.9.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "49874b5167b65d7193b8aba1567f5c7d93d001cafc34600cee003eda787e483f" +[[package]] +name = "walkdir" +version = "2.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "29790946404f91d9c5d06f9874efddea1dc06c5efe94541a7d6863108e3a5e4b" +dependencies = [ + "same-file", + "winapi-util", +] + [[package]] name = "wasm-bindgen" version = "0.2.92" @@ -834,6 +854,15 @@ version = "0.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ac3b87c63620426dd9b991e5ce0329eff545bccbbb34f3be09ff6fb6ab51b7b6" +[[package]] +name = "winapi-util" +version = "0.1.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cf221c93e13a30d793f7645a0e7762c55d169dbb0a49671918a2319d289b10bb" +dependencies = [ + "windows-sys 0.52.0", +] + [[package]] name = "winapi-x86_64-pc-windows-gnu" version = "0.4.0" diff --git a/mdbook-spec/Cargo.toml b/mdbook-spec/Cargo.toml index 703322cb0..c9a6e31af 100644 --- a/mdbook-spec/Cargo.toml +++ b/mdbook-spec/Cargo.toml @@ -19,3 +19,4 @@ regex = "1.9.4" semver = "1.0.21" serde_json = "1.0.113" tempfile = "3.10.1" +walkdir = "2.5.0" diff --git a/mdbook-spec/src/lib.rs b/mdbook-spec/src/lib.rs index add3b25e8..e55935023 100644 --- a/mdbook-spec/src/lib.rs +++ b/mdbook-spec/src/lib.rs @@ -1,7 +1,7 @@ #![deny(rust_2018_idioms, unused_lifetimes)] use crate::rules::Rules; -use anyhow::Result; +use anyhow::{bail, Context, Result}; use mdbook::book::{Book, Chapter}; use mdbook::errors::Error; use mdbook::preprocess::{CmdPreprocessor, Preprocessor, PreprocessorContext}; @@ -10,9 +10,11 @@ use once_cell::sync::Lazy; use regex::{Captures, Regex}; use semver::{Version, VersionReq}; use std::io; +use std::path::PathBuf; mod rules; mod std_links; +mod test_links; /// The Regex for the syntax for blockquotes that have a specific CSS class, /// like `> [!WARNING]`. @@ -47,12 +49,37 @@ pub struct Spec { /// Whether or not warnings should be errors (set by SPEC_DENY_WARNINGS /// environment variable). deny_warnings: bool, + /// Path to the rust-lang/rust git repository (set by SPEC_RUST_ROOT + /// environment variable). + rust_root: Option, + /// The git ref that can be used in a URL to the rust-lang/rust repository. + git_ref: String, } impl Spec { fn new() -> Result { let deny_warnings = std::env::var("SPEC_DENY_WARNINGS").as_deref() == Ok("1"); - Ok(Spec { deny_warnings }) + let rust_root = std::env::var_os("SPEC_RUST_ROOT").map(PathBuf::from); + if deny_warnings && rust_root.is_none() { + bail!("SPEC_RUST_ROOT environment variable must be set"); + } + let git_ref = match git_ref(&rust_root) { + Ok(s) => s, + Err(e) => { + if deny_warnings { + eprintln!("error: {e:?}"); + std::process::exit(1); + } else { + eprintln!("warning: {e:?}"); + "master".into() + } + } + }; + Ok(Spec { + deny_warnings, + rust_root, + git_ref, + }) } /// Generates link references to all rules on all pages, so you can easily @@ -115,6 +142,28 @@ fn to_initial_case(s: &str) -> String { format!("{first}{rest}") } +/// Determines the git ref used for linking to a particular branch/tag in GitHub. +fn git_ref(rust_root: &Option) -> Result { + let Some(rust_root) = rust_root else { + return Ok("master".into()); + }; + let channel = std::fs::read_to_string(rust_root.join("src/ci/channel")) + .context("failed to read src/ci/channel")?; + let git_ref = match channel.trim() { + // nightly/beta are branches, not stable references. Should be ok + // because we're not expecting those channels to be long-lived. + "nightly" => "master".into(), + "beta" => "beta".into(), + "stable" => { + let version = std::fs::read_to_string(rust_root.join("src/version")) + .context("|| failed to read src/version")?; + version.trim().into() + } + ch => bail!("unknown channel {ch}"), + }; + Ok(git_ref) +} + impl Preprocessor for Spec { fn name(&self) -> &str { "spec" @@ -122,6 +171,8 @@ impl Preprocessor for Spec { fn run(&self, _ctx: &PreprocessorContext, mut book: Book) -> Result { let rules = self.collect_rules(&book); + let tests = self.collect_tests(&rules); + let summary_table = test_links::make_summary_table(&book, &tests, &rules); book.for_each_mut(|item| { let BookItem::Chapter(ch) = item else { @@ -132,7 +183,10 @@ impl Preprocessor for Spec { } ch.content = self.admonitions(&ch); ch.content = self.auto_link_references(&ch, &rules); - ch.content = self.render_rule_definitions(&ch.content); + ch.content = self.render_rule_definitions(&ch.content, &tests); + if ch.name == "Test summary" { + ch.content = ch.content.replace("{{summary-table}}", &summary_table); + } }); // Final pass will resolve everything as a std link (or error if the diff --git a/mdbook-spec/src/rules.rs b/mdbook-spec/src/rules.rs index ac402b880..b477ab721 100644 --- a/mdbook-spec/src/rules.rs +++ b/mdbook-spec/src/rules.rs @@ -1,11 +1,13 @@ //! Handling for rule identifiers. +use crate::test_links::RuleToTests; use crate::Spec; use mdbook::book::Book; use mdbook::BookItem; use once_cell::sync::Lazy; use regex::{Captures, Regex}; use std::collections::{BTreeMap, HashSet}; +use std::fmt::Write; use std::path::PathBuf; /// The Regex for rules like `r[foo]`. @@ -76,13 +78,35 @@ impl Spec { /// Converts lines that start with `r[…]` into a "rule" which has special /// styling and can be linked to. - pub fn render_rule_definitions(&self, content: &str) -> String { + pub fn render_rule_definitions(&self, content: &str, tests: &RuleToTests) -> String { RULE_RE .replace_all(content, |caps: &Captures<'_>| { let rule_id = &caps[1]; + let mut test_html = String::new(); + if let Some(tests) = tests.get(rule_id) { + test_html = format!( + "\n\ +     \ + Tests\n\ +
\n\ + Tests with this rule: +
    "); + for test in tests { + writeln!( + test_html, + "
  • {test_path}
    • ", + test_path = test.path, + git_ref = self.git_ref + ) + .unwrap(); + } + + test_html.push_str("
"); + } format!( "
\ [{rule_id}]\ + {test_html}\
\n" ) }) diff --git a/mdbook-spec/src/test_links.rs b/mdbook-spec/src/test_links.rs new file mode 100644 index 000000000..8f847d58c --- /dev/null +++ b/mdbook-spec/src/test_links.rs @@ -0,0 +1,203 @@ +//! Handling for linking tests in rust's testsuite to rule identifiers. + +use crate::{Rules, Spec}; +use mdbook::book::{Book, BookItem}; +use std::collections::HashMap; +use std::fmt::Write; +use std::path::PathBuf; +use walkdir::WalkDir; + +/// Mapping of rule identifier to the tests that include that identifier. +pub type RuleToTests = HashMap>; +/// A test in rustc's test suite. +pub struct Test { + pub path: String, +} + +const TABLE_START: &str = " + + + + + + + + +"; + +/// Generates an HTML table summarizing the coverage of the testsuite. +pub fn make_summary_table(book: &Book, tests: &RuleToTests, rules: &Rules) -> String { + let ch_to_rules = invert_rule_map(rules); + + let mut table = String::from(TABLE_START); + let mut total_rules = 0; + let mut total_tests = 0; + let mut total_uncovered = 0; + + for (item_index, item) in book.iter().enumerate() { + let BookItem::Chapter(ch) = item else { + continue; + }; + let Some(ch_path) = &ch.path else { + continue; + }; + let level = ch + .number + .as_ref() + .map(|ch| ch.len() - 1) + .unwrap_or_default() as u32; + // Note: This path assumes that the summary chapter is in the root of + // the book. If instead it is in a subdirectory, then this needs to + // include relative `../` as needed. + let html_path = ch_path + .with_extension("html") + .to_str() + .unwrap() + .replace('\\', "/"); + let number = ch + .number + .as_ref() + .map(|n| n.to_string()) + .unwrap_or_default(); + let mut num_rules = 0; + let mut num_tests_str = String::from(""); + let mut uncovered_str = String::from(""); + let mut coverage_str = String::from(""); + if let Some(rules) = ch_to_rules.get(ch_path) { + num_rules = rules.len(); + total_rules += num_rules; + let num_tests = rules + .iter() + .map(|rule| tests.get(rule).map(|ts| ts.len()).unwrap_or_default()) + .sum::(); + total_tests += num_tests; + num_tests_str = num_tests.to_string(); + let uncovered_rules: Vec<_> = rules + .iter() + .filter(|rule| !tests.contains_key(rule.as_str())) + .collect(); + let uncovered = uncovered_rules.len(); + total_uncovered += uncovered; + coverage_str = fmt_pct(uncovered, num_rules); + if uncovered == 0 { + uncovered_str = String::from("0"); + } else { + uncovered_str = format!( + "
\n\ + \ + {uncovered}\n\ +
\n\ + Uncovered rules +
    "); + for uncovered_rule in uncovered_rules { + writeln!( + uncovered_str, + "
  • {uncovered_rule}
    • " + ) + .unwrap(); + } + uncovered_str.push_str("
"); + } + } + let indent = " ".repeat(level as usize * 6); + + writeln!( + table, + "\n\ + \n\ + \n\ + \n\ + \n\ + \n\ + ", + name = ch.name, + ) + .unwrap(); + } + + let total_coverage = fmt_pct(total_uncovered, total_rules); + writeln!( + table, + "\n\ + \n\ + \n\ + \n\ + \n\ + \n\ + " + ) + .unwrap(); + table.push_str("
RulesTestsUncovered RulesCoverage
{indent}{number} {name}{num_rules}{num_tests_str}{uncovered_str}{coverage_str}
Total:{total_rules}{total_tests}{total_uncovered}{total_coverage}
\n"); + table +} + +/// Formats a float as a percentage string. +fn fmt_pct(uncovered: usize, total: usize) -> String { + let pct = ((total - uncovered) as f32 / total as f32) * 100.0; + // Round up to tenths of a percent. + let x = (pct * 10.0).ceil() / 10.0; + format!("{x:.1}%") +} + +/// Inverts the rule map so that it is chapter path to set of rules in that +/// chapter. +fn invert_rule_map(rules: &Rules) -> HashMap> { + let mut map: HashMap> = HashMap::new(); + for (rule, (_, path)) in &rules.def_paths { + map.entry(path.clone()).or_default().push(rule.clone()); + } + for value in map.values_mut() { + value.sort(); + } + map +} + +impl Spec { + /// Scans all tests in rust-lang/rust, and creates a mapping of a rule + /// identifier to the set of tests that include that identifier. + pub fn collect_tests(&self, rules: &Rules) -> RuleToTests { + let mut map = HashMap::new(); + let Some(rust_root) = &self.rust_root else { + return map; + }; + for entry in WalkDir::new(rust_root.join("tests")) { + let entry = entry.unwrap(); + let path = entry.path(); + let relative = path.strip_prefix(rust_root).unwrap_or_else(|_| { + panic!("expected root {rust_root:?} to be a prefix of {path:?}") + }); + if path.extension().unwrap_or_default() == "rs" { + let contents = std::fs::read_to_string(path).unwrap(); + for line in contents.lines() { + if let Some(id) = line.strip_prefix("//@ reference: ") { + if rules.interior_prefixes.contains(id) { + let instead: Vec<_> = rules + .def_paths + .keys() + .filter(|key| key.starts_with(&format!("{id}."))) + .collect(); + eprintln!( + "info: Interior prefix rule {id} found in {path:?}\n \ + Tests should not be annotated with prefixed rule names.\n \ + Use the rules from {instead:?} instead." + ); + } else if !rules.def_paths.contains_key(id) { + eprintln!( + "info: Orphaned rule identifier {id} found in {path:?}\n \ + Please update the test to use an existing rule name." + ); + } + let test = Test { + path: relative.to_str().unwrap().replace('\\', "/"), + }; + map.entry(id.to_string()).or_default().push(test); + } + } + } + } + for tests in map.values_mut() { + tests.sort_by(|a, b| a.path.cmp(&b.path)); + } + map + } +} diff --git a/src/SUMMARY.md b/src/SUMMARY.md index 2b17bf45d..91f343b8d 100644 --- a/src/SUMMARY.md +++ b/src/SUMMARY.md @@ -132,4 +132,5 @@ - [Appendices](appendices.md) - [Macro Follow-Set Ambiguity Formal Specification](macro-ambiguity.md) - [Influences](influences.md) + - [Test summary](test-summary.md) - [Glossary](glossary.md) diff --git a/src/test-summary.md b/src/test-summary.md new file mode 100644 index 000000000..e4e3e7491 --- /dev/null +++ b/src/test-summary.md @@ -0,0 +1,5 @@ +# Test summary + +The following is a summary of the total tests that are linked to individual rule identifiers within the reference. + +{{summary-table}} diff --git a/theme/reference.css b/theme/reference.css index cbc7aca8a..58be91816 100644 --- a/theme/reference.css +++ b/theme/reference.css @@ -200,3 +200,33 @@ dfn { .history > blockquote { background: #f7c0eb; } + +/* Provides a anchor container for positioning popups. */ +.popup-container { + position: relative; +} +/* In the test summary page, a convenience class for toggling visibility. */ +.popup-hidden { + display: none; +} +/* In the test summary page, the styling for the uncovered rule popup. */ +.uncovered-rules-popup { + position: absolute; + left: -250px; + width: 400px; + background: var(--bg); + border-radius: 4px; + border: 1px solid; + z-index: 1000; + padding: 1rem; +} + +/* The popup that shows when viewing tests for a specific rule. */ +.tests-popup { + color: var(--fg); + background: var(--bg); + border-radius: 4px; + border: 1px solid; + z-index: 1000; + padding: 1rem; +} diff --git a/theme/reference.js b/theme/reference.js new file mode 100644 index 000000000..44a237034 --- /dev/null +++ b/theme/reference.js @@ -0,0 +1,24 @@ +/* On the test summary page, toggles the popup for the uncovered tests. */ +function spec_toggle_uncovered(item_index) { + let el = document.getElementById(`uncovered-${item_index}`); + const currently_hidden = el.classList.contains('popup-hidden'); + const all = document.querySelectorAll('.uncovered-rules-popup'); + all.forEach(element => { + element.classList.add('popup-hidden'); + }); + if (currently_hidden) { + el.classList.remove('popup-hidden'); + } +} + +function spec_toggle_tests(rule_id) { + let el = document.getElementById(`tests-${rule_id}`); + const currently_hidden = el.classList.contains('popup-hidden'); + const all = document.querySelectorAll('.tests-popup'); + all.forEach(element => { + element.classList.add('popup-hidden'); + }); + if (currently_hidden) { + el.classList.remove('popup-hidden'); + } +} From d5c6fb14e7143ff1be454828dfa4b7642bda985a Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Wed, 9 Oct 2024 19:10:11 -0700 Subject: [PATCH 08/10] Link the appendix --- docs/authoring.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/authoring.md b/docs/authoring.md index fa32fc281..5c068c6b2 100644 --- a/docs/authoring.md +++ b/docs/authoring.md @@ -101,7 +101,7 @@ When assigning rules to new paragraphs, or when modifying rule names, use the fo #### Test rule annotations -Tests in can be linked to rules in the reference. The rule will include a link to the tests, and there is also an appendix which tracks how the rules are currently linked. +Tests in can be linked to rules in the reference. The rule will include a link to the tests, and there is also an [appendix] which tracks how the rules are currently linked. Tests in the `tests` directory can be annotated with the `//@ reference: x.y.z` header to link it to a rule. The header can be specified multiple times if a single file covers multiple rules. @@ -111,6 +111,8 @@ Prefixed rule names should not be used in tests. That is, do not use something l We are not expecting 100% coverage at any time. Although it would be nice, it is unrealistic due to the sequence things are developed, and resources available. +[appendix]: https://doc.rust-lang.org/nightly/reference/test-summary.html + ### Standard library links You should link to the standard library without specifying a URL in a fashion similar to [rustdoc intra-doc links][intra]. Some examples: From 603ab3d00a2f38ac9d8d0deb6cade8f3c2315365 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Wed, 9 Oct 2024 19:14:08 -0700 Subject: [PATCH 09/10] Add a little clarity around who is responsible for what --- docs/authoring.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/authoring.md b/docs/authoring.md index 5c068c6b2..29a476c01 100644 --- a/docs/authoring.md +++ b/docs/authoring.md @@ -105,7 +105,9 @@ Tests in can be linked to rules in the refer Tests in the `tests` directory can be annotated with the `//@ reference: x.y.z` header to link it to a rule. The header can be specified multiple times if a single file covers multiple rules. -You *should* when possible make sure every rule has a test associated with it. This is beneficial for reviewers to see the behavior and readers who may want to see examples of particular behaviors. When adding new rules, you should wait until the reference side is approved before submitting a PR to `rust-lang/rust` (to avoid churn if we decide on different names). +Compiler developers are not expected to add `reference` annotations to tests. However, if they do want to help, their cooperation is very welcome. Reference authors and editors are responsible for making sure every rule has a test associated with it. + +The tests are beneficial for reviewers to see the behavior of a rule. It is also a benefit to readers who may want to see examples of particular behaviors. When adding new rules, you should wait until the reference side is approved before submitting a PR to `rust-lang/rust` (to avoid churn if we decide on different names). Prefixed rule names should not be used in tests. That is, do not use something like `asm.rules` when there are specific rules like `asm.rules.reg-not-input`. From 4d83b0b15b08faabed2c38883208fde6f51aefb8 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sat, 12 Oct 2024 08:22:16 -0700 Subject: [PATCH 10/10] CI: Update preview job to include test links --- .github/workflows/main.yml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 0c2b9b884..718b54f0c 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -109,6 +109,11 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@master + - name: Checkout rust-lang/rust + uses: actions/checkout@master + with: + repository: rust-lang/rust + path: rust - name: Update rustup run: rustup self update - name: Install Rust @@ -123,7 +128,8 @@ jobs: echo "$(pwd)/bin" >> $GITHUB_PATH - name: Build the book env: - SPEC_RELATIVE: 0 + SPEC_RELATIVE: 0 + SPEC_RUST_ROOT: ${{ github.workspace }}/rust run: mdbook build --dest-dir dist/preview-${{ github.event.pull_request.number }} - name: Upload artifact uses: actions/upload-artifact@v4