Experiment proposal: `#[discourage_direct_import]` attribute

[nik-rev]

Experimental feature gate: #![feature(discourage_direct_import)]

Previous form of proposal (for archive purposes only)

I propose to add a new lint for making imports in codebases more consistent, which fires when items are imported inconsistently.

For example, many items in the std::fs, std::io, std::iter, std::env and other modules are conventionally referred to via the parent path:

use std::fs;
use std::fmt;
use std::io;

fs::read("Cargo.toml")?;

impl fmt::Write for Struct { ... }

fn save_to_disk(file: &str) -> io::Result<()> { ... }

It is discouraged to import those items directly:

use std::fs::read;
use std::fmt::Write;
use std::io::Result; // shadows std::result::Result, causing confusion

read("Cargo.toml")?;

// is this io::Write or fmt::Write?
impl Write for Struct { ... }

// shadows std::result::Result, causing confusion
fn save_to_disk(file: &str) -> Result<()> {

To enforce this style, there is a new attribute #[import_style]. This attribute configures the new wrong_import_style_via_super warn-by-default lint.

The #[import_style] attribute tells how others should import your item.

For example, we can annotate fs::read with it:

[!NOTE]

All syntax in this proposal is just placeholder.

// std::fs
#[import_style(via = super)]
fn read() {}

When someone imports read directly, the lint wrong_import_style_via_super fires:

use std::fs::read;
//           ^^^^ do not directly import
//                instead, `use std::fs` then refer as `fs::read`

The basic MVP would be to implement #[import_style(via = super)], with no further configuration.

Extensions

The rest of this proposal goes into how this feature can be further extended to cover other common patterns.

Specifically, the use-cases the extensions support are:

• Items imported directly

  use rustc_middle::ty::Ty commonly is referred to directly as Ty.

  We should allow linting other ways of referring to Ty, for consumer
  crates, the current crate, or a specific module.

• Types imported , but the module name should be renamed. Like use rustc_hir as hir, then refer as hir::Ty. Other ways of referring are linted.

• Override import style for external paths

• Ignore the import style for external path

The proposal provides a bird's eye view of the feature, there are many details left out that can be figured out during the implementation.

Feature: Direct import

We can require that the item is imported directly. Let's say we want to require that rustc_middle::ty::Ty is always used as Ty, with no path segments:

// rustc_middle::ty

#[import_style(via = self)]
struct Ty;

Then not using the item directly triggers the lint wrong_import_style_via_self:

use rustc_middle::ty;
// +                ::Ty
//                  import `Ty` directly

fn f(_: ty::Ty) {}
//      ^^^^ wrong import style, `Ty` should be
//           imported directly.

The following is required:

use rustc_middle::ty::Ty;

fn f(_: Ty) {}

The wrong_import_style_via_self lint is allow-by-default
to prevent libraries from polluting the user's scope.

Feature: Renaming

The import_style field also accepts an as key, which requires the item to be renamed when it is imported directly.

For example:

// syn::parse

#[import_style(via = self, as = SynParse)]
trait Parse {

All of the following usage is wrong:

// refer as `syn::parse::Parse`
use syn::parse; // then refer as `parse::Parse`
use syn::parse::Parse; // then refer as Parse

You must import Parse directly, then rename it:

use syn::parse::Parse as SynParse

If via = self is omitted, then Parse needs to be renamed only when imported directly.

This cannot be used with via = super.

Feature: Configure external items

We have an issue with paths that are too awkward:

impl syn::parse::Parse for RenameRule {

• syn::parse::Parse is too long to type, and contains duplication in "parse".
• use syn::parse, then refer as parse::Parse looks odd + not clear that it's from syn
• use syn::parse::Parse then refer as Parse has large potential to conflict with other types, it's also not clear that the type is from syn

Given that, one may reasonably require the following style in their codebase:

use syn::parse::Parse as SynParse;

impl SynParse for RenameRule {

To enforce this, the syn crate could add import_style attribute to Parse:

#[import_style(via = self, as = SynParse)]
trait Parse {

But they might not do that, and we still want to enforce that style in our codebase.

To accomodate this, we can configure the import style for other items:

#![import_style(for = syn::parse::Parse, via = self, as = SynParse)]

This works much like a #[warn]:

• On functions, sets the import style within the body
• On modules, sets the import style for the item to all sub-items recursively
• On the crate root, sets the import style for the whole crate

Ignoring

It may also be desireable to ignore the import style of specific items, which can be done with ::ignore:

#![import_style::ignore(std::fs::read)]

That accepts a list of paths. For any path that is ignored, all sub-items in that path is also ignored.

This ignores all #[import_style] attributes on items in std::io and std::fs:

#![import_style::ignore(std::fs, std::io)]

In which case this no longer triggers the wrong_import_style_via_super lint:

use std::fs::read;

read("Cargo.toml"); // OK

I propose to add a new lint for making imports in codebases more consistent, which fires when certain items are imported directly.

For example, many items in the std::fs, std::io, std::iter, std::env and other modules are conventionally referred to via the parent path:

use std::fs;
use std::fmt;
use std::io;

fs::read("Cargo.toml")?;

impl fmt::Write for Struct { ... }

fn save_to_disk(file: &str) -> io::Result<()> { ... }

It is discouraged to import those items directly:

use std::fs::read;
use std::fmt::Write;
use std::io::Result; // shadows std::result::Result, causing confusion

read("Cargo.toml")?;

// is this io::Write or fmt::Write?
impl Write for Struct { ... }

// shadows std::result::Result, causing confusion
fn save_to_disk(file: &str) -> Result<()> {

To enforce this style, there is a new attribute #[discourage_direct_import]. This attribute configures the new discouraged_direct_imports warn-by-default lint.

The #[import_style] attribute tells how others should import your item.

For example, we can annotate fs::read with it:

// std::fs
#[discourage_direct_import]
fn read() {}

When someone imports read directly, the lint wrong_import_style_via_super fires:

use std::fs::read;
//           ^^^^ do not directly import
//                instead, `use std::fs` then refer as `fs::read`

Another example use-case: One of my code bases has 80 types named Request, each in their individual modules. This API is meant to be used via a full type to Request, you'd never import it directly.

Contacts

• Design: @nik-rev
• Implementation: @nik-rev
• Lang liaison: @joshtriplett

[kennytm]

I don't think it is a good idea for the definer to decide on the coding style of the consumer.

For #[import_style(via = self, as = "...")] the library user (not library author) can configure enforced-import-renames in Clippy.

enforced-import-renames = [
  { path = "syn::parse::Parse", rename = "SynParse" },
]

seems no existing lint for the user to check the other variants though.

[nik-rev]

@kennytm

Thanks, I wasn't aware of that lint

I originally proposed this idea as a Clippy lint in https://rust-lang.zulipchat.com/#narrow/channel/257328-clippy/topic/New.20lint.3A.20Discourage.20Direct.20Imports.20of.20Marked.20Items/with/583861442, which received interest from users.

The idea was much simpler, though:

#[clippy::discourage_direct_import]

This proposal fleshes it out a bit more, but the core is still the same. The "initial MVP" is still the same idea. In my opinion that's the most important part to implement.

Since we already have a clippy lint for the "Rename" part, we could likely drop that from this proposal. An earlier draft of this also suggested as = _ rename for traits that are unused, but due to it being an existing clippy lint I dropped that

There was interest in having something like this built-in into rustc rather than clippy. So I asked if it is something that we could do an experiment for.

See also this thread: https://rust-lang.zulipchat.com/#narrow/channel/213817-t-lang/topic/Vibe.20check.3A.20lint.20on.20direct.20imports/with/583866054

I don't think it is a good idea for the definer to decide on the coding style of the consumer.

You can do both. The primary motivation for this is having a warn-by-default lint when someone does use std::io::read, for example. It could be perma-unstable, for use by the standard library only

[kennytm]

The primary motivation for this is having a warn-by-default lint when someone does use std::io::read, for example.

The point is that I don't want libstd to dictate how to properly fix use std::fs::read, e.g. both of the following are found in the wild and I think they are both valid disambiguation techniques:

use std::{io, fs};
fn f() -> io::Result<Vec<u8>> {
  fs::read("1.bin")
}

use std::fs::read as read_file;
use std::io::Result as IoResult;
fn f() -> IoResult<Vec<u8>> {
  read_file("1.bin")
}

By letting the library (libstd) decide on the via =, users of one of the formatting style can only ignore/#[allow] the whole thing 🤷. So IMO allowing the library to declare #[must_not_use_directly] is the furthest acceptable, suggesting via = would be overstepping.

On the user side, Clippy can add an additional prefer-qualified-usage lint, pre-populated with the common confusable identifiers like std::ptr::read etc, and also allowing user to configure more items e.g. serde_json::to_string. But if this is entirely under user's control there seems no need to have the attribute in the first place.

[scottmcm]

I don't think this belongs in the top-level attribute namespace.

If it's going to happen, I think diagnostics::something or rustfmt::something or clippy::something (or whatever) would be better, since it doesn't have any effect on the output, let alone have any semantics.

---

use std::io::Result; // shadows std::result::Result, causing confusion

OTOH, https://docs.rs/anyhow/latest/anyhow/type.Result.html is intended to be used by shadowing Result.

[Veykril]

This could also be a rust_analyzer tool attribute, if this is mainly about instructing rust-analyzer to give this a specific import style. Personally this feels wrong to be shaped as a diagnostic as it's more a style concern? It could fit clippy given it has a style category though.

[kennytm]

• #[diagnostics::something] — based on RFC 3368's text, the purpose of the attribute namespace is "to influence error messages emitted by the compiler", while this proposal is about producing a new lint, so it does not fit at all.

• #[rustfmt::something] — this suggested the scope is formatting-related. Unless this means (only) cargo fmt will automatically rewrite to the desired import style (or at least produce an error/lint if the style does not match), this proposal does not belong.

• #[clippy::something] — that's the original proposal according to Experiment proposal: #[discourage_direct_import] attribute #373 (comment), but chat encouraged making the lint built-in so we're here 😅.

• #[rust_analyzer::something] — pretty sure the underlying concern (discouraging use std::fs::read;) is beyond LSP, placing it in rust_analyzer::* would be too narrow.

I still think a new Clippy lint + configurable ban list would be the best way forward.

[joshtriplett]

So IMO allowing the library to declare #[must_not_use_directly] is the furthest acceptable

For what it's worth, I think this is the most useful aspect as well. I don't think we need all the variations, I primarily want to see something that says "don't import this directly". Also, I think you're right that it should suppress the lint if you import the item directly but rename it.

[nik-rev]

In light of this discussion, I'm changing this proposal to only propose #[discourage_direct_import], with zero further configuration.

[demosdemon]

This feels like a library is trying to encourage or even force a specific code style on its consumers. Why would a library want or even need that?

[scottmcm]

I'm changing this proposal to only propose #[discourage_direct_import], with zero further configuration.

I'm still against it being in the top-level namespace. It should be somewhere where we could entirely omit it from the rust spec.

[nik-rev]

@demosdemon

Why would a library want or even need that?

The primary motivators are APIs in std like in std::fs and std::io, and also APIs that you own yourself, and want to discourage importing items in a certain way. Example from the post:

Another example use-case: One of my code bases has 80 types named Request, each in their individual modules. This API is meant to be used via a full type to Request, you'd never import it directly.

Another example is from RfL:

RfL also has several Device types that should be used via the parent module {pwm,platform,phy,pci,drm,dma,device,auxiliary}::Device

---

This feels like a library is trying to encourage or even force a specific code style on its consumers.

There are N ways to import an item. This lint doesn't allow libraries to say "There is only 1 way to import this item", instead it means "There are N - 1 ways to import this item". So there is no forcing of a specific style. These are the ways the lint could be silenced:

// renaming silences
use std::fs::read as fs_read;

// importing parent module silences
use std::fs; fs::read();

// renaming parent silences
use std::fs as filesystem; filesystem::read();

// can be used via absolute path
std::fs::read();

#[allow(discouraged_direct_imports)]
use std::fs::read;

[nik-rev]

@scottmcm

I'm still against it being in the top-level namespace. It should be somewhere where we could entirely omit it from the rust spec.

I would be happy with that, but it's not clear where it should live. diagnostic seems the most appropriate, but that was suggested before and:

That is not what the diagnostic namespace is for. It should probably need to be its own thing like #[must_use] and #[must_not_suspend]

Which makes sense, considering:

This RFC proposed to add a stable #[diagnostic] attribute namespace, which contains attributes to influence error messages emitted by the compiler

This creates new diagnostics, rather than influencing existing diagnostics.

It's similar in spirit to #[must_use] so it could also make sense to live in the top-level namespace for that reason.

[kennytm]

It should be somewhere where we could entirely omit it from the rust spec.

well the only valid namespace for now would be #[clippy::discourage_direct_import] as I've mentioned before #373 (comment).

[kanashimia]

This goes contrary to the way current imports are suggested by rustc and rust-analyzer, so that requires consideration too if this is expected to be a part of rustc.

Also just my opinion but I don't think specifically stuff like use std::fs or std::io is generally "good" either, that is only saving 5 characters per item, but makes it confusing compared to absolute if you work with oh so popular tokio and then people do use std::fs in one file and use tokio::fs in another as an example, and so you still have to worry about renames and conflicts [1].
And when people do that in the code snippets on the internet it is something that just makes it more annoying to copy-paste parts.

[^1] Details

It is pretty personal and not a status quo, but what I also like to do is to use a module, custom prelude basically, which is global per project and contains majority of the imports and module aliases, conflicts are resolved in one place. It really eases refactoring and readability, as with that use almost aren't a concern at all anymore, copy-pasting a function from one file to another is trivial.

I think this makes more sense to have as a part of clippy, as was suggested.
They aready have a very similar lint, though one that is the opposite of being useful: clippy::restriction::absolute_paths