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

Improve out of line module resolution #5142

Merged
merged 1 commit into from
Jan 1, 2022

Conversation

ytmimi
Copy link
Contributor

@ytmimi ytmimi commented Dec 20, 2021

Fixes #5119

When the directory structure was laid out as follows:

dir
 |---mod_a
 |    |---sub_mod_1.rs
 |    |---sub_mod_2.rs
 |---mod_a.rs

And mod_a.rs contains the following content:

mod mod_a {
    mod sub_mod_1;
    mod sub_mod_2;
}

rustfmt previously tried to find sub_mod_1.rs and sub_mod_2.rs
in ./mod_a/mod_a/. This directory does not exist and this caused
rustfmt to fail with the error message:

Error writing files: failed to resolve mod

Now, both sub_mod_1.rs and sub_mod_2.rs are resolved correctly
and found at mod_a/sub_mod_1.rs and mod_a/sub_mod_2.rs.

@ytmimi ytmimi force-pushed the issue_5119 branch 2 times, most recently from 77feb51 to 38d9352 Compare December 20, 2021 05:34
@calebcartwright
Copy link
Member

Thanks for diving into this one! Few things:

  • There's a merge conflict now due the sync of the subtree, which ties back to Remove SymbolStr rust#91957 (tl;dr the borrow/deref sigils can be dropped now)
  • There's a number of other mod resolution issues and I'm somewhat paranoid about fixing one introducing another. We don't necessarily need to hold off on this, but if you have bandwidth before I get around to reviewing, it would be good to compare to some other issues to see whether they are fixed (and a true, non specious fix) as well as if any older "resolved" issues are impacted but not detected due to inadequate testing

@ytmimi
Copy link
Contributor Author

ytmimi commented Dec 21, 2021

I fixed the merge conflict!

I understand you wanting to be cautious about not introducing any module resolution issues. Last night I tried to add the a-mods label to a handful of issues that I thought were module resolution related. I'm sure there are plenty more that I missed, so any help pointing out the more relevant module resolution errors would be greatly appreciated.

I'm also happy to work on another PR to build up our tests around module resolution / work on other module resolution related issues.

As a side note, I don't think this fix will introduce any issues as the behavior is almost identical to how it was before, with the only difference being that we bail early if adding the next path component would lead to an invalid directory path.

@calebcartwright
Copy link
Member

As a side note, I don't think this fix will introduce any issues as the behavior is almost identical to how it was before, with the only difference being that we bail early if adding the next path component would lead to an invalid directory path.

The question/concern for me would be whether the early bail results in failing to discover/load valid mods.

I think one way to assuage that concern would be porting/replicating some of the existing tests (e.g. the path-based mod loads, cfg_if, etc.) and ensure we've got a test for them using the newer model leveraged here that more explicitly validates the module loading piece. That doesn't necessarily need to be done in this particular PR, but would probably want some of those to be done as well before a sync and release of this change

Fixes 5119

When the directory structure was laid out as follows:

```
dir
 |---mod_a
 |    |---sub_mod_1.rs
 |    |---sub_mod_2.rs
 |---mod_a.rs
```

And ``mod_a.rs`` contains the following content:

```rust
mod mod_a {
    mod sub_mod_1;
    mod sub_mod_2;
}
```

rustfmt previously tried to find ``sub_mod_1.rs`` and ``sub_mod_2.rs``
in ``./mod_a/mod_a/``. This directory does not exist and this caused
rustfmt to fail with the error message:

    Error writing files: failed to resolve mod

Now, both ``sub_mod_1.rs`` and ``sub_mod_2.rs`` are resolved correctly
and found at ``mod_a/sub_mod_1.rs`` and ``mod_a/sub_mod_2.rs``.
@ytmimi
Copy link
Contributor Author

ytmimi commented Dec 21, 2021

I went ahead and adjusted the current test case to incorporate both the #[cfg] and mod-rs to validate the module resolution. It would probably be a little cleaner to have each one of these scenarios be a separate test (and I'm happy to break it up), but this should help verify these changes.

@calebcartwright
Copy link
Member

I went ahead and adjusted the current test case to incorporate both the #[cfg] and mod-rs to validate the module resolution.

To clarify and elaborate, I was referring to some of these test scenarios https://github.com/rust-lang/rustfmt/tree/master/tests/source/cfg_if and https://github.com/rust-lang/rustfmt/tree/master/tests/source/cfg_mod

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.

Still want to poke at this a bit more later on, but should be fine for now (going to wait to merge til after the next sync though)

@calebcartwright calebcartwright merged commit 737e6f7 into rust-lang:master Jan 1, 2022
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.

Rustfmt cannot resolve submodules of integration tests
2 participants