Skip to content

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

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

Merged
merged 1 commit into from
Mar 9, 2022

Conversation

ytmimi
Copy link
Contributor

@ytmimi ytmimi commented Feb 26, 2022

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.

Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

Looks like a good improvement to me, thanks! For the sake of consistent tests across the variants, would you mind adding one that still triggers the file not found variant (I assume easiest way would be a test that had a path attribute on the mod import pointing to some non-existent file)

@ytmimi
Copy link
Contributor Author

ytmimi commented Mar 1, 2022

Good call! I'll get on that soon.

@ytmimi ytmimi force-pushed the issue_5167 branch 2 times, most recently from caecaee to 6584c7e Compare March 1, 2022 14:36
@ytmimi
Copy link
Contributor Author

ytmimi commented Mar 1, 2022

@calebcartwright I added additional tests to cover 3 different cases where a module might not be found. I also needed to modify the behavior I introduced in #5205 to surface the original error when the fallback doesn't work.

Unverified

This user has not yet uploaded their public signing key.
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.
@ytmimi
Copy link
Contributor Author

ytmimi commented Mar 1, 2022

A slight improvement could be made to the existing behavior proposed in this PR.

In the currently implementation we'll always tell uses that foo.rs is missing (which is technically true if we reach a ModError::FileNotFound error), but if the foo directory exists it might be nicer to tell users that foo/mod.rs doesn't exist. The code to make the change is as follows (in the cotext of the match in src/modules.rs line 454):

                ModError::FileNotFound(_, default_path, secondary_path) => {
                    let file = if let Some(path) = secondary_path.parent() && path.exist() {
                        secondary_path
                    } else {
                        default_path
                    };
                    Err(ModuleResolutionError {
                        module: mod_name.to_string(),
                        kind: ModuleResolutionErrorKind::NotFound { file },
                    })
                }

to write the code more succinctly I've used the new let-chain feature, but I'm not sure if this PR should be the one to introduce that feature (#![feature(let_chains)]) to the project. I can certainly write the code without let-chain's if you think this is a worthwile improvement to what I've already got here, and I'm happy to write up some additional tests to cover cases where we'd inform users of a missing foo/mod.rs file.

@calebcartwright
Copy link
Member

but I'm not sure if this PR should be the one to introduce that feature (#![feature(let_chains)]) to the project.

We've historically tried to stay away from using features, though I'm not entirely sure why to be honest. Not sure if I'm ready to take that plunge here, so let's keep that separate (can be addressed in a follow up PR if we decide to open that door)

Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

Will get back to regarding the potential introduction of the usage of gated features

@calebcartwright calebcartwright merged commit 18c0369 into rust-lang:master Mar 9, 2022
@ytmimi ytmimi deleted the issue_5167 branch August 7, 2022 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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