Add `future_prelude_collision` lint by jam1garner · Pull Request #85707 · rust-l...

Copy link

Contributor

rust-highfive commented on May 26

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @estebank (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

jam1garner commented on May 27

I believe all of the issues preventing a review have been resolved if you'd like to take a look or tag someone in @estebank!

nikomatsakis commented on May 27

r? @nikomatsakis

I can try to review this!

compiler/rustc_typeck/src/check/method/mod.rs

Outdated

Show resolved

m-ou-se commented on May 27

For some more test cases, try running cargo +yourversion fix on https://github.com/derekjw/try_from. That results in:

after fixes were automatically applied the compiler reported errors within these files:

  * src/char.rs
  * src/int.rs
  * src/lib.rs

This likely indicates a bug in either rustc or cargo itself,
and we would appreciate a bug report! You're likely to see 
a number of compiler warnings after this message which cargo
attempted to fix but failed. If you could open an issue at
https://github.com/rust-lang/rust/issues
quoting the full output of this command we'd be very appreciative!
Note that you may be able to make some more progress in the near-term
fixing code with the `--broken-code` flag

The following errors were reported:
error[E0107]: missing generics for trait `TryFrom`
  --> src/lib.rs:55:30
   |
55 |         let result = <u32 as TryFrom>::try_from("3");
   |                              ^^^^^^^ expected 1 generic argument
   |
note: trait defined here, with 1 generic parameter: `T`
  --> src/lib.rs:19:11
   |
19 | pub trait TryFrom<T>: Sized {
   |           ^^^^^^^ -
help: add missing generic argument
   |
55 |         let result = <u32 as TryFrom<T>>::try_from("3");
   |                              ^^^^^^^^^^

m-ou-se commented on May 27

It fails in another way on https://github.com/droundy/clapme:

after fixes were automatically applied the compiler reported errors within these files:

  * tests/nested.rs

<snip>

The following errors were reported:
error[E0433]: failed to resolve: use of undeclared crate or module `required_option`
  --> tests/nested.rs:31:10
   |
31 |         <required_option::SuperOpt as ClapMe>::from_iter(&["", "--arg-arg", "7", "--other", "hello"]).unwrap());
   |          ^^^^^^^^^^^^^^^ use of undeclared crate or module `required_option`
   
<snip>

(required_option is the name of the function the SuperOpt::from_iter call appears in.)

m-ou-se commented on May 27

(Sorry! Don't want to demotivate you! ^^' I'm just going over the crater results from a test where we added these traits to all the preludes, to see if this lint fixes the crates that broke.)

jam1garner commented on May 27

(Sorry! Don't want to demotivate you! ^^' I'm just going over the crater results from a test where we added these traits to all the preludes, to see if this lint fixes the crates that broke.)

No worries, I appreciate the thorough review actually, these are very much good finds, there's a reason we have the tooling to check the things that are error-prone :)

m-ou-se commented on May 27

Another interesting case is https://github.com/mechiru/json-ext

The lint does not trigger on that code.

But it should, because compiling that crate on edition 2021 (with the prelude changes) results in:

error[E0107]: this associated function takes 0 generic arguments but 1 generic argument was supplied
   --> src/object.rs:102:24
    |
102 |         let obj2 = obj.try_into::<Ext>()?;
    |                        ^^^^^^^^------- help: remove these generics
    |                        |
    |                        expected 0 generic arguments
    |
note: associated function defined here, with 0 generic parameters
   --> rust/library/core/src/convert/mod.rs:395:8
    |
395 |     fn try_into(self) -> Result<T, Self::Error>;
    |        ^^^^^^^^

Not sure why the new prelude affects this code though. try_into is a inherent method here, not a trait method.

m-ou-se commented on May 27

And another one: https://github.com/mqnfred/ffishim

The lint does also not trigger on this code.

But compiling it in the new edition results in:

error[E0061]: this function takes 0 arguments but 2 arguments were supplied
   --> ffishim/src/field.rs:40:44
    |
40  |             crate::types::switch(&self.ty).try_into(&self.ty, receiver)
    |                                            ^^^^^^^^ --------  -------- supplied 2 arguments
    |                                            |
    |                                            expected 0 arguments
    |
note: associated function defined here
   --> rust/library/core/src/convert/mod.rs:395:8
    |
395 |     fn try_into(self) -> Result<T, Self::Error>;
    |        ^^^^^^^^

Here, the receiver is a &Box<dyn Behavior>, where Behavior is a trait with a try_into function (taking two arguments).

m-ou-se commented on May 27

I think that's all I can find. Everything else seem to be the same issues as in the cases I already commented about.

nikomatsakis commented on May 27

<u32 as TryFrom<_>>::try_from("3"); would probably work

This comment has been hidden.

jam1garner commented on May 27

It fails in another way on https://github.com/droundy/clapme:

after fixes were automatically applied the compiler reported errors within these files:

  * tests/nested.rs

<snip>

The following errors were reported:
error[E0433]: failed to resolve: use of undeclared crate or module `required_option`
  --> tests/nested.rs:31:10
   |
31 |         <required_option::SuperOpt as ClapMe>::from_iter(&["", "--arg-arg", "7", "--other", "hello"]).unwrap());
   |          ^^^^^^^^^^^^^^^ use of undeclared crate or module `required_option`
   
<snip>

(required_option is the name of the function the SuperOpt::from_iter call appears in.)

It looks like this is because the way I was resolving the path via TyCtxt::def_path_str, which is intended for user output, so in the above case, the code is tests which are embed types with identical names inside. For disambiguating to the user, describing the type path being in a function "module", so that the 10 or so different types with the same name are distinct.

This comment has been hidden.

jam1garner commented on May 28 •

@m-ou-se when you have some time could you check if the warnings from the previous @rust-log-analyzer message are accurate? I took a look at them and they might be right? But I also wouldn't even know where to start with trying to compile rustc itself using the 2021 prelude, so I can't actually confirm if those are correct.

And if they're correct, I can resolve them.

This comment has been hidden.

jam1garner commented on May 28

Ok current status:

• json-ext is still broken, no lint firing. I still haven't gotten to diving into what causes the breakage in the first place.
• ffishim is still broken. Lint fires, but
  • It handles the boxed trait object fine, however TyCtxt::def_path_str isn't suitable for referencing inherent impls or renamed/duplicate types within a crate
    • inherent impl failing: field::<impl Field>::try_into(&*field)
    • duplicate type failing: types::Behavior::try_into(&**crate::types::switch(&self.ty))
      • This fails because types::Behavior describes a trait, but there's also 10 structs in the crate named Behavior, so the module name gets included

Point being: def_path_str is a poor fit. Sorry if I'm being a broken record but if anyone has any ideas where I should look for a replacement that takes into account what is imported at the given scope, it would help immensely, as I've spent an embarrassingly long amount of time looking for possibilities with no luck. One alternative is to output absolute paths to types, however this has the rather obvious downside of producing rather unappealing output and feels like throwing out the baby with the bathwater.

As far as I know, ignoring things that I don't think are my place to resolve (which group to add the lint to, linking to the edition guide), I believe these two issues are the last outstanding bits from what has been identified so far

This comment has been hidden.

This comment has been hidden.

This comment has been hidden.

jam1garner commented 11 days ago

Remaining issues:

Note: We should link to the edition guide once the 2021 stuff is online.

• The test failing (see the @rust-log-analyzer comment above): future-prelude-collision-shadow. This is because the test is made to ensure compile-fail maintains even after a rustfix run. However ui tests don't seem to support this. Should this test be removed or is there some way to test compile-file for rustfix'd UI tests?

I have gone back through all the crater issues @m-ou-se pointed out and rustfix seems to handle them all.

m-ou-se commented 10 days ago

Note: We should link to the edition guide once the 2021 stuff is online.

We need to do that for all the 2021 migraiton lints. We can just do that for all of them in a separate PR.

This comment has been hidden.

nikomatsakis commented 9 days ago

@bors r+

bors commented 9 days ago

Commit aa3580b has been approved by nikomatsakis

bors commented 9 days ago

Testing commit aa3580b with merge 44f4a87...

bors commented 9 days ago

Test successful - checks-actions
Approved by: nikomatsakis
Pushing 44f4a87 to master...