Skip to content
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

Compare formatted and unformatted ASTs during formatter tests #8624

Merged
merged 2 commits into from
Nov 13, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions crates/ruff_python_ast/src/comparable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1480,3 +1480,44 @@ impl<'a> From<&'a ast::Stmt> for ComparableStmt<'a> {
}
}
}

#[derive(Debug, PartialEq, Eq, Hash)]
pub enum ComparableMod<'a> {
Module(ComparableModModule<'a>),
Expression(ComparableModExpression<'a>),
}

#[derive(Debug, PartialEq, Eq, Hash)]
pub struct ComparableModModule<'a> {
body: Vec<ComparableStmt<'a>>,
}

#[derive(Debug, PartialEq, Eq, Hash)]
pub struct ComparableModExpression<'a> {
body: Box<ComparableExpr<'a>>,
}

impl<'a> From<&'a ast::Mod> for ComparableMod<'a> {
fn from(mod_: &'a ast::Mod) -> Self {
match mod_ {
ast::Mod::Module(module) => Self::Module(module.into()),
ast::Mod::Expression(expr) => Self::Expression(expr.into()),
}
}
}

impl<'a> From<&'a ast::ModModule> for ComparableModModule<'a> {
fn from(module: &'a ast::ModModule) -> Self {
Self {
body: module.body.iter().map(Into::into).collect(),
}
}
}

impl<'a> From<&'a ast::ModExpression> for ComparableModExpression<'a> {
fn from(expr: &'a ast::ModExpression) -> Self {
Self {
body: (&expr.body).into(),
}
}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(These aren't necessary, but I made the changes before learning that we need to normalize the AST, figured they're worth having anyway.)

85 changes: 67 additions & 18 deletions crates/ruff_python_formatter/tests/fixtures.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,17 @@
use ruff_formatter::FormatOptions;
use ruff_python_formatter::{format_module_source, PreviewMode, PyFormatOptions};
use similar::TextDiff;
use std::fmt::{Formatter, Write};
use std::io::BufReader;
use std::path::Path;
use std::{fmt, fs};

use similar::TextDiff;

use normalized_ast::NormalizedMod;
use ruff_formatter::FormatOptions;
use ruff_python_formatter::{format_module_source, PreviewMode, PyFormatOptions};
use ruff_python_parser::{parse, AsMode};

mod normalized_ast;

#[test]
fn black_compatibility() {
let test_file = |input_path: &Path| {
Expand Down Expand Up @@ -33,6 +39,7 @@ fn black_compatibility() {

let formatted_code = printed.as_code();

ensure_unchanged_ast(&content, formatted_code, &options, input_path);
ensure_stability_when_formatting_twice(formatted_code, options, input_path);

if formatted_code == expected_output {
Expand Down Expand Up @@ -111,6 +118,7 @@ fn format() {
format_module_source(&content, options.clone()).expect("Formatting to succeed");
let formatted_code = printed.as_code();

ensure_unchanged_ast(&content, formatted_code, &options, input_path);
ensure_stability_when_formatting_twice(formatted_code, options.clone(), input_path);

let mut snapshot = format!("## Input\n{}", CodeFrame::new("python", &content));
Expand All @@ -128,6 +136,7 @@ fn format() {
format_module_source(&content, options.clone()).expect("Formatting to succeed");
let formatted_code = printed.as_code();

ensure_unchanged_ast(&content, formatted_code, &options, input_path);
ensure_stability_when_formatting_twice(formatted_code, options.clone(), input_path);

writeln!(
Expand All @@ -140,29 +149,20 @@ fn format() {
.unwrap();
}
} else {
let printed =
format_module_source(&content, options.clone()).expect("Formatting to succeed");
let formatted = printed.as_code();

ensure_stability_when_formatting_twice(formatted, options.clone(), input_path);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not clear to me why we're doing this, we do this exact thing immediately before the if-else.

// We want to capture the differences in the preview style in our fixtures
let options_preview = options.with_preview(PreviewMode::Enabled);
let printed_preview = format_module_source(&content, options_preview.clone())
.expect("Formatting to succeed");
let formatted_preview = printed_preview.as_code();

ensure_stability_when_formatting_twice(
formatted_preview,
options_preview.clone(),
input_path,
);
ensure_unchanged_ast(&content, formatted_preview, &options_preview, input_path);
ensure_stability_when_formatting_twice(formatted_preview, options_preview, input_path);

if formatted == formatted_preview {
if formatted_code == formatted_preview {
writeln!(
snapshot,
"## Output\n{}",
CodeFrame::new("python", &formatted)
CodeFrame::new("python", &formatted_code)
)
.unwrap();
} else {
Expand All @@ -171,10 +171,10 @@ fn format() {
writeln!(
snapshot,
"## Output\n{}\n## Preview changes\n{}",
CodeFrame::new("python", &formatted),
CodeFrame::new("python", &formatted_code),
CodeFrame::new(
"diff",
TextDiff::from_lines(formatted, formatted_preview)
TextDiff::from_lines(formatted_code, formatted_preview)
.unified_diff()
.header("Stable", "Preview")
)
Expand Down Expand Up @@ -239,6 +239,55 @@ Formatted twice:
}
}

/// Ensure that formatting doesn't change the AST.
///
/// Like Black, there are a few exceptions to this "invariant" which are encoded in
/// [`NormalizedMod`] and related structs. Namely, formatting can change indentation within strings,
/// and can also flatten tuples within `del` statements.
fn ensure_unchanged_ast(
unformatted_code: &str,
formatted_code: &str,
options: &PyFormatOptions,
input_path: &Path,
) {
let source_type = options.source_type();

// Parse the unformatted code.
let unformatted_ast = parse(
unformatted_code,
source_type.as_mode(),
&input_path.to_string_lossy(),
)
.expect("Unformatted code to be valid syntax");
let unformatted_ast = NormalizedMod::from(&unformatted_ast);

// Parse the formatted code.
let formatted_ast = parse(
formatted_code,
source_type.as_mode(),
&input_path.to_string_lossy(),
)
.expect("Formatted code to be valid syntax");
let formatted_ast = NormalizedMod::from(&formatted_ast);

if formatted_ast != unformatted_ast {
let diff = TextDiff::from_lines(
&format!("{unformatted_ast:#?}"),
&format!("{formatted_ast:#?}"),
)
.unified_diff()
.header("Unformatted", "Formatted")
.to_string();
panic!(
r#"Reformatting the unformatted code of {} resulted in AST changes.
---
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
r#"Reformatting the unformatted code of {} resulted in AST changes.
r#"Reformatting the unformatted code of {} resulted in AST changes. Please open an issue at https://github.com/astral-sh/ruff/issues/new with your configuration and the failing code snippet

or something along the lines

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine to omit since this is in tests only.

{diff}
"#,
input_path.display(),
);
}
}

struct Header<'a> {
title: &'a str,
}
Expand Down
Loading
Loading