Confusing error when a module is found as a file and a directory

[camsteffen]

When a module is found as both a file modname.rs and a directory modname/mod.rs, the error from cargo fmt --all is wrong and misleading.

Reproduction steps:

cargo new foo --lib
cd foo
echo "mod bar;" > src/lib.rs
touch src/bar.rs
mkdir -p src/bar
touch src/bar/mod.rs
cargo fmt --all

Output:

Error writing files: failed to resolve mod `bar`: hi/foo/src does not exist

[ytmimi]

Thanks for the report!

Confirming that I can reproduce the issue.

I assumed there was an ambiguity error, and if you run cargo check that's exactly what you get. Here' the output:

error[E0761]: file for module `bar` found at both "src\bar.rs" and "src\bar\mod.rs"
 --> src\lib.rs:1:1
  |
1 | mod bar;
  | ^^^^^^^^
  |
  = help: delete or rename one of them to remove the ambiguity

Cargo check's is a much more helpful error message, so I'll see if I can hunt down what's happening and hopefully provide something similar.

[calebcartwright]

Cargo check's is a much more helpful error message, so I'll see if I can hunt down what's happening and hopefully provide something similar.

This almost certainly goes back to a number of pending PR in need of backport triaging. I'd start with those (they should be labeled accordingly) to avoid accidentally reinventing any wheels

[ytmimi]

Before I saw your comment I actually identified what was going wrong. The issue here is that we're matching on a Err(rustc_expand::module::ModError::MultipleCandidates) in ModResolver::find_external_module when we call ParseSess::default_submod_path. Previously this error case was always turned into a Err(ModuleResolutionErrorKind::NotFound), which isn't correct.

rustfmt/src/modules.rs

Lines 447 to 453 in 5056f4c

	Err(_) => Err(ModuleResolutionError {
	module: mod_name.to_string(),
	kind: ModuleResolutionErrorKind::NotFound {
	file: self.directory.path.clone(),
	},
	}),
	}

I was quickly able to fix the issue before I had a chance to read your comment. I've gone through the PRs and I'm struggling to find one that addresses this specific scenario. It's not a major change, just adding a new variant to ModuleResolutionErrorKind, but I'll hold off on submitting it. If you can identify the backport I'll take a look at that otherwise I've got a fix ready to go!

[calebcartwright]

Try reproducing on the 2.x branch. If you can reproduce then definitely feel free to submit a new PR with your changes. However, if you can't let's try to track down the original fix, and make a decision from there. My hesitation stems from having at least 3 or 4 fixes on that branch that are all dealing with the same code, and I want to try to avoid complicating our ability to pull in those prior fixes

[ytmimi]

Just tested this out on the 2.0 branch, and couldn't reproduce, so I'll go back through the backport triage more thoroughly to find the PR that fixes this.

[calebcartwright]

https://github.com/rust-lang/rustfmt/pulls?page=1&q=is%3Apr+label%3Abackport-triage+is%3Aclosed plus any other relevant search criteria you can think of

[camsteffen]

#4198 may be relevant.

[ytmimi]

Hey, So I went back and revisited the code. A lot of investigating between the two branches, but I figured out what's going on here. On further inspection I wasn't 100% correct when I said that the issue doesn't occur on the 2.0 branch. We still run into the same parsing error, but the 2.0 branch silently ignores it. The issue comes down to how modules::visit_mod_from_ast is implemented.

In the 1.0 branch we always return early if their is an error when calling ModResolver::visit_sub_mod (using the ? operator), but in the 2.0 branch we only return early if the error is encountered while we're recursively resolving modules. See the code linked below:

On the 1.0 branch:

rustfmt/src/modules.rs

Lines 209 to 223 in 5056f4c

	if let ast::ItemKind::Mod(_, ref sub_mod_kind) = item.kind {
	let span = item.span;
	self.visit_sub_mod(
	item,
	Module::new(
	span,
	Some(Cow::Borrowed(sub_mod_kind)),
	Cow::Owned(vec![]),
	Cow::Borrowed(&item.attrs),
	),
	)?;
	}
	}
	Ok(())
	}

On the 2.0 branch:

rustfmt/src/formatting/modules.rs

Lines 250 to 264 in cc5521d

	if let ast::ItemKind::Mod(_, ref sub_mod_kind) = item.kind {
	let result = self.visit_sub_mod(Module::new(
	item.span,
	Some(Cow::Borrowed(sub_mod_kind)),
	Some(Cow::Borrowed(item)),
	Cow::Owned(vec![]),
	Cow::Borrowed(&item.attrs),
	));
	if result.is_err() && self.recursive {
	return result;
	}
	}
	}
	Ok(())
	}

#4284 is the backport-triage PR that introduced this recursive check for returning errors.

I personally feel like the 1.0 behavior is what we want, and that we should let users know that there was a module resolution error (the aforementioned rustc_expand::module::ModError::MultipleCandidates error). However, as things are currently implemented we're telling users that files don't exist, when it would be more appropriate to inform them about the ambiguous module path.