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

Feature request: imports_scope #6123

Open
jvcmarcenes opened this issue Mar 22, 2024 · 4 comments
Open

Feature request: imports_scope #6123

jvcmarcenes opened this issue Mar 22, 2024 · 4 comments

Comments

@jvcmarcenes
Copy link

jvcmarcenes commented Mar 22, 2024

This option configures if imports are placed at the top of a module, or if they are scoped to a block if possible.

Motivation

Rust allows for imports to be scoped to either a module or a block (reference).

It is standard practice to place imports bundled at the top of modules. This is also common in other programming languages (that often don't have another choice). However, some users (me) prefer to not leave all their imports at the top of their modules.

I believe we've persisted the "imports-at-the-top" practice simply because many developers are coming from other languages where having a scoped import wasn't possible. It doesn't help that tooling that performs auto-imports also places imports at the top of the module.

Module imports are often the first thing you see when reading a module. They clutter the first lines of a file with information that will only be relevant later. I can't say I've met a single developer who would read through the imports of a module before the items it defines.

Module imports can also lead to confusion as there is no visual connection between the imported item and the code that references it. Module imports are also more prone to suffer from import collisions.

Scoped imports also present a better developer experience when moving code between modules, as it moves the necessary "dependencies" with it. When moving code that uses items imported at the module it is necessary to add the imports to the target module and remove the unused dependencies from the old module.

Options

"ModuleAlways"

Moves all imports to the top of the nearest parent module.

use std::collections::{Vec, HashSet, HashMap};

fn foo<T>(_vec: Vec<T>) {
    let _map = HashMap::<T>::new();
    let _set = HashSet::<T>::new();
}


fn bar<T>() {
    let _map = HashMap::<T>::new();
}

"ExclusiveInScope"

If an imported item is used exclusively within a single block in a module, its use statement is scoped to the block.

use std::collections::{Vec, HashMap};

fn foo<T>(_vec: Vec<T>) {
    use std::collections::HashSet;

    let _map = HashMap::<T>::new();
    let _set = HashSet::<T>::new();
}

fn bar<T>() {
    let _map = HashMap::<T>::new();
}

"AlwaysScoped"

If an imported item is referenced in a block, the imports are placed inside the block.

use std::collections::Vec;

fn foo<T>(_vec: Vec<T>) {
    use std::collections::{HashSet, HashMap};

    let _map = HashMap::<T>::new();
    let _set = HashSet::<T>::new();
}

fn bar<T>() {
    use std::collections::HashMap;

    let _map = HashMap::<T>::new();
}

"Mixed" (Default)

What it is today, respects where users write their use statements.

Additional Considerations

Naming

It's possible there are better names for both the configuration and its options. I haven't put too much thought into these names. At the moment their job is merely to illustrate the problem.

Specifying the exclusive import threshold

If configured to "BlockExclusive", an additional configuration could be used to determine how many times an import has to be used across a module for it to be placed at the module level.

Fully qualified names on items that don't support imports

An additional configuration could remove module level imports in favor of fully qualified names (maybe with a threshold for path size).

fn foo<T>(_vec: std::collections::Vec<T>) {
    use std::collections::{HashSet, HashMap};

    let _map = HashMap::<T>::new();
    let _set = HashSet::<T>::new();
}

fn bar<T>() {
    use std::collections::HashMap;

    let _map = HashMap::<T>::new();
}
@ytmimi
Copy link
Contributor

ytmimi commented Mar 22, 2024

@jvcmarcenes thanks for reaching out. I want to be up front and say this isn't a request the team will work on directly. I'll leave the issue open for now in case a motivated contributor comes along and wants to try implementing this.

In the meantime, could you share more about your motivation for this feature request?

@jvcmarcenes
Copy link
Author

jvcmarcenes commented Mar 22, 2024

Of course, sorry for not including it to start with. If the options I presented were available I would personally use "FunctionAlways".

I find that leaving imports inside functions instead of modules in general leads to a better developer experience for the following reasons:

  • I care more about the dependencies of a function than of a whole module. The information of "what functions/modules a function depends on" is often more relevant to me than "what functions/modules do functions inside a module depend on". Leaving the imports in the function means it is closer to where it gets used;
  • It leads to visual clutter. While working on large projects it's common for modules to accumulate dozens of imports, and some of those imports might only be used once or twice;
  • It's easier to move the function during a refactor to a different module. If all imports are at the top you have to go through and add all the necessary imports to the new module and then remove them from the old module;

However now that I think about it "FunctionAlways" probably isn't the best name, we could want to use imported types inside type declarations or function signatures, and these imports would need to be at the module level.

"ItemExclusive" and "ItemAlways" might be a better fit, and the item will either be a module or a function.

I have the habit of depending on RA's code actions for automatic imports (And I have opened an issue on RA about where auto-imports are placed too). And reformatting them closer to their usages would help me avoid manually writing the imports.

I feel "ModuleAlways" is necessary given that it seems to be the norm for import placement.
"ItemExclusive" is a compromise between "ModuleAlways" and "ItemAlways", that would declutter single-usage imports from the module level.
And "ItemAlways" would be the option for users like me (I have not done any research to know if I'm alone here).

The imports-first discussion to me feels very similar to the declarations-first discussion. It was common when writing functions to leave all variable declarations at the beginning and only later initialize/use them. However, nowadays we implicitly agree that declaring variables when we need them is better.

Of course, it's different with imports, since they don't have to deal with initialization, which might have been the biggest issue with declarations-first. But some of the ergonomic benefits of declarations-when-needed are also relevant for imports.

I'd be willing to try and solve this issue, but I have no experience with the rustfmt codebase. I might try picking it up whenever I have some better availability.

@jvcmarcenes
Copy link
Author

I've improved the issue body, it should be easier to understand this feature request now.

@ytmimi
Copy link
Contributor

ytmimi commented Mar 22, 2024

I also want to note that some of the requests aren't feasible given the limited information availabe in the AST rustfmt uses to reformat your program. For example, rustfmt doesn't do any name resolution so when it sees the type Vec<T> in a function signature it can't know for sure that you're referring to std::collections::Vec<T>. Trying to make that transformation could cause compilation errors or change the semantics of the code if the user was using a different type also named Vec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants