rustfmt deletes lines instead of detecting a syntax error

[jbaublitz]

Code sample

This code sample had an error that I introduced while trying to fix a clippy error.

impl<'a, P> Iterator for NlMessageIter<'a, NlTypeWrapper, P>
where
    P: Nl + Debug,
{
    type Item = Result<Nlmsghdr<NlTypeWrapper, P>, NlError>;

    fn next(&mut self) -> Option<Self::Item> {
        if let Some(true) = self.next_is_none {
            return None;
        }

        if self.stored.is_empty() {
            match self.socket_ref.recv_all_nl() {
                Ok(mut rn) => {
                    while let Some(item) = rn.pop() {
                        self.stored.push(item)
                    }
                }
                Err(e) => return Some(Err(e)),
            }
        }
        let next = self.stored.pop();
        if let Some(ref n) = next {
            if self.next_is_none.is_some() && if !n.nl_flags.contains(&NlmF::Multi) {
                self.next_is_none = Some(true);
            }
            if n.nl_type == NlTypeWrapper::Nlmsg(Nlmsg::Done) {
                return None;
            }
        }
        next.map(Ok)
    }
}

In particular, the line:

            if self.next_is_none.is_some() && if !n.nl_flags.contains(&NlmF::Multi) {

has two ifs.

Changes made by rustfmt

This is the diff I received when I ran rustfmt:

diff --git a/src/socket.rs b/src/socket.rs
index 029c27a..c3f562d 100644
--- a/src/socket.rs
+++ b/src/socket.rs
@@ -125,11 +125,7 @@ where
         }
         let next = self.stored.pop();
         if let Some(ref n) = next {
-            if self.next_is_none.is_some() && if !n.nl_flags.contains(&NlmF::Multi) {
-                self.next_is_none = Some(true);
             }
-            if n.nl_type == NlTypeWrapper::Nlmsg(Nlmsg::Done) {
-                return None;
             }
         }
         next.map(Ok)

Expected behavior

I would hope that rustfmt would throw a syntax error instead of deleting the code. Will rustfmt typically proceed even if there is a syntax error in the code? I don't think I've ever run rustfmt on malformed code before.

[calebcartwright]

what version of rustfmt are you using @jbaublitz?(rustfmt --version)

[jbaublitz]

Whoops! Sorry about that. rustfmt 1.4.13-nightly (c126730 2020-03-31)

[calebcartwright]

No worries! Could you try the latest version (1.4.14) and see if there's any difference in behavior?

rustfmt uses the rustc parser to get the AST for the input files, so the parsing behavior is really dependent on the version of the rustc parser that's in use by rustfmt.

If the rustc parser errors, then rustfmt will bubble that error up accordingly. However, if the parser succeeds and is able to generate the AST, then rustfmt will use that AST to determine the corresponding formatting.

[jbaublitz]

Just confirmed that rustfmt 1.4.14-nightly (a5cb5d2 2020-04-14) has the same issue.

[calebcartwright]

So far I'm not able to reproduce this.

Could you run rustfmt in --check mode against your original snippet and include the command you are running as well as the terminal output from rustfmt (not a git diff)

if you are running cargo fmt then run cargo fmt -- --check, or if you are running rustfmt directly then just include the --check flag.

[jbaublitz]

This may be related to the other code in the module outside of the example I pasted because I am getting:

Diff in /home/jbaublitz/Source/neli/src/socket.rs at line 125:
         }
         let next = self.stored.pop();
         if let Some(ref n) = next {
-            if self.next_is_none.is_some() && if !n.nl_flags.contains(&NlmF::Multi) {
-                self.next_is_none = Some(true);
             }
-            if n.nl_type == NlTypeWrapper::Nlmsg(Nlmsg::Done) {
-                return None;
             }
         }
         next.map(Ok)

when I reapply the incorrect syntax and run cargo fmt -- --check.

The repository is https://github.com/jbaublitz/neli and it is line number 128 on the branch v0.5.0-dev (or you can view it here). I've changed the code to be syntactically correct in git but when I add the second if again, I'm able to reproduce this behavior consistently.

[calebcartwright]

I've changed the code to be syntactically correct in git but when I add the second if again, I'm able to reproduce this behavior consistently.

Yikes! Thanks for the extra info @jbaublitz. I can reproduce this with cargo fmt and rustfmt with the entry point lib.rs, but not with rustfmt directly against the file which is baffling 😕

Here's things working as expected using rustfmt

$ rustc -Vv
rustc 1.44.0-nightly (7f3df5772 2020-04-16)
binary: rustc
commit-hash: 7f3df5772439eee1c512ed2eb540beef1124d236
commit-date: 2020-04-16
host: x86_64-unknown-linux-gnu
release: 1.44.0-nightly
LLVM version: 9.0

$ rustfmt --version
rustfmt 1.4.14-nightly (a5cb5d26 2020-04-14)

$ git branch
  master
* v0.5.0-dev

$ rustfmt src/socket.rs --check
error: expected `{`, found keyword `if`
   --> /home/caleb/dev/forks/neli/src/socket.rs:131:13
    |
