-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[red-knot] Report line numbers in mdtest relative to the markdown file, not the test snippet #13804
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -4,10 +4,11 @@ use red_knot_python_semantic::types::check_types; | |||||||||||||||||||||||||||
use ruff_db::files::system_path_to_file; | ||||||||||||||||||||||||||||
use ruff_db::parsed::parsed_module; | ||||||||||||||||||||||||||||
use ruff_db::system::{DbWithTestSystem, SystemPathBuf}; | ||||||||||||||||||||||||||||
use ruff_source_file::OneIndexed; | ||||||||||||||||||||||||||||
use std::collections::BTreeMap; | ||||||||||||||||||||||||||||
use std::path::PathBuf; | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
type Failures = BTreeMap<SystemPathBuf, matcher::FailuresByLine>; | ||||||||||||||||||||||||||||
type Failures = BTreeMap<AbsoluteLineNumberPath, matcher::FailuresByLine>; | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
mod assertion; | ||||||||||||||||||||||||||||
mod db; | ||||||||||||||||||||||||||||
|
@@ -34,12 +35,24 @@ pub fn run(path: &PathBuf, title: &str) { | |||||||||||||||||||||||||||
any_failures = true; | ||||||||||||||||||||||||||||
println!("\n{}\n", test.name().bold().underline()); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
for (path, by_line) in failures { | ||||||||||||||||||||||||||||
for ( | ||||||||||||||||||||||||||||
AbsoluteLineNumberPath { | ||||||||||||||||||||||||||||
path, | ||||||||||||||||||||||||||||
line_number: absolute_line_number, | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
by_line, | ||||||||||||||||||||||||||||
) in failures | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
Comment on lines
+38
to
+45
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: just because you can do arbitrary unpacking in Rust doesn't mean you should ;) Here, I'd find this more readable:
Suggested change
|
||||||||||||||||||||||||||||
println!("{}", path.as_str().bold()); | ||||||||||||||||||||||||||||
for (line_number, failures) in by_line.iter() { | ||||||||||||||||||||||||||||
for failure in failures { | ||||||||||||||||||||||||||||
let absolute_line_info = format!( | ||||||||||||||||||||||||||||
"{}:{}", | ||||||||||||||||||||||||||||
title, | ||||||||||||||||||||||||||||
absolute_line_number.saturating_add(line_number.get()) | ||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||
let line_info = format!("line {line_number}:").cyan(); | ||||||||||||||||||||||||||||
println!(" {line_info} {failure}"); | ||||||||||||||||||||||||||||
println!("{absolute_line_info} {line_info} {failure}"); | ||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The indentation should come before
Suggested change
This means that the output in the terminal looks is beautifully organised like this if you have a test which has multiple Python snippets in the same Markdown test, and failures in more than one of those code blocks: I checked, and I'm still able to double-click on the Separately, I'm wondering if we need the relative line number to be reported at all, or if we could just make do with the relative line number? It feels like it just adds noise. Maybe we could just do this (diff relative to your PR branch): diff --git a/crates/red_knot_test/src/lib.rs b/crates/red_knot_test/src/lib.rs
index 691a71528..e31fad615 100644
--- a/crates/red_knot_test/src/lib.rs
+++ b/crates/red_knot_test/src/lib.rs
@@ -50,9 +50,9 @@ pub fn run(path: &PathBuf, title: &str) {
"{}:{}",
title,
absolute_line_number.saturating_add(line_number.get())
- );
- let line_info = format!("line {line_number}:").cyan();
- println!("{absolute_line_info} {line_info} {failure}");
+ )
+ .cyan();
+ println!(" {absolute_line_info} {failure}");
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find the many line numbers and file names slightly confusing... Especially because they're next to each other. But it's hard to come up with a sane grouping. The grouping by file seems reasonable but is also somewhat confusing with diagnostics that then have line numbers absolute to the entire file. And do we still need to show both line numbers? Aren't the md file line numbers sufficient?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I already asked that; I agree with you. Take a look at the final suggested diff in my comment above :-) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh shoot. Reading is hard. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also agree with removing the old embedded-relative line number entirely, in favor of just the new absolute line number. I would favor removing the grouping by embedded-file altogether in the output, and just having flat output of one failure per line, per test. It's weird to me to have it grouped by embedded file but then the line numbers be relative to the markdown file. If you find the given line in the markdown file, it'll be obvious what embedded file you are in. And so many of the embedded file names are just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That rationale makes sense to me! |
||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
println!(); | ||||||||||||||||||||||||||||
|
@@ -52,11 +65,17 @@ pub fn run(path: &PathBuf, title: &str) { | |||||||||||||||||||||||||||
assert!(!any_failures, "Some tests failed."); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
#[derive(PartialEq, PartialOrd, Eq, Ord)] | ||||||||||||||||||||||||||||
struct AbsoluteLineNumberPath { | ||||||||||||||||||||||||||||
path: SystemPathBuf, | ||||||||||||||||||||||||||||
line_number: OneIndexed, | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
fn run_test(test: &parser::MarkdownTest) -> Result<(), Failures> { | ||||||||||||||||||||||||||||
let workspace_root = SystemPathBuf::from("/src"); | ||||||||||||||||||||||||||||
let mut db = db::Db::setup(workspace_root.clone()); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
let mut system_paths = vec![]; | ||||||||||||||||||||||||||||
let mut paths: Vec<AbsoluteLineNumberPath> = Vec::with_capacity(test.files().count()); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
for file in test.files() { | ||||||||||||||||||||||||||||
assert!( | ||||||||||||||||||||||||||||
|
@@ -65,12 +84,16 @@ fn run_test(test: &parser::MarkdownTest) -> Result<(), Failures> { | |||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||
let full_path = workspace_root.join(file.path); | ||||||||||||||||||||||||||||
db.write_file(&full_path, file.code).unwrap(); | ||||||||||||||||||||||||||||
system_paths.push(full_path); | ||||||||||||||||||||||||||||
paths.push(AbsoluteLineNumberPath { | ||||||||||||||||||||||||||||
path: full_path, | ||||||||||||||||||||||||||||
line_number: file.md_line_number, | ||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
let mut failures = BTreeMap::default(); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
for path in system_paths { | ||||||||||||||||||||||||||||
for numbered_path in paths { | ||||||||||||||||||||||||||||
let path = numbered_path.path.clone(); | ||||||||||||||||||||||||||||
let file = system_path_to_file(&db, path.clone()).unwrap(); | ||||||||||||||||||||||||||||
let parsed = parsed_module(&db, file); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
|
@@ -84,7 +107,7 @@ fn run_test(test: &parser::MarkdownTest) -> Result<(), Failures> { | |||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
matcher::match_file(&db, file, check_types(&db, file)).unwrap_or_else(|line_failures| { | ||||||||||||||||||||||||||||
failures.insert(path, line_failures); | ||||||||||||||||||||||||||||
failures.insert(numbered_path, line_failures); | ||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
if failures.is_empty() { | ||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
use once_cell::sync::Lazy; | ||
use regex::{Captures, Regex}; | ||
use ruff_index::{newtype_index, IndexVec}; | ||
use ruff_source_file::OneIndexed; | ||
use rustc_hash::{FxHashMap, FxHashSet}; | ||
|
||
/// Parse the Markdown `source` as a test suite with given `title`. | ||
|
@@ -131,6 +132,7 @@ pub(crate) struct EmbeddedFile<'s> { | |
pub(crate) path: &'s str, | ||
pub(crate) lang: &'s str, | ||
pub(crate) code: &'s str, | ||
pub(crate) md_line_number: OneIndexed, | ||
} | ||
|
||
/// Matches an arbitrary amount of whitespace (including newlines), followed by a sequence of `#` | ||
|
@@ -186,6 +188,9 @@ struct Parser<'s> { | |
/// The unparsed remainder of the Markdown source. | ||
unparsed: &'s str, | ||
|
||
/// Current line number of the parser | ||
current_line_number: OneIndexed, | ||
|
||
/// Stack of ancestor sections. | ||
stack: SectionStack, | ||
|
||
|
@@ -205,6 +210,7 @@ impl<'s> Parser<'s> { | |
sections, | ||
files: IndexVec::default(), | ||
unparsed: source, | ||
current_line_number: OneIndexed::new(1).unwrap(), | ||
stack: SectionStack::new(root_section_id), | ||
current_section_files: None, | ||
} | ||
|
@@ -225,6 +231,12 @@ impl<'s> Parser<'s> { | |
} | ||
} | ||
|
||
fn increment_line_count(&mut self, captures: &Captures<'s>) { | ||
self.current_line_number = self | ||
.current_line_number | ||
.saturating_add(captures[0].lines().count()); | ||
} | ||
|
||
fn parse_impl(&mut self) -> anyhow::Result<()> { | ||
while !self.unparsed.is_empty() { | ||
if let Some(captures) = self.scan(&HEADER_RE) { | ||
|
@@ -235,6 +247,7 @@ impl<'s> Parser<'s> { | |
// ignore other Markdown syntax (paragraphs, etc) used as comments in the test | ||
if let Some(next_newline) = self.unparsed.find('\n') { | ||
(_, self.unparsed) = self.unparsed.split_at(next_newline + 1); | ||
self.current_line_number = self.current_line_number.saturating_add(1); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aren't the line numbers off after a code block (that could span multiple lines)? I think a better approach is to track the |
||
} else { | ||
break; | ||
} | ||
|
@@ -270,6 +283,8 @@ impl<'s> Parser<'s> { | |
|
||
self.current_section_files = None; | ||
|
||
self.increment_line_count(captures); | ||
|
||
Ok(()) | ||
} | ||
|
||
|
@@ -303,6 +318,7 @@ impl<'s> Parser<'s> { | |
// CODE_RE can't match without matches for 'lang' and 'code'. | ||
lang: captures.name("lang").unwrap().into(), | ||
code: captures.name("code").unwrap().into(), | ||
md_line_number: self.current_line_number, | ||
}); | ||
|
||
if let Some(current_files) = &mut self.current_section_files { | ||
|
@@ -324,6 +340,8 @@ impl<'s> Parser<'s> { | |
self.current_section_files = Some(FxHashSet::from_iter([path])); | ||
} | ||
|
||
self.increment_line_count(captures); | ||
|
||
Ok(()) | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I'm inclined to use a regular
Vec
here and instead sort it before returning it inrun_test
. Vec's are just overall simpler data structure (e.g. for iterating). I then would add themd_offset
to theFailuresByLine
data structure (do we still needline_number
or can we replace it with themd_offset
?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we remove the grouping by embedded-file path from the output altogether, as I suggested below, that also suggests simplifying this data structure.