[internal] Return Messages from check_path
#16837
Conversation
Summary -- This PR updates `check_path` in the `ruff_linter` crate to return a `Vec<Message>` instead of a `Vec<Diagnostic>`. The main motivation for this is to make it easier to convert semantic syntax errors directly into `Message`s rather than `Diagnostic`s in #16106. However, this also has the benefit of keeping the preview check on unsupported syntax errors in `check_path`, as suggested in https://github.com/astral-sh/ruff/pull/16429/files#r1974748024. Test Plan -- Existing tests. I also tested the playground and server manually.
I tried to preserve `diagnostic` where `Message::as_diagnostic_message` or `Message::into_diagnostic_message` were called but otherwise convert everything to `message`.
|
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
This is a great improvement. I also think that it will make Andrew's diagnostic refactor easier because check_path. Thanks for working on this.
crates/ruff_linter/src/fix/mod.rs
Outdated
| range: edit.range(), | ||
| fix: Some(Fix::safe_edit(edit)), | ||
| parent: None, | ||
| file: SourceFileBuilder::new("<filename>", "<code>").finish(), |
There was a problem hiding this comment.
While the source text doesn't matter here, I'd still prefer to pass it in because the test here otherwise start panicking (at best) or produce nonsensical results) if apply_fixes ever starts accessing file
There was a problem hiding this comment.
Are you suggesting passing in a placeholder like "<filename>" but just requiring the caller to make that choice? It looks like we don't actually have a filename where this function is used, unlike message_from_diagnostic. We do have a Locator we could extract the source code argument from, though.
There was a problem hiding this comment.
I made this function take both a filename: &str and a source: &str to pass to the SourceFileBuilder and called the function throughout the tests module with "<filename>", locator.contents(), .... Happy to revisit it if that's not what you had in mind, though.
| fixes: applied, | ||
| source_map, | ||
| }) = fix_file(&diagnostics, &locator, unsafe_fixes) | ||
| }) = fix_file(&messages, &locator, unsafe_fixes) |
There was a problem hiding this comment.
Did this PR just open up the possibility for the parser to have fixes :)
crates/ruff_linter/src/test.rs
Outdated
| let noqa_start = diagnostic.range.start(); | ||
| diagnostic.file = source_file.clone(); | ||
| diagnostic.noqa_offset = noqa_start; |
There was a problem hiding this comment.
Do we still need the noqa offset mapping? Shouldn't diagnostic.file and diagnostic.noqa_offset already have the right file and offset?
There was a problem hiding this comment.
Ah I think you're right. Tests were failing because some of them truncate the input path to just the file name, but I think the cause ended up being somewhere else. We can actually just print all of the messages here, and the tests still pass, but I'll leave the filter on DiagnosticMessages for now.
dhruvmanila
left a comment
dhruvmanila
left a comment
There was a problem hiding this comment.
This looks great! Thanks for doing this.
Summary -- Deletes the unused `parent` field on two structs, which should make converting these to the new `Diagnostic` type a little easier. It looks like I accidentally added both of these in #16837 in an early commit that I only partially reverted, so they have never actually been used. Test Plan -- Existing tests
Summary
This PR updates
check_pathin theruff_lintercrate to return aVec<Message>instead of aVec<Diagnostic>. The main motivation for this is to make it easier to convert semantic syntax errors directly intoMessages rather thanDiagnostics in #16106. However, this also has the benefit of keeping the preview check on unsupported syntax errors incheck_path, as suggested in #16429 (comment).All of the interesting changes are in the first commit. The second commit just renames variables like
diagnosticstomessages, and the third commit is a tiny import fix.I also updated the
ExpandedMessage::locationfield name, which caused a few extra commits tidying up the playground code. I thought it was nicely symmetric withend_location, but I'm happy to revert that too.Test Plan
Existing tests. I also tested the playground and server manually.