128 | ...   if self.next_is_none.is_some() && if !n.nl_flags.contains(&NlmF::...
    |       -- this `if` expression has a condition, but no block
...
131 | ...   if n.nl_type == NlTypeWrapper::Nlmsg(Nlmsg::Done) {
    |       ^^ expected `{`
    |
help: try placing this code inside a block
    |
131 |             { if n.nl_type == NlTypeWrapper::Nlmsg(Nlmsg::Done) {
132 |                 return None;
133 |             } }
    |

but then with cargo fmt/rustfmt src/lib.rs

$ cargo fmt -- --check
Diff in /home/caleb/dev/forks/neli/src/socket.rs at line 125:
         }
         let next = self.stored.pop();
         if let Some(ref n) = next {
-            if self.next_is_none.is_some() && if !n.nl_flags.contains(&NlmF::Multi) {
-                self.next_is_none = Some(true);
             }
-            if n.nl_type == NlTypeWrapper::Nlmsg(Nlmsg::Done) {
-                return None;
             }
         }
         next.map(Ok)

[calebcartwright]

A few things are happening here, will some require some changes to the mod resolver/non-inline module parsing (for example no longer disabling the parse session emitter after the initial crate parsing)

[jbaublitz]

@calebcartwright Excellent, thanks for looking into this! Given that I'm doing development on that branch, is it sufficient for you to keep a copy of the file/repo or do you have another suggestion?

[calebcartwright]

If you're asking if we need your source input as a reference then the answer is no, go ahead and do whatever you need with it!

There's far simpler reproduction cases (basically any mod defined in an external file that has a recoverable parser error) that we'll put together as part of the test suites that'll accompany the fix for this.

[topecongiro]

@calebcartwright Is this still reproducible with rustfmt from the master branch?

[calebcartwright]

@topecongiro - unfortunately yes it is. I've pushed a minimal reproducible example here where cargo fmt and rustfmt src/lib.rs --recursive will strip syntax just like described in the OP, and rustfmt src/bad.rs will fail as expected with parser error.

The problem comes from the changes made to incorporate the upstream parser/expansion changes (particularly that the parser no longer parses out-of-line mods), and I just didn't think about this use case.

The formatting/mod resolver flow is still operating as if the pre-v651 version of the parser were in use,

rustfmt/rustfmt-core/rustfmt-lib/src/formatting.rs

Lines 73 to 99 in a9d7cd5

	let krate = match Parser::parse_crate(config, input, directory_ownership, &parse_session) {
	Ok(krate) => krate,
	Err(e) => {
	let forbid_verbose = input_is_stdin || e != ParserError::ParsePanicError;
	should_emit_verbose(forbid_verbose, config, || {
	eprintln!(
	"The Rust parser panicked while parsing input: {:?}: {:?}",
	main_file, e
	);
	});
	report.add_parsing_error();
	return Ok(report);
	}
	};
	timer = timer.done_parsing();
	// Suppress error output if we have to do any further parsing.
	parse_session.set_silent_emitter();
	let mut context = FormatContext::new(&krate, report, parse_session, config, handler);
	let files = modules::ModResolver::new(
	&context.parse_session,
	directory_ownership.unwrap_or(DirectoryOwnership::UnownedViaMod),
	!input_is_stdin && config.recursive(),
	)
	.visit_crate(&krate)
	.map_err(|e| io::Error::new(io::ErrorKind::Other, e))?;

originally, if the parser successfully generated the AST for the crate, we were basically done with parsing, thus capturing the timing and disabling the emitter.

rustfmt/rustfmt-core/rustfmt-lib/src/formatting.rs

Lines 87 to 90 in a9d7cd5

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

however, parsing of out-of-line mods is now done as needed within the mod resolver as part of building out the mod/file mapping (initial step towards #3930).

the parser is, of course, encountering a syntax error in this case, but because we've switched to a silent emitter that diagnostic message is being swallowed. And in the case of parsing a file as mod, we aren't checking the ParseSess for any parsing errors:

rustfmt/rustfmt-core/rustfmt-lib/src/syntux/parser.rs

Lines 185 to 192 in a9d7cd5

	match Parser::parse_mod_items(&mut parser, lo) {
	Ok(m) => Some(m),
	Err(mut db) => {
	db.cancel();
	sess.reset_errors();
	None
	}
	}

like we do when parsing the crate mod:

rustfmt/rustfmt-core/rustfmt-lib/src/syntux/parser.rs

Lines 220 to 236 in a9d7cd5

	Ok(Ok(krate)) => {
	if !self.sess.has_errors() {
	return Ok(krate);
	}
	if self.sess.can_reset_errors() {
	self.sess.reset_errors();
	return Ok(krate);
	}
	Err(ParserError::ParseError)
	}
	Ok(Err(mut db)) => {
	db.emit();
	Err(ParserError::ParseError)
	}
	Err(_) => Err(ParserError::ParsePanicError),

the mod resolver/mapping process is due for a refactor (#3930), but I'm not sure if it'd be easiest to try to fix the existing flow (by not disabling the emitter and checking for session errors when parsing the out-of-line mod files) or just go ahead with the refactor

[topecongiro]

I think we should check for session errors in parse_file_as_module and make it a hard error. cc #3989.