fallback to dir_path when relative external mod resolution fails #5205

ytmimi · 2022-01-30T20:51:37Z

We only want to fall back if two conditions are met:

Initial module resolution is performed relative to some nested directory.
Module resolution fails because of a ModError::FileNotFound error.

When these conditions are met we can try to fallback to searching for the module's file relative to the dir_path instead of the nested relative directory.

Fixes #5198

As demonstrated by 5198, it's possible that a directory name conflicts with a rust file name. For example, src/lib/ and src/lib.rs.

If src/lib.rs references an external module like mod foo;, then module resolution will try to resolve foo to src/lib/foo.rs or src/lib/foo/mod.rs. Module resolution would fail with a file not found error if the foo module were defined at src/foo.rs.

When encountering these kinds of module resolution issues we now fall back to the current directory and attempt to resolve the module again.

Given the current example, this means that if we can't find the module foo at src/lib/foo.rs or src/lib/foo/mod.rs, we'll attempt to resolve the module to src/foo.rs.

using rustfmt build from rustfmt master with rust-lang/rustfmt#5205 applied

We only want to fall back if two conditions are met: 1) Initial module resolution is performed relative to some nested directory. 2) Module resolution fails because of a ModError::FileNotFound error. When these conditions are met we can try to fallback to searching for the module's file relative to the dir_path instead of the nested relative directory. Fixes 5198 As demonstrated by 5198, it's possible that a directory name conflicts with a rust file name. For example, src/lib/ and src/lib.rs. If src/lib.rs references an external module like ``mod foo;``, then module resolution will try to resolve ``foo`` to src/lib/foo.rs or src/lib/foo/mod.rs. Module resolution would fail with a file not found error if the ``foo`` module were defined at src/foo.rs. When encountering these kinds of module resolution issues we now fall back to the current directory and attempt to resolve the module again. Given the current example, this means that if we can't find the module ``foo`` at src/lib/foo.rs or src/lib/foo/mod.rs, we'll attempt to resolve the module to src/foo.rs.

calebcartwright · 2022-02-15T23:28:13Z

Realize the follow ask is above and beyond the in-scope issue, but we have unfortunately had a history here of trading bug fixes where a fix for one reintroduces another. Whenever you get time, would you be up for checking how these changes handle some of the similar types of open issues?

Think we've got things labeled sufficiently well enough that https://github.com/rust-lang/rustfmt/issues?q=is%3Aopen+is%3Aissue+label%3Aa-mods should include a few worthwhile ones

ytmimi · 2022-02-16T14:50:36Z

@calebcartwright sure thig! I'd be happy to do a deep dive into the backlog to look and see if these changes resolve or change the behavior of some of the existing issues. Will probably have some time later this weekend to look into it!

calebcartwright · 2022-02-19T23:58:10Z

Awesome, thank you! My sense is that there might be an upstream issue causing this (e.g. bad dir path), but I'm okay with this approach since we're still letting the compiler drive resolution and just incorporating our own fallback behavior

ytmimi · 2022-02-21T02:15:38Z

Did some digging:

Confusing error when a module is found as a file and a directory #5167 is not resolved because there is a different underlying issue. This issue is caused by a ModError::FileNotFound while 5167 is caused by a ModError::MultipleCandidates error.
- I ultimately think 5167 could be solved by rustfmt's ModuleResolutionErrorKind fully enumerating the compilers ModError, which then kinda begs the question could we remove our ModuleResolutionErrorKind in favor of ModError? (Maybe this isn't the right thread to dive into that idea?)
Alternative approach for constructing the AST and mod/file mapping #3930: Read the discussion on the thread, but I don't think there's anything actionable for this issue.
Some of the other issues had to do with not finding modules inside macros which I think is unrelated.
Lastly, 1 issue had to do with a windows specific issue related to #[path] attributes.

I'm wondering if we'd benefit from making more of the compiler module resolution machinery public, for example mod_file_path? Looks like that ends up calling default_submod_path and also handles resolving file paths for #[path] attributes.

Awesome, thank you! My sense is that there might be an upstream issue causing this

I wonder if this dummy module has anything to do with what we're seeing here:

https://github.com/rust-lang/rust/blob/a41a6925badac7508d7a72cc1fc20f43dc6ad75e/compiler/rustc_expand/src/module.rs#L71

calebcartwright · 2022-02-22T16:27:45Z

This upstream comment gave me a good chuckle 😆

// Note that this will produce weirdness when a file named `foo.rs` is
// `#[path]` included and contains a `mod foo;` declaration.
// If you encounter this, it's your own darn fault :P

calebcartwright · 2022-02-22T16:30:48Z

I'm wondering if we'd benefit from making more of the compiler module resolution machinery public, for example mod_file_path? Looks like that ends up calling default_submod_path and also handles resolving file paths for #[path] attributes.

Feel free to look into it, but one thing to keep in mind is that we have to handle attributes a bit differently than the compiler, e.g. we need to follow all cfg_attrs whereas the compiler only cares about the relevant ones for a given platform/config/etc.

