Unify OldDiagnostic and Message (#18391)

Summary
--

This PR unifies the remaining differences between `OldDiagnostic` and
`Message` (`OldDiagnostic` was only missing an optional `noqa_offset`
field) and
replaces `Message` with `OldDiagnostic`.

The biggest functional difference is that the combined `OldDiagnostic`
kind no
longer implements `AsRule` for an infallible conversion to `Rule`. This
was
pretty easy to work around with `is_some_and` and `is_none_or` in the
few places
it was needed. In `LintContext::report_diagnostic_if_enabled` we can
just use
the new `Violation::rule` method, which takes care of most cases.

Most of the interesting changes are in [this
range](https://github.com/astral-sh/ruff/pull/18391/files/8156992540f4e81f914a9534e99241b1e14382a1)
before I started renaming.

Test Plan
--

Existing tests

Future Work
--

I think it's time to start shifting some of these fields to the new
`Diagnostic`
kind. I believe we want `Fix` for sure, but I'm less sure about the
others. We
may want to keep a thin wrapper type here anyway to implement a `rule`
method,
so we could leave some of these fields on that too.

Author: ntBre

crates/ruff/src/cache.rs

@@ -19,7 +19,7 @@ use tempfile::NamedTempFile;
 
 use ruff_cache::{CacheKey, CacheKeyHasher};
 use ruff_diagnostics::Fix;
-use ruff_linter::message::Message;
+use ruff_linter::message::OldDiagnostic;
 use ruff_linter::package::PackageRoot;
 use ruff_linter::{VERSION, warn_user};
 use ruff_macros::CacheKey;
@@ -341,16 +341,16 @@ impl FileCache {
     /// Convert the file cache into `Diagnostics`, using `path` as file name.
     pub(crate) fn to_diagnostics(&self, path: &Path) -> Option<Diagnostics> {
         self.data.lint.as_ref().map(|lint| {
-            let messages = if lint.messages.is_empty() {
+            let diagnostics = if lint.messages.is_empty() {
                 Vec::new()
             } else {
                 let file = SourceFileBuilder::new(path.to_string_lossy(), &*lint.source).finish();
                 lint.messages
                     .iter()
                     .map(|msg| {
-                        Message::diagnostic(
-                            msg.body.clone(),
-                            msg.suggestion.clone(),
+                        OldDiagnostic::lint(
+                            &msg.body,
+                            msg.suggestion.as_ref(),
                             msg.range,
                             msg.fix.clone(),
                             msg.parent,
@@ -366,7 +366,7 @@ impl FileCache {
             } else {
                 FxHashMap::default()
             };
-            Diagnostics::new(messages, notebook_indexes)
+            Diagnostics::new(diagnostics, notebook_indexes)
         })
     }
 }
@@ -427,27 +427,27 @@ pub(crate) struct LintCacheData {
 }
 
 impl LintCacheData {
-    pub(crate) fn from_messages(
-        messages: &[Message],
+    pub(crate) fn from_diagnostics(
+        diagnostics: &[OldDiagnostic],
         notebook_index: Option<NotebookIndex>,
     ) -> Self {
-        let source = if let Some(msg) = messages.first() {
+        let source = if let Some(msg) = diagnostics.first() {
             msg.source_file().source_text().to_owned()
         } else {
             String::new() // No messages, no need to keep the source!
         };
 
-        let messages = messages
+        let messages = diagnostics
             .iter()
             // Parse the kebab-case rule name into a `Rule`. This will fail for syntax errors, so
             // this also serves to filter them out, but we shouldn't be caching files with syntax
             // errors anyway.
-            .filter_map(|msg| Some((msg.name().parse().ok()?, msg)))
+            .filter_map(|msg| Some((msg.noqa_code().and_then(|code| code.rule())?, msg)))
             .map(|(rule, msg)| {
                 // Make sure that all message use the same source file.
                 assert_eq!(
                     msg.source_file(),
-                    messages.first().unwrap().source_file(),
+                    diagnostics.first().unwrap().source_file(),
                     "message uses a different source file"
                 );
                 CacheMessage {
@@ -612,7 +612,7 @@ mod tests {
     use test_case::test_case;
 
     use ruff_cache::CACHE_DIR_NAME;
-    use ruff_linter::message::Message;
+    use ruff_linter::message::OldDiagnostic;
     use ruff_linter::package::PackageRoot;
     use ruff_linter::settings::flags;
     use ruff_linter::settings::types::UnsafeFixes;
@@ -680,7 +680,7 @@ mod tests {
                     UnsafeFixes::Enabled,
                 )
                 .unwrap();
-                if diagnostics.messages.iter().any(Message::is_syntax_error) {
+                if diagnostics.inner.iter().any(OldDiagnostic::is_syntax_error) {
                     parse_errors.push(path.clone());
                 }
                 paths.push(path);

crates/ruff/src/commands/check.rs

@@ -13,7 +13,6 @@ use rustc_hash::FxHashMap;
 
 use ruff_db::panic::catch_unwind;
 use ruff_linter::OldDiagnostic;
-use ruff_linter::message::Message;
 use ruff_linter::package::PackageRoot;
 use ruff_linter::registry::Rule;
 use ruff_linter::settings::types::UnsafeFixes;
@@ -130,9 +129,10 @@ pub(crate) fn check(
                         SourceFileBuilder::new(path.to_string_lossy().as_ref(), "").finish();
 
                     Diagnostics::new(
-                        vec![Message::from_diagnostic(
-                            OldDiagnostic::new(IOError { message }, TextRange::default(), &dummy),
-                            None,
+                        vec![OldDiagnostic::new(
+                            IOError { message },
+                            TextRange::default(),
+                            &dummy,
                         )],
                         FxHashMap::default(),
                     )
@@ -166,7 +166,7 @@ pub(crate) fn check(
             |a, b| (a.0 + b.0, a.1 + b.1),
         );
 
-    all_diagnostics.messages.sort();
+    all_diagnostics.inner.sort();
 
     // Store the caches.
     caches.persist()?;
@@ -283,7 +283,7 @@ mod test {
             .with_show_fix_status(true)
             .emit(
                 &mut output,
-                &diagnostics.messages,
+                &diagnostics.inner,
                 &EmitterContext::new(&FxHashMap::default()),
             )
             .unwrap();

crates/ruff/src/commands/check_stdin.rs

@@ -52,6 +52,6 @@ pub(crate) fn check_stdin(
         noqa,
         fix_mode,
     )?;
-    diagnostics.messages.sort_unstable();
+    diagnostics.inner.sort_unstable();
     Ok(diagnostics)
 }

crates/ruff/src/diagnostics.rs

@@ -15,7 +15,6 @@ use rustc_hash::FxHashMap;
 use ruff_linter::OldDiagnostic;
 use ruff_linter::codes::Rule;
 use ruff_linter::linter::{FixTable, FixerResult, LinterResult, ParseSource, lint_fix, lint_only};
-use ruff_linter::message::Message;
 use ruff_linter::package::PackageRoot;
 use ruff_linter::pyproject_toml::lint_pyproject_toml;
 use ruff_linter::settings::types::UnsafeFixes;
@@ -32,18 +31,18 @@ use crate::cache::{Cache, FileCacheKey, LintCacheData};
 
 #[derive(Debug, Default, PartialEq)]
 pub(crate) struct Diagnostics {
-    pub(crate) messages: Vec<Message>,
+    pub(crate) inner: Vec<OldDiagnostic>,
     pub(crate) fixed: FixMap,
     pub(crate) notebook_indexes: FxHashMap<String, NotebookIndex>,
 }
 
 impl Diagnostics {
     pub(crate) fn new(
-        messages: Vec<Message>,
+        diagnostics: Vec<OldDiagnostic>,
         notebook_indexes: FxHashMap<String, NotebookIndex>,
     ) -> Self {
         Self {
-            messages,
+            inner: diagnostics,
             fixed: FixMap::default(),
             notebook_indexes,
         }
@@ -63,15 +62,12 @@ impl Diagnostics {
                     let name = path.map_or_else(|| "-".into(), Path::to_string_lossy);
                     let source_file = SourceFileBuilder::new(name, "").finish();
                     Self::new(
-                        vec![Message::from_diagnostic(
-                            OldDiagnostic::new(
-                                IOError {
-                                    message: err.to_string(),
-                                },
-                                TextRange::default(),
-                                &source_file,
-                            ),
-                            None,
+                        vec![OldDiagnostic::new(
+                            IOError {
+                                message: err.to_string(),
+                            },
+                            TextRange::default(),
+                            &source_file,
                         )],
                         FxHashMap::default(),
                     )
@@ -102,7 +98,11 @@ impl Diagnostics {
                 let name = path.map_or_else(|| "-".into(), Path::to_string_lossy);
                 let dummy = SourceFileBuilder::new(name, "").finish();
                 Self::new(
-                    vec![Message::syntax_error(err, TextRange::default(), dummy)],
+                    vec![OldDiagnostic::syntax_error(
+                        err,
+                        TextRange::default(),
+                        dummy,
+                    )],
                     FxHashMap::default(),
                 )
             }
@@ -121,7 +121,7 @@ impl Add for Diagnostics {
 
 impl AddAssign for Diagnostics {
     fn add_assign(&mut self, other: Self) {
-        self.messages.extend(other.messages);
+        self.inner.extend(other.inner);
         self.fixed += other.fixed;
         self.notebook_indexes.extend(other.notebook_indexes);
     }
@@ -202,7 +202,7 @@ pub(crate) fn lint_path(
                 if match fix_mode {
                     flags::FixMode::Generate => true,
                     flags::FixMode::Apply | flags::FixMode::Diff => {
-                        diagnostics.messages.is_empty() && diagnostics.fixed.is_empty()
+                        diagnostics.inner.is_empty() && diagnostics.fixed.is_empty()
                     }
                 } {
                     return Ok(diagnostics);
@@ -222,7 +222,7 @@ pub(crate) fn lint_path(
         Some(source_type) => source_type,
         None => match SourceType::from(path) {
             SourceType::Toml(TomlSourceType::Pyproject) => {
-                let messages = if settings
+                let diagnostics = if settings
                     .rules
                     .iter_enabled()
                     .any(|rule_code| rule_code.lint_source().is_pyproject_toml())
@@ -240,7 +240,7 @@ pub(crate) fn lint_path(
                     vec![]
                 };
                 return Ok(Diagnostics {
-                    messages,
+                    inner: diagnostics,
                     ..Diagnostics::default()
                 });
             }
@@ -324,7 +324,7 @@ pub(crate) fn lint_path(
         };
 
     let has_error = result.has_syntax_errors();
-    let messages = result.messages;
+    let diagnostics = result.diagnostics;
 
     if let Some((cache, relative_path, key)) = caching {
         // We don't cache parsing errors.
@@ -335,14 +335,14 @@ pub(crate) fn lint_path(
             if match fix_mode {
                 flags::FixMode::Generate => true,
                 flags::FixMode::Apply | flags::FixMode::Diff => {
-                    messages.is_empty() && fixed.is_empty()
+                    diagnostics.is_empty() && fixed.is_empty()
                 }
             } {
                 cache.update_lint(
                     relative_path.to_owned(),
                     &key,
-                    LintCacheData::from_messages(
-                        &messages,
+                    LintCacheData::from_diagnostics(
+                        &diagnostics,
                         transformed.as_ipy_notebook().map(Notebook::index).cloned(),
                     ),
                 );
@@ -357,7 +357,7 @@ pub(crate) fn lint_path(
     };
 
     Ok(Diagnostics {
-        messages,
+        inner: diagnostics,
         fixed: FixMap::from_iter([(fs::relativize_path(path), fixed)]),
         notebook_indexes,
     })
@@ -396,7 +396,7 @@ pub(crate) fn lint_stdin(
                 }
 
                 return Ok(Diagnostics {
-                    messages: lint_pyproject_toml(&source_file, &settings.linter),
+                    inner: lint_pyproject_toml(&source_file, &settings.linter),
                     fixed: FixMap::from_iter([(fs::relativize_path(path), FixTable::default())]),
                     notebook_indexes: FxHashMap::default(),
                 });
@@ -417,7 +417,7 @@ pub(crate) fn lint_stdin(
     };
 
     // Lint the inputs.
-    let (LinterResult { messages, .. }, transformed, fixed) =
+    let (LinterResult { diagnostics, .. }, transformed, fixed) =
         if matches!(fix_mode, flags::FixMode::Apply | flags::FixMode::Diff) {
             if let Ok(FixerResult {
                 result,
@@ -501,7 +501,7 @@ pub(crate) fn lint_stdin(
     };
 
     Ok(Diagnostics {
-        messages,
+        inner: diagnostics,
         fixed: FixMap::from_iter([(
             fs::relativize_path(path.unwrap_or_else(|| Path::new("-"))),
             fixed,

crates/ruff/src/lib.rs

@@ -363,7 +363,7 @@ pub fn check(args: CheckCommand, global_options: GlobalConfigArgs) -> Result<Exi
         Printer::clear_screen()?;
         printer.write_to_user("Starting linter in watch mode...\n");
 
-        let messages = commands::check::check(
+        let diagnostics = commands::check::check(
             &files,
             &pyproject_config,
             &config_arguments,
@@ -372,7 +372,7 @@ pub fn check(args: CheckCommand, global_options: GlobalConfigArgs) -> Result<Exi
             fix_mode,
             unsafe_fixes,
         )?;
-        printer.write_continuously(&mut writer, &messages, preview)?;
+        printer.write_continuously(&mut writer, &diagnostics, preview)?;
 
         // In watch mode, we may need to re-resolve the configuration.
         // TODO(charlie): Re-compute other derivative values, like the `printer`.
@@ -392,7 +392,7 @@ pub fn check(args: CheckCommand, global_options: GlobalConfigArgs) -> Result<Exi
                     Printer::clear_screen()?;
                     printer.write_to_user("File change detected...\n");
 
-                    let messages = commands::check::check(
+                    let diagnostics = commands::check::check(
                         &files,
                         &pyproject_config,
                         &config_arguments,
@@ -401,7 +401,7 @@ pub fn check(args: CheckCommand, global_options: GlobalConfigArgs) -> Result<Exi
                         fix_mode,
                         unsafe_fixes,
                     )?;
-                    printer.write_continuously(&mut writer, &messages, preview)?;
+                    printer.write_continuously(&mut writer, &diagnostics, preview)?;
                 }
                 Err(err) => return Err(err.into()),
             }
@@ -463,11 +463,11 @@ pub fn check(args: CheckCommand, global_options: GlobalConfigArgs) -> Result<Exi
                 // there are any violations, unless we're explicitly asked to exit zero on
                 // fix.
                 if cli.exit_non_zero_on_fix {
-                    if !diagnostics.fixed.is_empty() || !diagnostics.messages.is_empty() {
+                    if !diagnostics.fixed.is_empty() || !diagnostics.inner.is_empty() {
                         return Ok(ExitStatus::Failure);
                     }
                 } else {
-                    if !diagnostics.messages.is_empty() {
+                    if !diagnostics.inner.is_empty() {
                         return Ok(ExitStatus::Failure);
                     }
                 }