Skip to content

Warn that trait implementation is gated by a feature flag #87717

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

Open
edward-shen opened this issue Aug 3, 2021 · 3 comments
Open

Warn that trait implementation is gated by a feature flag #87717

edward-shen opened this issue Aug 3, 2021 · 3 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@edward-shen
Copy link
Contributor

Given the following code: (Playground link not provided because this involves a crate not on playground)

use rayon::iter::IntoParallelIterator;
use dashmap::DashSet;

#[derive(PartialEq, Eq, PartialOrd, Ord, Hash)]
struct Foo;

let test = DashSet::<Foo>::new();
test.par_iter();

The current output is:

error[E0599]: the method `par_iter` exists for struct `DashSet<Foo>`, but its trait bounds were not satisfied
  --> src/lib.rs:74:14
   |
74 |         test.par_iter();
   |              ^^^^^^^^ method cannot be called on `DashSet<Foo>` due to unsatisfied trait bounds
   | 
  ::: /home/edward/.cargo/registry/src/git.colasdn.top-1ecc6299db9ec823/dashmap-4.0.2/src/set.rs:19:1
   |
19 | pub struct DashSet<K, S = RandomState> {
   | -------------------------------------- doesn't satisfy `DashSet<Foo>: IntoParallelRefIterator`
   |
   = note: the following trait bounds were not satisfied:
           `&DashSet<Foo>: IntoParallelIterator`
           which is required by `DashSet<Foo>: IntoParallelRefIterator`

Ideally the output should look like:

error[E0599]: the method `par_iter` exists for struct `DashSet<Foo>`, but its trait bounds were not satisfied
  --> src/lib.rs:74:14
   |
74 |         test.par_iter();
   |              ^^^^^^^^ method cannot be called on `DashSet<Foo>` due to unsatisfied trait bounds
   | 
  ::: /home/edward/.cargo/registry/src/git.colasdn.top-1ecc6299db9ec823/dashmap-4.0.2/src/set.rs:19:1
   |
19 | pub struct DashSet<K, S = RandomState> {
   | -------------------------------------- doesn't satisfy `DashSet<Foo>: IntoParallelRefIterator`
   |
   = note: the following trait bounds were not satisfied:
           `&DashSet<Foo>: IntoParallelIterator`
           which is required by `DashSet<Foo>: IntoParallelRefIterator`
   = note: `dashmap` has an optional feature `rayon` that is currently not enabled.

I was trying to call par_iter() on the following example, but was very confused with regards to the error message. Consider the following image, snapshotted from the dashmap::DashSet docs:
image

It looks like Foo satisfies all the trait bounds needed for DashSet<Foo> to fufill InterParallelIterator, yet the trait still isn't being satisfied! What's going on? The solution is a case of either confusing docs.rs or a perhaps unintuitive diagnostic: a missing feature flag (in this case, rayon).

As a quick concept, I would suggest a note indicating that the trait implementation does exist for a struct but isn't enabled because the feature isn't enabled. As a layman on the rustc internals, I'm not sure how feasible this would be, but something like the following might be viable:

  1. On a failed trait bound check Trait,
  2. Check if struct Foo from crate crate implements Trait regardless of enabled features,
  3. If so, check if the Trait implementation or its parent mods is enabled through a feature, and that feature isn't enabled,
  4. If so, add a note saying that Trait is not satisfied because the trait implementation is gated by a feature flag.

I think this is worthwhile to implement, as I believe this is incredibly confusing to new Rust users. The compiler and docs make no indication that a trait implementation is gated, which means that a new user would be absolutely lost on next steps.
Heck, I would consider myself an intermediate or even an advanced user, and had to ask the community for help.

@edward-shen edward-shen added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 3, 2021
@SkiFire13
Copy link
Contributor

SkiFire13 commented Aug 3, 2021

I don't think the proposed approach would work. Feature gates implementations are filtered out when expanding macros, meaning their tokens are ignored, not just the higher-level concept of implementation. To make it work rustc would need to expand cfg macros regarless and track whether the code generated was cfged or not, however I would expect this to be a huge performance hit, not even mentioning those crates that put a lot of optional code behind feature flags to improve compile times in the first place.

However I think it would be possible to do something like:

  1. On a failed trait bound check Trait from crate foo,
  2. Check if crate bar in which the type Bar is defined has an optional dependency on crate foo,
  3. If so, suggest enabling that feature.

The advantage of this is that you would only need to track optional dependencies, without compiling feature gated code. The downside is that this might produce false negatives since there's no guarantee that with that optional dependency the implementation will be there.

I would also invest in improving rustdoc. In this case for example the dashmap crates defined rayon as an optional dependency, but not as an explicit feature. For cargo this means that there's a feature flag named rayon, but for rustdoc it doesn't exist, thus it doesn't show up in the docs. I think this is a big shortcoming in rustdoc that should addressed regardless of rustc's diagnostics.

@delbonis
Copy link

delbonis commented Aug 3, 2021

This might be a stretch but I feel like it could be convenient for a lot of users if there was some heuristic that's takes any traits that get filtered out by a cfg attr and put them to the side and if you try to implement a trait with the name but can't it gives you a hint saying "hey did you want this?", even if it's wrong. That would have false positives and false negatives but it could sort out a lot of oopses. Maybe it could be a clippy feature.

@nbdd0121
Copy link
Contributor

nbdd0121 commented Aug 3, 2021

For this to work a trait impl needs to participate in trait resolution, but cfg-gated code doesn't even have to typecheck, so I don't think it'll work.

Best way is probably to use doc_cfg.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants