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

Add inherent `try_from` and `try_into` methods to integer types #66852

Open
wants to merge 1 commit into
base: master
from

Conversation

@SimonSapin
Copy link
Contributor

SimonSapin commented Nov 28, 2019

This allows these methods to be used without importing the corresponding traits.

This causes new unused-imports warnings for existing imports used only by method calls. This is a non-trivial amount of churn, maybe worth it?

The warnings can be fixed by removing imports (like this PR does in rustc and tests), or by adding #[allow(unused_import)] to them until the minimum supported Rust version is incremented to one that has this PR.

The new methods are insta-stable in order to avoid causing unstable-name-collisions warnings. These would be harder to deal with since they happen at calls sites rather than trait imports.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Nov 28, 2019

r? @dtolnay

(rust_highfive has picked a reviewer for you, use r? to override)

@jonas-schievink

This comment has been minimized.

Copy link
Member

jonas-schievink commented Nov 28, 2019

Why can't we add the traits to the prelude instead? Do prelude imports still collide with non-prelude glob imports?

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

SimonSapin commented Nov 28, 2019

This is an idea for “consolation” for not having the TryFrom and TryInto traits in the prelude #49518 since that causes errors in crates that defined their own traits with same-name methods #49305 (comment). (I tried it again just now and it also happens in one of the dependencies of rustdoc.)

@rust-lang/libs what do you think?

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

SimonSapin commented Nov 28, 2019

@jonas-schievink It looks like this:

Building rustdoc for stage2 (x86_64-unknown-linux-gnu)
[…]
error[E0034]: multiple applicable items in scope
   --> /home/simon/.cargo/registry/src/github.com-1ecc6299db9ec823/minifier-0.0.33/src/css/token.rs:132:9
    |
132 |         Operator::try_from(*self).is_ok()
    |         ^^^^^^^^^^^^^^^^^^ multiple `try_from` found
    |
    = note: candidate #1 is defined in an impl of the trait `std::convert::TryFrom` for the type `_`
    = help: to disambiguate the method call, write `std::convert::TryFrom::try_from(...)` instead
note: candidate #2 is defined in the trait `css::token::MyTryFrom`
   --> /home/simon/.cargo/registry/src/github.com-1ecc6299db9ec823/minifier-0.0.33/src/css/token.rs:29:5
    |
29  |     fn try_from(value: T) -> Result<Self, Self::Error>;
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = help: to disambiguate the method call, write `css::token::MyTryFrom::try_from(...)` instead
@jonas-schievink

This comment has been minimized.

Copy link
Member

jonas-schievink commented Nov 28, 2019

Ah, I see. That's unfortunate then...

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Nov 28, 2019

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-11-28T21:30:02.9235768Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-11-28T21:30:02.9506568Z ##[command]git config gc.auto 0
2019-11-28T21:30:02.9581149Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-11-28T21:30:02.9639979Z ##[command]git config --get-all http.proxy
2019-11-28T21:30:02.9812294Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/66852/merge:refs/remotes/pull/66852/merge
---
2019-11-28T21:39:02.1071424Z configure: build.locked-deps    := True
2019-11-28T21:39:02.1071474Z configure: llvm.ccache          := sccache
2019-11-28T21:39:02.1071733Z configure: build.cargo-native-static := True
2019-11-28T21:39:02.1071973Z configure: dist.missing-tools   := True
2019-11-28T21:39:02.1072280Z configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
2019-11-28T21:39:02.1072402Z configure: writing `config.toml` in current directory
2019-11-28T21:39:02.1072446Z configure: 
2019-11-28T21:39:02.1072695Z configure: run `python /checkout/x.py --help`
2019-11-28T21:39:02.1072760Z configure: 
---
2019-11-28T21:40:51.6589988Z   local time: Thu Nov 28 21:40:51 UTC 2019
2019-11-28T21:40:51.8072491Z   network time: Thu, 28 Nov 2019 21:40:51 GMT
2019-11-28T21:40:51.8076351Z == end clock drift check ==
2019-11-28T21:41:00.1575275Z 
2019-11-28T21:41:00.1683211Z ##[error]Bash exited with code '1'.
2019-11-28T21:41:00.1713985Z ##[section]Starting: Checkout
2019-11-28T21:41:00.1715479Z ==============================================================================
2019-11-28T21:41:00.1715526Z Task         : Get sources
2019-11-28T21:41:00.1715569Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

This allows these methods to be used without importing the corresponding traits.

This causes new `unused-imports` warnings for existing imports used only by method calls. This is a non-trivial amount of churn, maybe worth it?

The warnings can be fixed by removing imports (like this PR does in rustc and tests), or by adding `#[allow(unused_import)]` to them until the minimum supported Rust version is incremented to one that has this PR.

The new methods are insta-stable in order to avoid causing `unstable-name-collisions` warnings. These would be harder to deal with since they happen at calls sites rather than trait imports.
@SimonSapin SimonSapin force-pushed the SimonSapin:inherent-try-from-into branch from 628b34f to da301ef Nov 29, 2019
@jhpratt

This comment has been minimized.

Copy link

jhpratt commented Nov 30, 2019

I'd rather add these traits to the prelude in the next edition. They're used increasingly more, and PRs like this show that there's definitely a reason to.

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

SimonSapin commented Nov 30, 2019

Been there, haven’t done that.

I proposed this (admittedly only a few months) before the 2018 edition and asked for help with implementing the new lint that would be required by the editions RFC. The Lang team “received [the idea] positively”, and then nothing happened: #51418