ytmimi · 2022-02-23T01:56:25Z

This upstream comment gave me a good chuckle 😆

Yeah I saw that one too and thought it was funny. Definitely something to keep in mind if someone opens up an issue about that weird case.

Feel free to look into it, but one thing to keep in mind is that we have to handle attributes a bit differently than the compiler, e.g. we need to follow all cfg_attrs whereas the compiler only cares about the relevant ones for a given platform/config/etc.

I don't necessarily know if I'll look into it right now, but I know you mentioned a while back (offline) that you want some help on improving the mod resolution in rustfmt. I think when we get started on that initiative it might be a better time for me to a do a deeper dive into what the compiler is doing and seeing if there's more that we can leverage.

All that being said, is there anything else you'd like to see on this one?

calebcartwright · 2022-02-23T03:08:44Z

All that being said, is there anything else you'd like to see on this one?

Nope! Was hoping it might've solved some others too, but sometimes we don't get that lucky.

ytmimi · 2022-02-23T13:29:05Z

Nope! Was hoping it might've solved some others too, but sometimes we don't get that lucky.

I have a fix for #5167 that should help improve the error message we're seeing in that case. I'll open that PR soon.

calebcartwright

Couple minor non-block items left inline for your awareness, otherwise lgtm!

calebcartwright · 2022-03-01T05:12:08Z

tests/mod-resolver/issue-5198/lib/b.rs

@@ -0,0 +1 @@
+fn main(        ) {   println!("Hello World!")                         }


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

I really appreciate you calling this out!

calebcartwright · 2022-03-01T05:12:29Z

tests/mod-resolver/issue-5198/lib/explanation.txt

+    * 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


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

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?

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

ytmimi force-pushed the issue_5198 branch from 296d51a to 1f14979 Compare January 30, 2022 20:51

ytmimi added the pr-not-reviewed label Jan 30, 2022

Skgland added a commit to Skgland/scryer-prolog that referenced this pull request Jan 30, 2022

rustfmt 7 scryer-prolog lib.rs and children

55d0891

using rustfmt build from rustfmt master with rust-lang/rustfmt#5205 applied

ytmimi force-pushed the issue_5198 branch from 1f14979 to 753a564 Compare January 31, 2022 14:22

ytmimi force-pushed the issue_5198 branch from 753a564 to f975b88 Compare January 31, 2022 14:36

calebcartwright removed the pr-not-reviewed label Feb 18, 2022

calebcartwright added the pr-waiting-on-author label Feb 19, 2022

ytmimi removed the pr-waiting-on-author label Feb 21, 2022

calebcartwright self-requested a review February 25, 2022 03:18

calebcartwright approved these changes Mar 1, 2022

View reviewed changes

calebcartwright merged commit 12048e4 into rust-lang:master Mar 1, 2022

ytmimi mentioned this pull request Mar 1, 2022

Improve mod resolution error for mods with multiple candidate files #5243

Merged

ytmimi deleted the issue_5198 branch August 7, 2022 15:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fallback to dir_path when relative external mod resolution fails #5205

fallback to dir_path when relative external mod resolution fails #5205

ytmimi commented Jan 30, 2022

calebcartwright commented Feb 15, 2022

ytmimi commented Feb 16, 2022

calebcartwright commented Feb 19, 2022 •

edited

Loading

ytmimi commented Feb 21, 2022

calebcartwright commented Feb 22, 2022

calebcartwright commented Feb 22, 2022

ytmimi commented Feb 23, 2022

calebcartwright commented Feb 23, 2022

ytmimi commented Feb 23, 2022

calebcartwright left a comment

calebcartwright Mar 1, 2022

ytmimi Mar 1, 2022

calebcartwright Mar 1, 2022

ytmimi Mar 1, 2022 •

edited

Loading

calebcartwright Mar 5, 2022

	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

fallback to dir_path when relative external mod resolution fails #5205

fallback to dir_path when relative external mod resolution fails #5205

Conversation

ytmimi commented Jan 30, 2022

calebcartwright commented Feb 15, 2022

ytmimi commented Feb 16, 2022

calebcartwright commented Feb 19, 2022 • edited Loading

ytmimi commented Feb 21, 2022

calebcartwright commented Feb 22, 2022

calebcartwright commented Feb 22, 2022

ytmimi commented Feb 23, 2022

calebcartwright commented Feb 23, 2022

ytmimi commented Feb 23, 2022

calebcartwright left a comment

Choose a reason for hiding this comment

calebcartwright Mar 1, 2022

Choose a reason for hiding this comment

ytmimi Mar 1, 2022

Choose a reason for hiding this comment

calebcartwright Mar 1, 2022

Choose a reason for hiding this comment

ytmimi Mar 1, 2022 • edited Loading

Choose a reason for hiding this comment

calebcartwright Mar 5, 2022

Choose a reason for hiding this comment

calebcartwright commented Feb 19, 2022 •

edited

Loading

ytmimi Mar 1, 2022 •

edited

Loading