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.

Sign up for GitHub

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

Jump to bottom

Clarify docs on implementing Into. #42126

Merged
merged 1 commit into from
May 31, 2017
Merged

Clarify docs on implementing Into. #42126

merged 1 commit into from
May 31, 2017

Conversation

clarfonthey
Copy link
Contributor

This was suggested by @dtolnay in #40380.

This explicitly clarifies in what circumstances you should implement Into instead of From.

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

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

@dtolnay
Copy link
Member

dtolnay commented May 20, 2017

This works when I tried it. https://play.rust-lang.org/?gist=29711851d1acb274cd4ba9fd3e05d781

struct Wrapper(String);

impl From<Wrapper> for String {
    fn from(w: Wrapper) -> String {
        w.0
    }
}

fn main() {}

@Mark-Simulacrum
Copy link
Member

Travis failed:

[01:13:49] failures:
[01:13:49]
[01:13:49] ---- convert.rs - convert::Into (line 178) stdout ----
[01:13:49]  thread 'rustc' panicked at 'test compiled while it wasn't supposed to', /checkout/src/librustdoc/test.rs:263
[01:13:49] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:13:49]
[01:13:49] ---- convert.rs - convert::Into (line 194) stdout ----
[01:13:49]  error[E0425]: cannot find value `w` in this scope
[01:13:49]  --> <anon>:7:9
[01:13:49]   |
[01:13:49] 7 |         w.0
[01:13:49]   |         ^ not found in this scope
[01:13:49]
[01:13:49] error[E0053]: method `into` has an incompatible type for trait
[01:13:49]  --> <anon>:6:5
[01:13:49]   |
[01:13:49] 6 | /     fn into(self) -> Wrapper {
[01:13:49] 7 | |         w.0
[01:13:49] 8 | |     }
[01:13:49]   | |_____^ expected struct `std::string::String`, found struct `main::Wrapper`
[01:13:49]   |
[01:13:49]   = note: expected type `fn(main::Wrapper) -> std::string::String`
[01:13:49]              found type `fn(main::Wrapper) -> main::Wrapper`
[01:13:49]
[01:13:49] error: aborting due to previous error(s)
[01:13:49]
[01:13:49] thread 'rustc' panicked at 'Box<Any>', /checkout/src/librustc/session/mod.rs:214
[01:13:49] failures:
[01:13:49]     convert.rs - convert::Into (line 178)
[01:13:49]     convert.rs - convert::Into (line 194)
[01:13:49]
[01:13:49] test result: FAILED. 1200 passed; 2 failed; 12 ignored; 0 measured; 0 filtered out

@Mark-Simulacrum Mark-Simulacrum added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 20, 2017
@frewsxcv frewsxcv added the A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools label May 22, 2017
steveklabnik
steveklabnik suggested changes May 22, 2017
View reviewed changes
src/libcore/convert.rs Outdated
/// not part of the current crate, then you can't implement `From` directly.
/// For example, take this crate:
///
/// ```{.compile_fail}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for the {}s

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed this

dtolnay
dtolnay requested changes May 22, 2017
View reviewed changes
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compile_fail code block actually compiles fine.

@shepmaster
Copy link
Member

@clarcharr a gentle reminder that this PR is awaiting your deft hand to fix the failures and respond to review feedback.

@clarfonthey
Copy link
Contributor Author

@shepmaster my deft hand has responded.

@Mark-Simulacrum
Copy link
Member

Looks like this still needs a few more updates to get Travis passing.

[01:04:37] failures:
[01:04:37] 
[01:04:37] ---- convert.rs - convert::Into (line 191) stdout ----
[01:04:37] 	error[E0053]: method `into` has an incompatible type for trait
[01:04:37]  --> <anon>:6:5
[01:04:37]   |
[01:04:37] 6 | /     fn into(self) -> Wrapper<T> {
[01:04:37] 7 | |         self.0
[01:04:37] 8 | |     }
[01:04:37]   | |_____^ expected struct `std::vec::Vec`, found struct `main::Wrapper`
[01:04:37]   |
[01:04:37]   = note: expected type `fn(main::Wrapper<T>) -> std::vec::Vec<T>`
[01:04:37]              found type `fn(main::Wrapper<T>) -> main::Wrapper<T>`
[01:04:37] 
[01:04:37] error: aborting due to previous error(s)
[01:04:37] 
[01:04:37] thread 'rustc' panicked at 'Box<Any>', /checkout/src/librustc/session/mod.rs:214
[01:04:37] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:04:37] 
[01:04:37] 
[01:04:37] failures:
[01:04:37]     convert.rs - convert::Into (line 191)
[01:04:37] 
[01:04:37] test result: FAILED. 1203 passed; 1 failed; 13 ignored; 0 measured; 0 filtered out

@clarfonthey
Copy link
Contributor Author

My deft hand failed me once again :(

(fixed the error)

Mark-Simulacrum
Mark-Simulacrum reviewed May 28, 2017
View reviewed changes
src/libcore/convert.rs Outdated
/// generic variable, then you can't implement `From` directly. For example,
/// take this crate:
///
/// ```.compile_fail
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for the . here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@clarfonthey
Copy link
Contributor Author

Rebased and made the changes.

@Mark-Simulacrum
Copy link
Member

r? @steveklabnik

@steveklabnik
Copy link
Member

@bors: r+ rollup

thanks!

@bors
Copy link
Contributor

bors commented May 30, 2017

📌 Commit 54bbe23 has been approved by steveklabnik

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 31, 2017
@Mark-Simulacrum
Rollup merge of rust-lang#42126 - clarcharr:into_docs, r=steveklabnik
d5a7fd5 
Clarify docs on implementing Into.

This was suggested by @dtolnay in rust-lang#40380.

This explicitly clarifies in what circumstances you should implement `Into` instead of `From`.
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request May 31, 2017
Merged
bors added a commit that referenced this pull request May 31, 2017
@bors
Auto merge of #42336 - Mark-Simulacrum:rollup, r=Mark-Simulacrum
e0cc22b 
Rollup of 7 pull requests

- Successful merges: #42126, #42196, #42252, #42277, #42315, #42329, #42330
- Failed merges:
@bors bors merged commit 54bbe23 into rust-lang:master May 31, 2017
@clarfonthey clarfonthey deleted the into_docs branch January 29, 2022 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtolnay dtolnay dtolnay requested changes

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum approved these changes

@steveklabnik steveklabnik steveklabnik approved these changes

Assignees

@steveklabnik steveklabnik

Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@clarfonthey @rust-highfive @dtolnay @Mark-Simulacrum @shepmaster @steveklabnik @bors @alexcrichton @frewsxcv