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

fallback to dir_path when relative external mod resolution fails #5205

Merged
merged 1 commit into from
Mar 1, 2022
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
22 changes: 20 additions & 2 deletions src/parse/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use rustc_span::{

use crate::config::file_lines::LineRange;
use crate::ignore_path::IgnorePathSet;
use crate::parse::parser::{ModError, ModulePathSuccess};
use crate::source_map::LineRangeUtils;
use crate::utils::starts_with_newline;
use crate::visitor::SnippetProvider;
Expand Down Expand Up @@ -145,13 +146,30 @@ impl ParseSess {
})
}

/// Determine the submodule path for the given module identifier.
///
/// * `id` - The name of the module
/// * `relative` - If Some(symbol), the symbol name is a directory relative to the dir_path.
/// If relative is Some, resolve the submodle at {dir_path}/{symbol}/{id}.rs
/// or {dir_path}/{symbol}/{id}/mod.rs. if None, resolve the module at {dir_path}/{id}.rs.
/// * `dir_path` - Module resolution will occur relative to this direcotry.
pub(crate) fn default_submod_path(
&self,
id: symbol::Ident,
relative: Option<symbol::Ident>,
dir_path: &Path,
) -> Result<rustc_expand::module::ModulePathSuccess, rustc_expand::module::ModError<'_>> {
rustc_expand::module::default_submod_path(&self.parse_sess, id, relative, dir_path)
) -> Result<ModulePathSuccess, ModError<'_>> {
rustc_expand::module::default_submod_path(&self.parse_sess, id, relative, dir_path).or_else(
|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 matches!(e, ModError::FileNotFound(..)) && relative.is_some() {
rustc_expand::module::default_submod_path(&self.parse_sess, id, None, dir_path)
} else {
Err(e)
}
},
)
}

pub(crate) fn is_file_parsed(&self, path: &Path) -> bool {
Expand Down
16 changes: 16 additions & 0 deletions src/test/mod_resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,3 +64,19 @@ fn fmt_out_of_line_test_modules() {
],
)
}

#[test]
fn fallback_and_try_to_resolve_external_submod_relative_to_current_dir_path() {
// See also https://github.com/rust-lang/rustfmt/issues/5198
verify_mod_resolution(
"tests/mod-resolver/issue-5198/lib.rs",
&[
"tests/mod-resolver/issue-5198/a.rs",
"tests/mod-resolver/issue-5198/lib/b.rs",
"tests/mod-resolver/issue-5198/lib/c/mod.rs",
"tests/mod-resolver/issue-5198/lib/c/e.rs",
"tests/mod-resolver/issue-5198/lib/c/d/f.rs",
"tests/mod-resolver/issue-5198/lib/c/d/g/mod.rs",
],
)
}
1 change: 1 addition & 0 deletions tests/mod-resolver/issue-5198/a.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
fn main( ) { println!("Hello World!") }
3 changes: 3 additions & 0 deletions tests/mod-resolver/issue-5198/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
mod a;
mod b;
mod c;
1 change: 1 addition & 0 deletions tests/mod-resolver/issue-5198/lib/b.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
fn main( ) { println!("Hello World!") }
Copy link
Member

Choose a reason for hiding this comment

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

Fully understand the utility of copy/pasta but for future reference I find it's helpful to make these types of files even subtly different (even just a different fn name) as it stands out more on the off chance we end up mapping the wrong files to modules.

Understand that wouldn't be caught by the particular test here given the nature of the test, just wanted to make a note

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really appreciate you calling this out!

3 changes: 3 additions & 0 deletions tests/mod-resolver/issue-5198/lib/c/d.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
mod e;
mod f;
mod g;
16 changes: 16 additions & 0 deletions tests/mod-resolver/issue-5198/lib/c/d/explanation.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
This file is contained in the './lib/c/d/' directory.

The directory name './lib/c/d/' conflicts with the './lib/c/d.rs' file name.

'./lib/c/d.rs' defines 3 external modules:

* mod e;
* mod f;
* mod g;

Module resolution will fail if we look for './lib/c/d/e.rs' or './lib/c/d/e/mod.rs',
so we should fall back to looking for './lib/c/e.rs', which correctly finds the modlue, that
rustfmt should format.

'./lib/c/d/f.rs' and './lib/c/d/g/mod.rs' exist at the default submodule paths so we should be able
to resolve these modules with no problems.
1 change: 1 addition & 0 deletions tests/mod-resolver/issue-5198/lib/c/d/f.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
fn main( ) { println!("Hello World!") }
1 change: 1 addition & 0 deletions tests/mod-resolver/issue-5198/lib/c/d/g/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
fn main( ) { println!("Hello World!") }
1 change: 1 addition & 0 deletions tests/mod-resolver/issue-5198/lib/c/e.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
fn main( ) { println!("Hello World!") }
3 changes: 3 additions & 0 deletions tests/mod-resolver/issue-5198/lib/c/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
mod d;

fn main( ) { println!("Hello World!") }
16 changes: 16 additions & 0 deletions tests/mod-resolver/issue-5198/lib/explanation.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
This file is contained in the './lib' directory.

The directory name './lib' conflicts with the './lib.rs' file name.

'lib.rs' defines 3 external modules:

* mod a;
* mod b;
* mod c;

Module resolution will fail if we look for './lib/a.rs' or './lib/a/mod.rs',
so we should fall back to looking for './a.rs', which correctly finds the modlue that
Copy link
Member

Choose a reason for hiding this comment

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

minor typo fyi

Suggested change
so we should fall back to looking for './a.rs', which correctly finds the modlue that
so we should fall back to looking for './a.rs', which correctly finds the module that

Copy link
Contributor Author

@ytmimi ytmimi Mar 1, 2022

Choose a reason for hiding this comment

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

Aww man, I'm sorry about that. I totally would have gone back and fixed this before you merged had I had the opportunity to do so before the merge.

Unrelated to the typo, but was the explanation.txt file helpful?

Copy link
Member

Choose a reason for hiding this comment

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

Ha no worries. We've typos all over the place 😆

And yes I think the text file with explanatory commentary is helpful in cases like these as it lowers the barrier required to grok especially when trying to eyeball runtime module loading behavior

rustfmt should format.

'./lib/b.rs' and './lib/c/mod.rs' exist at the default submodule paths so we should be able
to resolve these modules with no problems.