From 54467ca716a5dbe815f041e2a0cfb18d0b214196 Mon Sep 17 00:00:00 2001 From: Elliot Bobrow Date: Mon, 14 Jun 2021 12:23:33 -0700 Subject: [PATCH] check for unbalanced tick pairs in doc-markdown --- clippy_lints/src/doc.rs | 54 ++++++++++++++++++++++------ clippy_lints/src/if_let_mutex.rs | 2 +- tests/ui/doc.rs | 17 +++++++-- tests/ui/doc.stderr | 62 +++++++++++++++++--------------- 4 files changed, 92 insertions(+), 43 deletions(-) diff --git a/clippy_lints/src/doc.rs b/clippy_lints/src/doc.rs index e67ec4e06c54..eaf9771c6897 100644 --- a/clippy_lints/src/doc.rs +++ b/clippy_lints/src/doc.rs @@ -1,4 +1,5 @@ use clippy_utils::diagnostics::{span_lint, span_lint_and_note}; +use clippy_utils::source::first_line_of_span; use clippy_utils::ty::{implements_trait, is_type_diagnostic_item}; use clippy_utils::{is_entrypoint_fn, is_expn_of, match_panic_def_id, method_chain_args, return_ty}; use if_chain::if_chain; @@ -37,7 +38,7 @@ declare_clippy_lint! { /// consider that. /// /// **Known problems:** Lots of bad docs won’t be fixed, what the lint checks - /// for is limited, and there are still false positives. + /// for is limited, and there are still false positives. HTML is not linted. /// /// In addition, when writing documentation comments, including `[]` brackets /// inside a link text would trip the parser. Therfore, documenting link with @@ -469,11 +470,11 @@ fn check_doc<'a, Events: Iterator, Range DocHeaders { // true if a safety header was found - use pulldown_cmark::CodeBlockKind; use pulldown_cmark::Event::{ Code, End, FootnoteReference, HardBreak, Html, Rule, SoftBreak, Start, TaskListMarker, Text, }; - use pulldown_cmark::Tag::{CodeBlock, Heading, Link}; + use pulldown_cmark::Tag::{CodeBlock, Heading, Link, Paragraph}; + use pulldown_cmark::{CodeBlockKind, CowStr}; let mut headers = DocHeaders { safety: false, @@ -485,6 +486,9 @@ fn check_doc<'a, Events: Iterator, Range, Span)> = Vec::new(); + let mut paragraph_span = spans[0].1; // First line for (event, range) in events { match event { Start(CodeBlock(ref kind)) => { @@ -512,24 +516,45 @@ fn check_doc<'a, Events: Iterator, Range in_link = None, Start(Heading(_)) => in_heading = true, End(Heading(_)) => in_heading = false, + Start(Paragraph) => { + ticks_unbalanced = false; + let (_, span) = get_current_span(spans, range.start); + paragraph_span = first_line_of_span(cx, span); + }, + End(Paragraph) => { + if ticks_unbalanced { + span_lint( + cx, + DOC_MARKDOWN, + paragraph_span, + "backticks are unbalanced; one may be missing a pair", + ); + } else { + for (text, span) in text_to_check { + check_text(cx, valid_idents, &text, span); + } + } + text_to_check = Vec::new(); + }, Start(_tag) | End(_tag) => (), // We don't care about other tags Html(_html) => (), // HTML is weird, just ignore it SoftBreak | HardBreak | TaskListMarker(_) | Code(_) | Rule => (), FootnoteReference(text) | Text(text) => { - if Some(&text) == in_link.as_ref() { + let (begin, span) = get_current_span(spans, range.start); + paragraph_span = paragraph_span.with_hi(span.hi()); + if Some(&text) == in_link.as_ref() || ticks_unbalanced { // Probably a link of the form `` // Which are represented as a link to "http://example.com" with // text "http://example.com" by pulldown-cmark continue; } + if text.contains('`') && !in_code { + ticks_unbalanced = true; + continue; + } headers.safety |= in_heading && text.trim() == "Safety"; headers.errors |= in_heading && text.trim() == "Errors"; headers.panics |= in_heading && text.trim() == "Panics"; - let index = match spans.binary_search_by(|c| c.0.cmp(&range.start)) { - Ok(o) => o, - Err(e) => e - 1, - }; - let (begin, span) = spans[index]; if in_code { if is_rust { let edition = edition.unwrap_or_else(|| cx.tcx.sess.edition()); @@ -538,8 +563,7 @@ fn check_doc<'a, Events: Iterator, Range, Range (usize, Span) { + let index = match spans.binary_search_by(|c| c.0.cmp(&idx)) { + Ok(o) => o, + Err(e) => e - 1, + }; + spans[index] +} + fn check_code(cx: &LateContext<'_>, text: &str, edition: Edition, span: Span) { fn has_needless_main(code: &str, edition: Edition) -> bool { rustc_driver::catch_fatal_errors(|| { diff --git a/clippy_lints/src/if_let_mutex.rs b/clippy_lints/src/if_let_mutex.rs index f661f7ede821..5403d76ea30c 100644 --- a/clippy_lints/src/if_let_mutex.rs +++ b/clippy_lints/src/if_let_mutex.rs @@ -83,7 +83,7 @@ impl<'tcx> LateLintPass<'tcx> for IfLetMutex { } } -/// Checks if `Mutex::lock` is called in the `if let _ = expr. +/// Checks if `Mutex::lock` is called in the `if let` expr. pub struct OppVisitor<'a, 'tcx> { mutex_lock_called: bool, found_mutex: Option<&'tcx Expr<'tcx>>, diff --git a/tests/ui/doc.rs b/tests/ui/doc.rs index 8afef6b23d47..1b9f5ddde66e 100644 --- a/tests/ui/doc.rs +++ b/tests/ui/doc.rs @@ -96,8 +96,6 @@ fn main() { /// ## CamelCaseThing /// Talks about `CamelCaseThing`. Titles should be ignored; see issue #897. /// -/// # CamelCaseThing -/// /// Not a title #897 CamelCaseThing /// be_sure_we_got_to_the_end_of_it fn issue897() { @@ -185,7 +183,6 @@ fn issue_1920() {} /// Ok: /// -/// Not ok: http://www.unicode.org /// Not ok: https://www.unicode.org /// Not ok: http://www.unicode.org/ /// Not ok: http://www.unicode.org/reports/tr9/#Reordering_Resolved_Levels @@ -203,6 +200,20 @@ fn issue_2343() {} /// __|_ _|__||_| fn pulldown_cmark_crash() {} +/// This has `unbalanced_tick marks so it should_not_lint elsewhere. +fn issue_6753() {} + +/// This paragraph has `unbalanced_tick marks and should throw an error. +/// +/// This paragraph is fine and should_be linted normally. +fn unbalanced_ticks() {} + +/// ``` +/// // Unbalanced tick mark in code block shouldn't warn: +/// ` +/// ``` +fn unbalanced_ticks_in_code() {} + // issue #7033 - const_evaluatable_checked ICE struct S where [(); N.checked_next_power_of_two().unwrap()]: { diff --git a/tests/ui/doc.stderr b/tests/ui/doc.stderr index 7eab8a85f093..f931d41c14ad 100644 --- a/tests/ui/doc.stderr +++ b/tests/ui/doc.stderr @@ -85,106 +85,112 @@ LL | /// ## CamelCaseThing | ^^^^^^^^^^^^^^ error: you should put `CamelCaseThing` between ticks in the documentation - --> $DIR/doc.rs:99:7 - | -LL | /// # CamelCaseThing - | ^^^^^^^^^^^^^^ - -error: you should put `CamelCaseThing` between ticks in the documentation - --> $DIR/doc.rs:101:22 + --> $DIR/doc.rs:99:22 | LL | /// Not a title #897 CamelCaseThing | ^^^^^^^^^^^^^^ error: you should put `be_sure_we_got_to_the_end_of_it` between ticks in the documentation - --> $DIR/doc.rs:102:5 + --> $DIR/doc.rs:100:5 | LL | /// be_sure_we_got_to_the_end_of_it | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: you should put `be_sure_we_got_to_the_end_of_it` between ticks in the documentation - --> $DIR/doc.rs:109:5 + --> $DIR/doc.rs:107:5 | LL | /// be_sure_we_got_to_the_end_of_it | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: you should put `be_sure_we_got_to_the_end_of_it` between ticks in the documentation - --> $DIR/doc.rs:122:5 + --> $DIR/doc.rs:120:5 | LL | /// be_sure_we_got_to_the_end_of_it | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: you should put `FooBar` between ticks in the documentation - --> $DIR/doc.rs:133:43 + --> $DIR/doc.rs:131:43 | LL | /** E.g., serialization of an empty list: FooBar | ^^^^^^ error: you should put `BarQuz` between ticks in the documentation - --> $DIR/doc.rs:138:5 + --> $DIR/doc.rs:136:5 | LL | And BarQuz too. | ^^^^^^ error: you should put `be_sure_we_got_to_the_end_of_it` between ticks in the documentation - --> $DIR/doc.rs:139:1 + --> $DIR/doc.rs:137:1 | LL | be_sure_we_got_to_the_end_of_it | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: you should put `FooBar` between ticks in the documentation - --> $DIR/doc.rs:144:43 + --> $DIR/doc.rs:142:43 | LL | /** E.g., serialization of an empty list: FooBar | ^^^^^^ error: you should put `BarQuz` between ticks in the documentation - --> $DIR/doc.rs:149:5 + --> $DIR/doc.rs:147:5 | LL | And BarQuz too. | ^^^^^^ error: you should put `be_sure_we_got_to_the_end_of_it` between ticks in the documentation - --> $DIR/doc.rs:150:1 + --> $DIR/doc.rs:148:1 | LL | be_sure_we_got_to_the_end_of_it | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: you should put `be_sure_we_got_to_the_end_of_it` between ticks in the documentation - --> $DIR/doc.rs:161:5 + --> $DIR/doc.rs:159:5 | LL | /// be_sure_we_got_to_the_end_of_it | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: you should put bare URLs between `<`/`>` or make a proper Markdown link - --> $DIR/doc.rs:188:13 - | -LL | /// Not ok: http://www.unicode.org - | ^^^^^^^^^^^^^^^^^^^^^^ - -error: you should put bare URLs between `<`/`>` or make a proper Markdown link - --> $DIR/doc.rs:189:13 + --> $DIR/doc.rs:186:13 | LL | /// Not ok: https://www.unicode.org | ^^^^^^^^^^^^^^^^^^^^^^^ error: you should put bare URLs between `<`/`>` or make a proper Markdown link - --> $DIR/doc.rs:190:13 + --> $DIR/doc.rs:187:13 | LL | /// Not ok: http://www.unicode.org/ | ^^^^^^^^^^^^^^^^^^^^^^ error: you should put bare URLs between `<`/`>` or make a proper Markdown link - --> $DIR/doc.rs:191:13 + --> $DIR/doc.rs:188:13 | LL | /// Not ok: http://www.unicode.org/reports/tr9/#Reordering_Resolved_Levels | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: you should put `mycrate::Collection` between ticks in the documentation - --> $DIR/doc.rs:194:22 + --> $DIR/doc.rs:191:22 | LL | /// An iterator over mycrate::Collection's values. | ^^^^^^^^^^^^^^^^^^^ -error: aborting due to 31 previous errors +error: backticks are unbalanced; one may be missing a pair + --> $DIR/doc.rs:203:1 + | +LL | /// This has `unbalanced_tick marks so it should_not_lint elsewhere. + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: backticks are unbalanced; one may be missing a pair + --> $DIR/doc.rs:206:1 + | +LL | /// This paragraph has `unbalanced_tick marks and should throw an error. + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: you should put `should_be` between ticks in the documentation + --> $DIR/doc.rs:208:32 + | +LL | /// This paragraph is fine and should_be linted normally. + | ^^^^^^^^^ + +error: aborting due to 32 previous errors