-
Notifications
You must be signed in to change notification settings - Fork 909
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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!( | ||
|
@@ -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!( | ||
|
@@ -251,6 +255,7 @@ fn configuration_snippet_tests() { | |
let blocks = get_code_blocks(); | ||
let failures = blocks | ||
.iter() | ||
.filter(|block| !block.fmt_skip()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
#![rustfmt::skip] | ||
|
||
mod bar { | ||
|
||
mod baz;} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
fn baz() { } |
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
struct One { value: String } |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
#![rustfmt::skip] | ||
|
||
fn main() { | ||
println!("Hello, world!"); | ||
} | ||
|
||
// Trailing Comment |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 writemodule.attrs()
. Here's the error message you get if you remove theuse statement
and runcargo check