mirror of
				https://github.com/astral-sh/ruff.git
				synced 2025-11-04 05:34:54 +00:00 
			
		
		
		
	Combine OldDiagnostic and Diagnostic (#19053)
				
					
				
			## Summary This PR is a collaboration with @AlexWaygood from our pairing session last Friday. The main goal here is removing `ruff_linter::message::OldDiagnostic` in favor of using `ruff_db::diagnostic::Diagnostic` directly. This involved a few major steps: - Transferring the fields - Transferring the methods and trait implementations, where possible - Converting some constructor methods to free functions - Moving the `SecondaryCode` struct - Updating the method names I'm hoping that some of the methods, especially those in the `expect_ruff_*` family, won't be necessary long-term, but I avoided trying to replace them entirely for now to keep the already-large diff a bit smaller. ### Related refactors Alex and I noticed a few refactoring opportunities while looking at the code, specifically the very similar implementations for `create_parse_diagnostic`, `create_unsupported_syntax_diagnostic`, and `create_semantic_syntax_diagnostic`. We combined these into a single generic function, which I then copied into `ruff_linter::message` with some small changes and a TODO to combine them in the future. I also deleted the `DisplayParseErrorType` and `TruncateAtNewline` types for reporting parse errors. These were added in #4124, I believe to work around the error messages from LALRPOP. Removing these didn't affect any tests, so I think they were unnecessary now that we fully control the error messages from the parser. On a more minor note, I factored out some calls to the `OldDiagnostic::filename` (now `Diagnostic::expect_ruff_filename`) function to avoid repeatedly allocating `String`s in some places. ### Snapshot changes The `show_statistics_syntax_errors` integration test changed because the `OldDiagnostic::name` method used `syntax-error` instead of `invalid-syntax` like in ty. I think this (`--statistics`) is one of the only places we actually use this name for syntax errors, so I hope this is okay. An alternative is to use `syntax-error` in ty too. The other snapshot changes are from removing this code, as discussed on [Discord](1388252408):34052a1185/crates/ruff_linter/src/message/mod.rs (L128-L135)I think both of these are technically breaking changes, but they only affect syntax errors and are very narrow in scope, while also pretty substantially simplifying the refactor, so I hope they're okay to include in a patch release. ## Test plan Existing tests, with the adjustments mentioned above --------- Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
This commit is contained in:
		
							parent
							
								
									9bee8376a1
								
							
						
					
					
						commit
						77a5c5ac80
					
				
					 61 changed files with 715 additions and 772 deletions
				
			
		| 
						 | 
				
			
			@ -1,9 +1,10 @@
 | 
			
		|||
use std::io::Write;
 | 
			
		||||
 | 
			
		||||
use ruff_db::diagnostic::Diagnostic;
 | 
			
		||||
use ruff_source_file::LineColumn;
 | 
			
		||||
 | 
			
		||||
use crate::fs::relativize_path;
 | 
			
		||||
use crate::message::{Emitter, EmitterContext, OldDiagnostic};
 | 
			
		||||
use crate::message::{Emitter, EmitterContext};
 | 
			
		||||
 | 
			
		||||
/// Generate error workflow command in GitHub Actions format.
 | 
			
		||||
/// See: [GitHub documentation](https://docs.github.com/en/actions/reference/workflow-commands-for-github-actions#setting-an-error-message)
 | 
			
		||||
| 
						 | 
				
			
			@ -14,12 +15,13 @@ impl Emitter for GithubEmitter {
 | 
			
		|||
    fn emit(
 | 
			
		||||
        &mut self,
 | 
			
		||||
        writer: &mut dyn Write,
 | 
			
		||||
        diagnostics: &[OldDiagnostic],
 | 
			
		||||
        diagnostics: &[Diagnostic],
 | 
			
		||||
        context: &EmitterContext,
 | 
			
		||||
    ) -> anyhow::Result<()> {
 | 
			
		||||
        for diagnostic in diagnostics {
 | 
			
		||||
            let source_location = diagnostic.compute_start_location();
 | 
			
		||||
            let location = if context.is_notebook(&diagnostic.filename()) {
 | 
			
		||||
            let source_location = diagnostic.expect_ruff_start_location();
 | 
			
		||||
            let filename = diagnostic.expect_ruff_filename();
 | 
			
		||||
            let location = if context.is_notebook(&filename) {
 | 
			
		||||
                // We can't give a reasonable location for the structured formats,
 | 
			
		||||
                // so we show one that's clearly a fallback
 | 
			
		||||
                LineColumn::default()
 | 
			
		||||
| 
						 | 
				
			
			@ -27,7 +29,7 @@ impl Emitter for GithubEmitter {
 | 
			
		|||
                source_location
 | 
			
		||||
            };
 | 
			
		||||
 | 
			
		||||
            let end_location = diagnostic.compute_end_location();
 | 
			
		||||
            let end_location = diagnostic.expect_ruff_end_location();
 | 
			
		||||
 | 
			
		||||
            write!(
 | 
			
		||||
                writer,
 | 
			
		||||
| 
						 | 
				
			
			@ -35,7 +37,7 @@ impl Emitter for GithubEmitter {
 | 
			
		|||
                code = diagnostic
 | 
			
		||||
                    .secondary_code()
 | 
			
		||||
                    .map_or_else(String::new, |code| format!(" ({code})")),
 | 
			
		||||
                file = diagnostic.filename(),
 | 
			
		||||
                file = filename,
 | 
			
		||||
                row = source_location.line,
 | 
			
		||||
                column = source_location.column,
 | 
			
		||||
                end_row = end_location.line,
 | 
			
		||||
| 
						 | 
				
			
			@ -45,7 +47,7 @@ impl Emitter for GithubEmitter {
 | 
			
		|||
            write!(
 | 
			
		||||
                writer,
 | 
			
		||||
                "{path}:{row}:{column}:",
 | 
			
		||||
                path = relativize_path(&*diagnostic.filename()),
 | 
			
		||||
                path = relativize_path(&filename),
 | 
			
		||||
                row = location.line,
 | 
			
		||||
                column = location.column,
 | 
			
		||||
            )?;
 | 
			
		||||
| 
						 | 
				
			
			
 | 
			
		|||
		Loading…
	
	Add table
		Add a link
		
	
		Reference in a new issue