(That’s without considering that there aren’t any concrete plans for a future edition so far, and if there’s gonna be one it’s two or three years away at best.)

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Dec 2, 2019

I'd personally be in favor of a PR like this, seems like a neat strategy to me!

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

SimonSapin commented Dec 2, 2019

In that case, let’s check consensus for new insta-stable API:

@rfcbot fcp merge

And maybe do a check-only Crater run? To check for inference regression and get an idea of the volume of #[deny]’d warnings.

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 2, 2019

⌛️ Trying commit da301ef with merge e922e64...

bors added a commit that referenced this pull request Dec 2, 2019
Add inherent `try_from` and `try_into` methods to integer types

This allows these methods to be used without importing the corresponding traits.

This causes new `unused-imports` warnings for existing imports used only by method calls. This is a non-trivial amount of churn, maybe worth it?

The warnings can be fixed by removing imports (like this PR does in rustc and tests), or by adding `#[allow(unused_import)]` to them until the minimum supported Rust version is incremented to one that has this PR.

The new methods are insta-stable in order to avoid causing `unstable-name-collisions` warnings. These would be harder to deal with since they happen at calls sites rather than trait imports.
@dtolnay dtolnay added the T-libs label Dec 2, 2019
@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Dec 2, 2019

@SimonSapin oh I think T-libs was missing from before, I'll re-issue

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Dec 2, 2019

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 2, 2019

☀️ Try build successful - checks-azure
Build commit: e922e64 (e922e6444795ab795fab1943288aee3d543699b8)

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Dec 2, 2019

@craterbot check

@craterbot

This comment has been minimized.

Copy link
Collaborator

craterbot commented Dec 2, 2019

👌 Experiment pr-66852 created and queued.
🤖 Automatically detected try build e922e64
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@xfix

This comment has been minimized.

Copy link
Contributor

xfix commented Dec 4, 2019

I wonder if it's possible to modify the behavior of prelude so that methods from non-prelude traits would have precedence over prelude trait methods, similarly to how non-prelude items have a precedence over prelude items. As in, if there are two traits, and one of those is from prelude, instead of erroring out, it would use non-prelude method instead.

This isn't even specifically about this change, it would also allow doing stuff like adding methods from itertools crate into Iterator without causing unstable-name-collisions warnings, and so on.

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

SimonSapin commented Dec 4, 2019

I think that sounds like a good idea. Would you like to file an issue so it doesn’t get lost in here?

@craterbot

This comment has been minimized.

Copy link
Collaborator

craterbot commented Dec 6, 2019

🚧 Experiment pr-66852 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot

This comment has been minimized.

Copy link
Collaborator

craterbot commented Dec 8, 2019

🎉 Experiment pr-66852 is completed!
📊 2046 regressed and 1 fixed (81904 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@jhpratt

This comment has been minimized.

Copy link

jhpratt commented Dec 9, 2019

Looking at a couple logs, it appears as though the try_from crate no longer works when combined with the ? operator.

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

SimonSapin commented Dec 9, 2019

2.4% of regressions is way too much to manage, and I’m afraid most of them are legit.

I don’t think it’s particularly tied to ?.

I’ve looked at a few and they all appear to be using try_from or try_into methods from traits that are not those in std::convert. With this PR, the new inherent method gets chosen instead of the trait method. This would be fine if they behaved the same, but (part of) the return type (through the associated error type) is not always the same and we get errors.

This seems like it should have been predictable, but that’s always easier to say in retrospect :)

I’ll leave it open for a bit in case someone has some other idea or comment, but it looks like we can’t land this PR for pretty much the same reason we couldn’t add these traits to the prelude.

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Dec 11, 2019

It was worth trying this, but I agree that this is too many regressions to push this through. I think the only downside of not having this is people need to write the use std::convert::TryFrom import when they call try_from? That's not ideal but not something I mind too much.

As our IDE story improves this is going to get less and less annoying over time. I know in Java land, IDEs tend to be great at filling in practically all imports automatically.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Dec 11, 2019

The primary root regressions are as follows. One thought if we do decide to drive this forward is to get the try_from crate to re-export std's types on recent enough versions which would hopefully drastically reduce the amount of breakage here.

  • root: flate2-1.0.13: start v. end
  • root: foundationdb-0.3.0: start v. end
  • root: pagecache-0.19.4: start v. end
  • root: pmdk-0.5.1: start v. end
  • root: simd-json-0.1.26: start v. end
    root: stdweb - 193 (172 gh, 21 crates.io) detected crates which regressed due to this
    root: try_from - 1815 (988 gh, 827 crates.io) detected crates which regressed due to this
@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

SimonSapin commented Dec 11, 2019

If breakage like this could be acceptable (with changes to some key crates), then probably so is adding the traits to the prelude? Last time around there was some resistance to that idea, though: #49305 (comment)

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Dec 11, 2019

To be clear, I personally feel similarly to @dtolnay that adding the import is not that hard and we should not cause breakage just for that minor win. I mainly posted the primary root regressions so that folks were aware of the causes of the breakage, if someone wanted to go out and fix crates in the ecosystem.

@xfix

This comment has been minimized.

Copy link
Contributor

xfix commented Dec 11, 2019

I don't think this could be done without something like the idea I proposed before in this issue (but I'm still not convinced whether it is really worth it complicating the language, so I didn't make a separate issue for it).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.