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

Backport: Do not touch module with #![rustfmt::skip] (4297) #5094

Merged
merged 1 commit into from
Dec 8, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
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
52 changes: 41 additions & 11 deletions src/formatting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use std::io::{self, Write};
use std::time::{Duration, Instant};

use rustc_ast::ast;
use rustc_ast::AstLike;
Copy link
Member

Choose a reason for hiding this comment

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

I gather this is indeed needed otherwise the deny warnings compiler lint would've caused build failures, though curious what we need this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah we need the trait rustc_ast::AstLike to be in scope to write module.attrs(). Here's the error message you get if you remove the use statement and run cargo check

  --> src\formatting.rs:70:29
   |
70 |     if contains_skip(module.attrs()) {
   |                             ^^^^^ method not found in `&Module<'_>`
   |
   = help: items from traits can only be used if the trait is in scope
help: the following trait is implemented but not in scope; perhaps add a `use` for it:
   |
3  | use crate::rustc_ast::AstLike;
   |

use rustc_span::Span;

use self::newline_style::apply_newline_style;
Expand All @@ -15,7 +16,7 @@ use crate::issues::BadIssueSeeker;
use crate::modules::Module;
use crate::syntux::parser::{DirectoryOwnership, Parser, ParserError};
use crate::syntux::session::ParseSess;
use crate::utils::count_newlines;
use crate::utils::{contains_skip, count_newlines};
use crate::visitor::FmtVisitor;
use crate::{modules, source_file, ErrorKind, FormatReport, Input, Session};

Expand Down Expand Up @@ -58,6 +59,39 @@ impl<'b, T: Write + 'b> Session<'b, T> {
}
}

/// Determine if a module should be skipped. True if the module should be skipped, false otherwise.
fn should_skip_module<T: FormatHandler>(
config: &Config,
context: &FormatContext<'_, T>,
input_is_stdin: bool,
main_file: &FileName,
path: &FileName,
module: &Module<'_>,
) -> bool {
if contains_skip(module.attrs()) {
return true;
}

if config.skip_children() && path != main_file {
return true;
}

if !input_is_stdin && context.ignore_file(&path) {
return true;
}

if !config.format_generated_files() {
let source_file = context.parse_session.span_to_file_contents(module.span);
let src = source_file.src.as_ref().expect("SourceFile without src");

if is_generated_file(src) {
return true;
}
}

false
}

// Format an entire crate (or subset of the module tree).
fn format_project<T: FormatHandler>(
input: Input,
Expand Down Expand Up @@ -97,23 +131,19 @@ fn format_project<T: FormatHandler>(
directory_ownership.unwrap_or(DirectoryOwnership::UnownedViaBlock),
!input_is_stdin && !config.skip_children(),
)
.visit_crate(&krate)?;
.visit_crate(&krate)?
.into_iter()
.filter(|(path, module)| {
!should_skip_module(config, &context, input_is_stdin, &main_file, path, module)
})
.collect::<Vec<_>>();

timer = timer.done_parsing();

// Suppress error output if we have to do any further parsing.
context.parse_session.set_silent_emitter();

for (path, module) in files {
let source_file = context.parse_session.span_to_file_contents(module.span);
let src = source_file.src.as_ref().expect("SourceFile without src");

let should_ignore = (!input_is_stdin && context.ignore_file(&path))
|| (!config.format_generated_files() && is_generated_file(src));

if (config.skip_children() && path != main_file) || should_ignore {
continue;
}
should_emit_verbose(input_is_stdin, config, || println!("Formatting {}", path));
context.format_file(path, &module, is_macro_def)?;
}
Expand Down
21 changes: 13 additions & 8 deletions src/test/configuration_snippet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,14 +110,7 @@ impl ConfigCodeBlock {
assert!(self.code_block.is_some() && self.code_block_start.is_some());

// See if code block begins with #![rustfmt::skip].
let fmt_skip = self
.code_block
.as_ref()
.unwrap()
.lines()
.nth(0)
.unwrap_or("")
== "#![rustfmt::skip]";
let fmt_skip = self.fmt_skip();

if self.config_name.is_none() && !fmt_skip {
write_message(&format!(
Expand All @@ -138,6 +131,17 @@ impl ConfigCodeBlock {
true
}

/// True if the code block starts with #![rustfmt::skip]
fn fmt_skip(&self) -> bool {
self.code_block
.as_ref()
.unwrap()
.lines()
.nth(0)
.unwrap_or("")
== "#![rustfmt::skip]"
}

fn has_parsing_errors<T: Write>(&self, session: &Session<'_, T>) -> bool {
if session.has_parsing_errors() {
write_message(&format!(
Expand Down Expand Up @@ -251,6 +255,7 @@ fn configuration_snippet_tests() {
let blocks = get_code_blocks();
let failures = blocks
.iter()
.filter(|block| !block.fmt_skip())
Copy link
Member

Choose a reason for hiding this comment

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

Was this change related/necessary for the fix? It seems reasonable enough but wasn't sure if it was just an add on

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it was needed. The tests fail otherwise. The original PR (4297) ran into a similar issue, and here's how they chose to fix it. The test code has changed a bit since (4297) was opened, but the comment in the commit is insightful as to what's happening and why we need to skip checking these code blocks.

Let me know if you have another suggestions for fixing the test.

.map(ConfigCodeBlock::formatted_is_idempotent)
.fold(0, |acc, r| acc + (!r as u32));

Expand Down
9 changes: 9 additions & 0 deletions src/test/mod_resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,12 @@ fn out_of_line_nested_inline_within_out_of_line() {
],
);
}

#[test]
fn skip_out_of_line_nested_inline_within_out_of_line() {
// See also https://github.com/rust-lang/rustfmt/issues/5065
verify_mod_resolution(
"tests/mod-resolver/skip-files-issue-5065/main.rs",
&["tests/mod-resolver/skip-files-issue-5065/one.rs"],
);
}
13 changes: 7 additions & 6 deletions src/visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -948,12 +948,13 @@ impl<'b, 'a: 'b> FmtVisitor<'a> {

pub(crate) fn format_separate_mod(&mut self, m: &Module<'_>, end_pos: BytePos) {
self.block_indent = Indent::empty();
if self.visit_attrs(m.attrs(), ast::AttrStyle::Inner) {
self.push_skipped_with_span(m.attrs(), m.span, m.span);
} else {
self.walk_mod_items(&m.items);
self.format_missing_with_indent(end_pos);
}
let skipped = self.visit_attrs(m.attrs(), ast::AttrStyle::Inner);
assert!(
!skipped,
"Skipping module must be handled before reaching this line."
);
self.walk_mod_items(&m.items);
self.format_missing_with_indent(end_pos);
}

pub(crate) fn skip_empty_lines(&mut self, end_pos: BytePos) {
Expand Down
5 changes: 5 additions & 0 deletions tests/mod-resolver/skip-files-issue-5065/foo.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#![rustfmt::skip]

mod bar {

mod baz;}
1 change: 1 addition & 0 deletions tests/mod-resolver/skip-files-issue-5065/foo/bar/baz.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
fn baz() { }
9 changes: 9 additions & 0 deletions tests/mod-resolver/skip-files-issue-5065/main.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
#![rustfmt::skip]

mod foo;
mod one;

fn main() {println!("Hello, world!");
}

// trailing commet
1 change: 1 addition & 0 deletions tests/mod-resolver/skip-files-issue-5065/one.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
struct One { value: String }
7 changes: 7 additions & 0 deletions tests/target/skip/preserve_trailing_comment.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
#![rustfmt::skip]

fn main() {
println!("Hello, world!");
}

// Trailing Comment