Repository: rust-lang/rustfmt (rust)

Issue:
Confusing error when a module is found as a file and a directory
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:
```console
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:
```console
Error writing files: failed to resolve mod `bar`: hi/foo/src does not exist
```

Pull request:
Improve mod resolution error for mods with multiple candidate files
Fixes #5167

When ``a.rs`` and ``a/mod.rs`` are both present we would emit an error
message telling the user that the module couldn't be found. However,
this behavior is misleading because we're dealing with an ambiguous
module path, not a "file not found" error.

Is the file ``a.rs`` or is it ``a/mod.rs``? Rustfmt can't decide, and
the user needs to resolve this ambiguity themselves.

Now, the error message displayed to the user is in line with what they
would see if they went to compile their code with these conflicting
module names.

Patch:
diff --git a/src/modules.rs b/src/modules.rs
index 70b937b0..49c99403 100644
--- a/src/modules.rs
+++ b/src/modules.rs
@@ -81,6 +81,7 @@ pub struct ModuleResolutionError {
     pub(crate) kind: ModuleResolutionErrorKind,
 }
 
+/// Defines variants similar to those of [rustc_expand::module::ModError]
 #[derive(Debug, Error)]
 pub(crate) enum ModuleResolutionErrorKind {
     /// Find a file that cannot be parsed.
@@ -89,6 +90,12 @@ pub(crate) enum ModuleResolutionErrorKind {
     /// File cannot be found.
     #[error("{file} does not exist")]
     NotFound { file: PathBuf },
+    /// File a.rs and a/mod.rs both exist
+    #[error("file for module found at both {default_path:?} and {secondary_path:?}")]
+    MultipleCandidates {
+        default_path: PathBuf,
+        secondary_path: PathBuf,
+    },
 }
 
 #[derive(Clone)]
@@ -444,12 +451,31 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> {
                 }
                 Ok(Some(SubModKind::MultiExternal(mods_outside_ast)))
             }
-            Err(_) => Err(ModuleResolutionError {
-                module: mod_name.to_string(),
-                kind: ModuleResolutionErrorKind::NotFound {
-                    file: self.directory.path.clone(),
-                },
-            }),
+            Err(e) => match e {
+                ModError::FileNotFound(_, default_path, _secondary_path) => {
+                    Err(ModuleResolutionError {
+                        module: mod_name.to_string(),
+                        kind: ModuleResolutionErrorKind::NotFound { file: default_path },
+                    })
+                }
+                ModError::MultipleCandidates(_, default_path, secondary_path) => {
+                    Err(ModuleResolutionError {
+                        module: mod_name.to_string(),
+                        kind: ModuleResolutionErrorKind::MultipleCandidates {
+                            default_path,
+                            secondary_path,
+                        },
+                    })
+                }
+                ModError::ParserError(_)
+                | ModError::CircularInclusion(_)
+                | ModError::ModInBlock(_) => Err(ModuleResolutionError {
+                    module: mod_name.to_string(),
+                    kind: ModuleResolutionErrorKind::ParseError {
+                        file: self.directory.path.clone(),
+                    },
+                }),
+            },
         }
     }
 
diff --git a/src/parse/session.rs b/src/parse/session.rs
index fb918215..87ab8fbf 100644
--- a/src/parse/session.rs
+++ b/src/parse/session.rs
@@ -163,8 +163,11 @@ impl ParseSess {
             |e| {
                 // If resloving a module relative to {dir_path}/{symbol} fails because a file
                 // could not be found, then try to resolve the module relative to {dir_path}.
+                // If we still can't find the module after searching for it in {dir_path},
+                // surface the original error.
                 if matches!(e, ModError::FileNotFound(..)) && relative.is_some() {
                     rustc_expand::module::default_submod_path(&self.parse_sess, id, None, dir_path)
+                        .map_err(|_| e)
                 } else {
                     Err(e)
                 }
diff --git a/src/vertical.rs b/src/vertical.rs
index c4208848..a06bc995 100644
--- a/src/vertical.rs
+++ b/src/vertical.rs
@@ -160,8 +160,18 @@ pub(crate) fn rewrite_with_alignment<T: AlignedItem>(
     };
     let init_span = mk_sp(span.lo(), init_last_pos);
     let one_line_width = if rest.is_empty() { one_line_width } else { 0 };
-    let result =
-        rewrite_aligned_items_inner(context, init, init_span, shape.indent, one_line_width)?;
+
+    // if another group follows, we must force a separator
+    let force_separator = !rest.is_empty();
+
+    let result = rewrite_aligned_items_inner(
+        context,
+        init,
+        init_span,
+        shape.indent,
+        one_line_width,
+        force_separator,
+    )?;
     if rest.is_empty() {
         Some(result + spaces)
     } else {
@@ -201,6 +211,7 @@ fn rewrite_aligned_items_inner<T: AlignedItem>(
     span: Span,
     offset: Indent,
     one_line_width: usize,
+    force_trailing_separator: bool,
 ) -> Option<String> {
     // 1 = ","
     let item_shape = Shape::indented(offset, context.config).sub_width(1)?;
@@ -246,9 +257,15 @@ fn rewrite_aligned_items_inner<T: AlignedItem>(
             });
     }
 
+    let separator_tactic = if force_trailing_separator {
+        SeparatorTactic::Always
+    } else {
+        context.config.trailing_comma()
+    };
+
     let fmt = ListFormatting::new(item_shape, context.config)
         .tactic(tactic)
-        .trailing_separator(context.config.trailing_comma())
+        .trailing_separator(separator_tactic)
         .preserve_newline(true);
     write_list(&items, &fmt)
 }


Test patch:
diff --git a/tests/mod-resolver/issue-5167/src/a.rs b/tests/mod-resolver/issue-5167/src/a.rs
new file mode 100644
index 00000000..e69de29b
diff --git a/tests/mod-resolver/issue-5167/src/a/mod.rs b/tests/mod-resolver/issue-5167/src/a/mod.rs
new file mode 100644
index 00000000..e69de29b
diff --git a/tests/mod-resolver/issue-5167/src/lib.rs b/tests/mod-resolver/issue-5167/src/lib.rs
new file mode 100644
index 00000000..f21af614
--- /dev/null
+++ b/tests/mod-resolver/issue-5167/src/lib.rs
@@ -0,0 +1,1 @@
+mod a;
diff --git a/tests/mod-resolver/module-not-found/bad_path_attribute/lib.rs b/tests/mod-resolver/module-not-found/bad_path_attribute/lib.rs
new file mode 100644
index 00000000..2a63c961
--- /dev/null
+++ b/tests/mod-resolver/module-not-found/bad_path_attribute/lib.rs
@@ -0,0 +1,3 @@
+// module resolution fails because the path does not exist.
+#[path = "path/to/does_not_exist.rs"]
+mod a;
diff --git a/tests/mod-resolver/module-not-found/relative_module/a.rs b/tests/mod-resolver/module-not-found/relative_module/a.rs
new file mode 100644
index 00000000..4a1eac89
--- /dev/null
+++ b/tests/mod-resolver/module-not-found/relative_module/a.rs
@@ -0,0 +1,2 @@
+// module resolution fails because `./a/b.rs` does not exist
+mod b;
diff --git a/tests/mod-resolver/module-not-found/relative_module/lib.rs b/tests/mod-resolver/module-not-found/relative_module/lib.rs
new file mode 100644
index 00000000..f21af614
--- /dev/null
+++ b/tests/mod-resolver/module-not-found/relative_module/lib.rs
@@ -0,0 +1,1 @@
+mod a;
diff --git a/tests/mod-resolver/module-not-found/sibling_module/lib.rs b/tests/mod-resolver/module-not-found/sibling_module/lib.rs
new file mode 100644
index 00000000..d9d9e1e3
--- /dev/null
+++ b/tests/mod-resolver/module-not-found/sibling_module/lib.rs
@@ -0,0 +1,2 @@
+// module resolution fails because `./a.rs` does not exist
+mod a;
diff --git a/tests/rustfmt/main.rs b/tests/rustfmt/main.rs
index 2262ae3a..450051d2 100644
--- a/tests/rustfmt/main.rs
+++ b/tests/rustfmt/main.rs
@@ -113,3 +113,47 @@ fn rustfmt_usage_text() {
     let (stdout, _) = rustfmt(&args);
     assert!(stdout.contains("Format Rust code\n\nusage: rustfmt [options] <file>..."));
 }
+
+#[test]
+fn mod_resolution_error_multiple_candidate_files() {
+    // See also https://github.com/rust-lang/rustfmt/issues/5167
+    let default_path = Path::new("tests/mod-resolver/issue-5167/src/a.rs");
+    let secondary_path = Path::new("tests/mod-resolver/issue-5167/src/a/mod.rs");
+    let error_message = format!(
+        "file for module found at both {:?} and {:?}",
+        default_path.canonicalize().unwrap(),
+        secondary_path.canonicalize().unwrap(),
+    );
+
+    let args = ["tests/mod-resolver/issue-5167/src/lib.rs"];
+    let (_stdout, stderr) = rustfmt(&args);
+    assert!(stderr.contains(&error_message))
+}
+
+#[test]
+fn mod_resolution_error_sibling_module_not_found() {
+    let args = ["tests/mod-resolver/module-not-found/sibling_module/lib.rs"];
+    let (_stdout, stderr) = rustfmt(&args);
+    // Module resolution fails because we're unable to find `a.rs` in the same directory as lib.rs
+    assert!(stderr.contains("a.rs does not exist"))
+}
+
+#[test]
+fn mod_resolution_error_relative_module_not_found() {
+    let args = ["tests/mod-resolver/module-not-found/relative_module/lib.rs"];
+    let (_stdout, stderr) = rustfmt(&args);
+    // The file `./a.rs` and directory `./a` both exist.
+    // Module resolution fails becuase we're unable to find `./a/b.rs`
+    #[cfg(not(windows))]
+    assert!(stderr.contains("a/b.rs does not exist"));
+    #[cfg(windows)]
+    assert!(stderr.contains("a\\b.rs does not exist"));
+}
+
+#[test]
+fn mod_resolution_error_path_attribute_does_not_exist() {
+    let args = ["tests/mod-resolver/module-not-found/bad_path_attribute/lib.rs"];
+    let (_stdout, stderr) = rustfmt(&args);
+    // The path attribute points to a file that does not exist
+    assert!(stderr.contains("does_not_exist.rs does not exist"));
+}
diff --git a/tests/source/issue-4791/buggy.rs b/tests/source/issue-4791/buggy.rs
new file mode 100644
index 00000000..4760022e
--- /dev/null
+++ b/tests/source/issue-4791/buggy.rs
@@ -0,0 +1,14 @@
+// rustfmt-struct_field_align_threshold: 30
+// rustfmt-trailing_comma: Never
+
+struct Foo {
+    group_a: u8,
+
+    group_b: u8,
+}
+
+struct Bar {
+    group_a: u8,
+
+    group_b: u8
+}
diff --git a/tests/source/issue-4791/trailing_comma.rs b/tests/source/issue-4791/trailing_comma.rs
new file mode 100644
index 00000000..c56c70fa
--- /dev/null
+++ b/tests/source/issue-4791/trailing_comma.rs
@@ -0,0 +1,14 @@
+// rustfmt-struct_field_align_threshold: 30
+// rustfmt-trailing_comma: Always
+
+struct Foo {
+    group_a: u8,
+
+    group_b: u8
+}
+
+struct Bar {
+    group_a: u8,
+
+    group_b: u8,
+}
diff --git a/tests/target/issue-4791/buggy.rs b/tests/target/issue-4791/buggy.rs
new file mode 100644
index 00000000..fff58be9
--- /dev/null
+++ b/tests/target/issue-4791/buggy.rs
@@ -0,0 +1,14 @@
+// rustfmt-struct_field_align_threshold: 30
+// rustfmt-trailing_comma: Never
+
+struct Foo {
+    group_a: u8,
+
+    group_b: u8
+}
+
+struct Bar {
+    group_a: u8,
+
+    group_b: u8
+}
diff --git a/tests/target/issue-4791/issue_4928.rs b/tests/target/issue-4791/issue_4928.rs
new file mode 100644
index 00000000..588656b5
--- /dev/null
+++ b/tests/target/issue-4791/issue_4928.rs
@@ -0,0 +1,70 @@
+// rustfmt-brace_style: SameLineWhere
+// rustfmt-comment_width: 100
+// rustfmt-edition: 2018
+// rustfmt-fn_args_layout: Compressed
+// rustfmt-hard_tabs: false
+// rustfmt-match_block_trailing_comma: true
+// rustfmt-max_width: 100
+// rustfmt-merge_derives: false
+// rustfmt-newline_style: Unix
+// rustfmt-normalize_doc_attributes: true
+// rustfmt-overflow_delimited_expr: true
+// rustfmt-reorder_imports: false
+// rustfmt-reorder_modules: true
+// rustfmt-struct_field_align_threshold: 20
+// rustfmt-tab_spaces: 4
+// rustfmt-trailing_comma: Never
+// rustfmt-use_small_heuristics: Max
+// rustfmt-use_try_shorthand: true
+// rustfmt-wrap_comments: true
+
+/// Lorem ipsum dolor sit amet.
+#[repr(C)]
+#[derive(Debug, Default, Copy, Clone, PartialEq, Eq)]
+pub struct BufferAttr {
+    /* NOTE: Blah blah blah blah blah. */
+    /// Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt
+    /// ut labore et dolore magna aliqua. Morbi quis commodo odio aenean sed adipiscing. Nunc
+    /// congue nisi vitae suscipit tellus mauris a. Consectetur adipiscing elit pellentesque
+    /// habitant morbi tristique senectus.
+    pub foo: u32,
+
+    /// Elit eget gravida cum sociis natoque penatibus et magnis dis. Consequat semper viverra nam
+    /// libero. Accumsan in nisl nisi scelerisque eu. Pellentesque id nibh tortor id aliquet. Sed
+    /// velit dignissim sodales ut. Facilisis sed odio morbi quis commodo odio aenean sed. Et
+    /// ultrices neque ornare aenean euismod elementum. Condimentum lacinia quis vel eros donec ac
+    /// odio tempor.
+    ///
+    /// Lacinia at quis risus sed vulputate odio ut enim. Etiam erat velit scelerisque in dictum.
+    /// Nibh tellus molestie nunc non blandit massa enim nec. Nascetur ridiculus mus mauris vitae.
+    pub bar: u32,
+
+    /// Mi proin sed libero enim sed faucibus turpis. Amet consectetur adipiscing elit duis
+    /// tristique sollicitudin nibh sit amet. Congue quisque egestas diam in arcu cursus euismod
+    /// quis viverra. Cum sociis natoque penatibus et magnis dis parturient montes. Enim sit amet
+    /// venenatis urna cursus eget nunc scelerisque viverra. Cras semper auctor neque vitae tempus
+    /// quam pellentesque. Tortor posuere ac ut consequat semper viverra nam libero justo. Vitae
+    /// auctor eu augue ut lectus arcu bibendum at. Faucibus vitae aliquet nec ullamcorper sit amet
+    /// risus nullam. Maecenas accumsan lacus vel facilisis volutpat. Arcu non odio euismod
+    /// lacinia.
+    ///
+    /// [`FooBar::beep()`]: crate::foobar::FooBar::beep
+    /// [`FooBar::boop()`]: crate::foobar::FooBar::boop
+    /// [`foobar::BazBaq::BEEP_BOOP`]: crate::foobar::BazBaq::BEEP_BOOP
+    pub baz: u32,
+
+    /// Eu consequat ac felis donec et odio pellentesque diam. Ut eu sem integer vitae justo eget.
+    /// Consequat ac felis donec et odio pellentesque diam volutpat.
+    pub baq: u32,
+
+    /// Amet consectetur adipiscing elit pellentesque habitant. Ut morbi tincidunt augue interdum
+    /// velit euismod in pellentesque. Imperdiet sed euismod nisi porta lorem. Nec tincidunt
+    /// praesent semper feugiat. Facilisis leo vel fringilla est. Egestas diam in arcu cursus
+    /// euismod quis viverra. Sagittis eu volutpat odio facilisis mauris sit amet. Posuere morbi
+    /// leo urna molestie at.
+    ///
+    /// Pretium aenean pharetra magna ac. Nisl condimentum id venenatis a condimentum vitae. Semper
+    /// quis lectus nulla at volutpat diam ut venenatis tellus. Egestas tellus rutrum tellus
+    /// pellentesque eu tincidunt tortor aliquam.
+    pub foobar: u32
+}
diff --git a/tests/target/issue-4791/no_trailing_comma.rs b/tests/target/issue-4791/no_trailing_comma.rs
new file mode 100644
index 00000000..4a371639
--- /dev/null
+++ b/tests/target/issue-4791/no_trailing_comma.rs
@@ -0,0 +1,8 @@
+// rustfmt-struct_field_align_threshold: 0
+// rustfmt-trailing_comma: Never
+
+pub struct Baz {
+    group_a: u8,
+
+    group_b: u8
+}
diff --git a/tests/target/issue-4791/trailing_comma.rs b/tests/target/issue-4791/trailing_comma.rs
new file mode 100644
index 00000000..29a224b3
--- /dev/null
+++ b/tests/target/issue-4791/trailing_comma.rs
@@ -0,0 +1,14 @@
+// rustfmt-struct_field_align_threshold: 30
+// rustfmt-trailing_comma: Always
+
+struct Foo {
+    group_a: u8,
+
+    group_b: u8,
+}
+
+struct Bar {
+    group_a: u8,
+
+    group_b: u8,
+}