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

impl ToSocketAddrs for String #39048

Merged
merged 1 commit into from
Jan 24, 2017

Conversation

lambda
Copy link
Contributor

@lambda lambda commented Jan 13, 2017

ToSocketAddrs is implemented for a number of different types,
including (IpAddr, u16), &str, and various others, for the
convenience of being able to run things like
TcpListener::bind("10.11.12.13:1415"). However, because this is a
generic parameter with a trait bound, if you have a String you cannot
pass it in, either directly as TcpListener::bind(string), or the
TcpListener::bind(&string) as you might expect due to deref coercion;
you have to use TcpListener::bind(&*string), which is noisy and hard
to discover (though #39029 suggests better error messages to make it
more discoverable).

Rather than making people stumble over this, just implement
ToSocketAddrs for String.

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

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

@brson
Copy link
Contributor

brson commented Jan 13, 2017

Hm, this doesn't seem like a 'to' conversion, but an 'into' conversion. I'm trying to think if this would make it easy to accidentally write less efficient code, but I can't think how.

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jan 13, 2017
@alexcrichton
Copy link
Member

Looks good to me, thanks! Impls like this are actually instantly stable so we just go ahead and tag them with a #[stable] rather than #[unstable]. Feel free to just pick a feature name out of thin air for that!

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Jan 14, 2017

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

No concerns currently listed.

Once these reviewers reach consensus, 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.

@alexcrichton
Copy link
Member

@brson hm I'm not sure that there's any resources that the return value of ToSocketAddrs can share with String (e.g. some memory buffer). In general perf probably isn't too much of a worry here b/c you're doing a DNS lookup as well.

@lambda lambda force-pushed the impl-tosocketaddrs-for-string branch from c010d36 to ac73ba4 Compare January 14, 2017 00:12
`ToSocketAddrs` is implemented for a number of different types,
including `(IpAddr, u16)`, `&str`, and various others, for the
convenience of being able to run things like
`TcpListener::bind("10.11.12.13:1415")`.  However, because this is a
generic parameter with a trait bound, if you have a `String` you cannot
pass it in, either directly as `TcpListener::bind(string)`, or the
`TcpListener::bind(&string)` as you might expect due to deref coercion;
you have to use `TcpListener::bind(&*string)`, which is noisy and hard
to discover (though rust-lang#39029 suggests better error messages to make it
more discoverable).

Rather than making people stumble over this, just implement
`ToSocketAddrs` for `String`.
@lambda lambda force-pushed the impl-tosocketaddrs-for-string branch from ac73ba4 to a5f2f36 Compare January 14, 2017 00:13
@lambda
Copy link
Contributor Author

lambda commented Jan 14, 2017

This is actually a "to" because it takes &self. It happens that if you use this as a generic argument like TcpListener::bind does, that will consume the string (as I demonstrate in the test cases), but I think that's generally OK, and it allows you to pass &string in as well if you don't want to consume it.

With this patch, any of the following should work (using TcpStream::connect as an example since it's a better example of when you may want to use this, if you get a hostname and port from different sources and need to build a string from them).

TcpStream::connect("hostname:42");  // Literal &str
TcpStream::connect(&*format!("{}:{}", host, port));  // String, the old way
TcpStream::connect(format!("{}:{}", host, port));  // String, the new way, consumes the string
let destination = format!("{}:{}", host, port);
TcpStream::connect(&destination);  // String, the new way, by reference, if you don't want to consume

I don't think there are any performance implications, just one way that you might consume the string by accident which you can fix by inserting a single & like you'd expect.

@lambda
Copy link
Contributor Author

lambda commented Jan 14, 2017

Also pushed new version with stability marked, as suggested by @alexcrichton.

@xen0n
Copy link
Contributor

xen0n commented Jan 14, 2017

Hmm, this makes writing things like TcpStream::connect(format!("{host}:{port}")) much easier, which I'd rather not see happen, but given the other examples shown above we can't stop people from doing similarly wrong things either. I'm fine with the addition then.

@golddranks
Copy link
Contributor

I found allowing passing of the ownership bad design at first. But then again, @alexcrichton had a good point about the DNS lookup dominating perf.

Besides, if you are using a String that you don't need an ownership of in the future, the odds are that you've built that string for that occasion, for passing that into a function, and just have no use for it afterwards.

If you have an use for it, unless you are a total newbie, you are going to prefer passing a reference over cloning it, and if you are a total newbie, that's not likely to be the most pressing problem in your code.

So, after all, the downsides don't seem too dire, and it's an ergonomics win.

@lambda
Copy link
Contributor Author

lambda commented Jan 14, 2017

It is possible to switch this to an impl on &String, but I feel that the ergonomics win is better for String so it works all of the ways you might try.

@tbu-
Copy link
Contributor

tbu- commented Jan 14, 2017

I share @llogiq's concern:

I'm not keen on having that impl consuming the String. It will lead to more confusing errors down the line when trying to re-use the String.

Can we impl ToSocketAddrs for &String instead?

llogiq on /r/rust

@golddranks
Copy link
Contributor

golddranks commented Jan 14, 2017

Consuming String and trying to use it later will lead to "use of moved value" error, whereas passing String where &String is expected will lead to "the trait ... is not implemented for std::string::String" error. Both are quite helpful. (Where the move and later use happens is shown, and the fact that an implementation exists for &String is shown.)

Which is more confusing? After considering the the fact that format!({}:{}, addr, port) might be quite common in the argument position where ToSocketAddrs is expected, I'd be inclined to implement for String. Edit: is it? There's an implementation for (&'a str, u16)...

@tbu-
Copy link
Contributor

tbu- commented Jan 14, 2017

In this case, you'd be better off passing (addr, port), e.g. ("github.com", 80) instead of format!("{}:{}", "github.com", 80).

@xen0n
Copy link
Contributor

xen0n commented Jan 15, 2017

@tbu- this is more about helping Rust newcomers than pursuing elegance, as several others have pointed out.

@tbu-
Copy link
Contributor

tbu- commented Jan 15, 2017

@xen0n I'm aware of that, and not opposed to adding a &String implpementation. Does it help if we promote slow solutions though? It'll lead to code like the format! above (where a universally faster solution exists) or like let s = String::from("github.com:80"); tcp.connect(s.clone()); where &s would be much more useful.

If we want such a change of string ideology, we should probably decide about that globally and not on a case-by-case basis, because it'll lead to an inconsistent standard library otherwise. E.g.: why does connect accept String, but I can't do a String + String? Would also be easier for newcomers.

@golddranks
Copy link
Contributor

Comparison to String + String concatenation is a good point :)

@llogiq
Copy link
Contributor

llogiq commented Jan 15, 2017

I personally feel that helpful suggestions from the compiler are going to be more useful in the long run than clogging up std with impls for everything and the kitchen sink.

@alexcrichton
Copy link
Member

ping @BurntSushi, @brson, @sfackler (checkboxes)

@brson
Copy link
Contributor

brson commented Jan 20, 2017

After looking at it again I don't have any particular concerns, and it seems like most comments on thread are either pro or meh, not a lot of total opposition.

@rfcbot
Copy link

rfcbot commented Jan 23, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Jan 23, 2017
@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Jan 23, 2017

📌 Commit a5f2f36 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Jan 24, 2017

⌛ Testing commit a5f2f36 with merge 18b6b8f...

bors added a commit that referenced this pull request Jan 24, 2017
…ichton

impl ToSocketAddrs for String

`ToSocketAddrs` is implemented for a number of different types,
including `(IpAddr, u16)`, `&str`, and various others, for the
convenience of being able to run things like
`TcpListener::bind("10.11.12.13:1415")`.  However, because this is a
generic parameter with a trait bound, if you have a `String` you cannot
pass it in, either directly as `TcpListener::bind(string)`, or the
`TcpListener::bind(&string)` as you might expect due to deref coercion;
you have to use `TcpListener::bind(&*string)`, which is noisy and hard
to discover (though #39029 suggests better error messages to make it
more discoverable).

Rather than making people stumble over this, just implement
`ToSocketAddrs` for `String`.
@bors
Copy link
Contributor

bors commented Jan 24, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 18b6b8f to master...

@bors bors merged commit a5f2f36 into rust-lang:master Jan 24, 2017
jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Mar 20, 2017
Version 1.16.0 (2017-03-16)
===========================

Language
--------

* Lifetimes in statics and consts default to `'static`. [RFC 1623]
* [The compiler's `dead_code` lint now accounts for type aliases][38051].
* [Uninhabitable enums (those without any variants) no longer permit wildcard
  match patterns][38069]
* [Clean up semantics of `self` in an import list][38313]
* [`Self` may appear in `impl` headers][38920]
* [`Self` may appear in struct expressions][39282]

Compiler
--------

* [`rustc` now supports `--emit=metadata`, which causes rustc to emit
  a `.rmeta` file containing only crate metadata][38571]. This can be
  used by tools like the Rust Language Service to perform
  metadata-only builds.
* [Levenshtein based typo suggestions now work in most places, while
  previously they worked only for fields and sometimes for local
  variables][38927]. Together with the overhaul of "no
  resolution"/"unexpected resolution" errors (#[38154]) they result in
  large and systematic improvement in resolution diagnostics.
* [Fix `transmute::<T, U>` where `T` requires a bigger alignment than
  `U`][38670]
* [rustc: use -Xlinker when specifying an rpath with ',' in it][38798]
* [`rustc` no longer attempts to provide "consider using an explicit
  lifetime" suggestions][37057]. They were inaccurate.

Stabilized APIs
---------------

* [`VecDeque::truncate`]
* [`VecDeque::resize`]
* [`String::insert_str`]
* [`Duration::checked_add`]
* [`Duration::checked_sub`]
* [`Duration::checked_div`]
* [`Duration::checked_mul`]
* [`str::replacen`]
* [`str::repeat`]
* [`SocketAddr::is_ipv4`]
* [`SocketAddr::is_ipv6`]
* [`IpAddr::is_ipv4`]
* [`IpAddr::is_ipv6`]
* [`Vec::dedup_by`]
* [`Vec::dedup_by_key`]
* [`Result::unwrap_or_default`]
* [`<*const T>::wrapping_offset`]
* [`<*mut T>::wrapping_offset`]
* `CommandExt::creation_flags`
* [`File::set_permissions`]
* [`String::split_off`]

Libraries
---------

* [`[T]::binary_search` and `[T]::binary_search_by_key` now take
  their argument by `Borrow` parameter][37761]
* [All public types in std implement `Debug`][38006]
* [`IpAddr` implements `From<Ipv4Addr>` and `From<Ipv6Addr>`][38327]
* [`Ipv6Addr` implements `From<[u16; 8]>`][38131]
* [Ctrl-Z returns from `Stdin.read()` when reading from the console on
  Windows][38274]
* [std: Fix partial writes in `LineWriter`][38062]
* [std: Clamp max read/write sizes on Unix][38062]
* [Use more specific panic message for `&str` slicing errors][38066]
* [`TcpListener::set_only_v6` is deprecated][38304]. This
  functionality cannot be achieved in std currently.
* [`writeln!`, like `println!`, now accepts a form with no string
  or formatting arguments, to just print a newline][38469]
* [Implement `iter::Sum` and `iter::Product` for `Result`][38580]
* [Reduce the size of static data in `std_unicode::tables`][38781]
* [`char::EscapeDebug`, `EscapeDefault`, `EscapeUnicode`,
  `CaseMappingIter`, `ToLowercase`, `ToUppercase`, implement
  `Display`][38909]
* [`Duration` implements `Sum`][38712]
* [`String` implements `ToSocketAddrs`][39048]

Cargo
-----

* [The `cargo check` command does a type check of a project without
  building it][cargo/3296]
* [crates.io will display CI badges from Travis and AppVeyor, if
  specified in Cargo.toml][cargo/3546]
* [crates.io will display categories listed in Cargo.toml][cargo/3301]
* [Compilation profiles accept integer values for `debug`, in addition
  to `true` and `false`. These are passed to `rustc` as the value to
  `-C debuginfo`][cargo/3534]
* [Implement `cargo --version --verbose`][cargo/3604]
* [All builds now output 'dep-info' build dependencies compatible with
  make and ninja][cargo/3557]
* [Build all workspace members with `build --all`][cargo/3511]
* [Document all workspace members with `doc --all`][cargo/3515]
* [Path deps outside workspace are not members][cargo/3443]

Misc
----

* [`rustdoc` has a `--sysroot` argument that, like `rustc`, specifies
  the path to the Rust implementation][38589]
* [The `armv7-linux-androideabi` target no longer enables NEON
  extensions, per Google's ABI guide][38413]
* [The stock standard library can be compiled for Redox OS][38401]
* [Rust has initial SPARC support][38726]. Tier 3. No builds
  available.
* [Rust has experimental support for Nvidia PTX][38559]. Tier 3. No
  builds available.
* [Fix backtraces on i686-pc-windows-gnu by disabling FPO][39379]

Compatibility Notes
-------------------

* [Uninhabitable enums (those without any variants) no longer permit wildcard
  match patterns][38069]
* In this release, references to uninhabited types can not be
  pattern-matched. This was accidentally allowed in 1.15.
* [The compiler's `dead_code` lint now accounts for type aliases][38051].
* [Ctrl-Z returns from `Stdin.read()` when reading from the console on
  Windows][38274]
* [Clean up semantics of `self` in an import list][38313]

[37057]: rust-lang/rust#37057
[37761]: rust-lang/rust#37761
[38006]: rust-lang/rust#38006
[38051]: rust-lang/rust#38051
[38062]: rust-lang/rust#38062
[38062]: rust-lang/rust#38622
[38066]: rust-lang/rust#38066
[38069]: rust-lang/rust#38069
[38131]: rust-lang/rust#38131
[38154]: rust-lang/rust#38154
[38274]: rust-lang/rust#38274
[38304]: rust-lang/rust#38304
[38313]: rust-lang/rust#38313
[38314]: rust-lang/rust#38314
[38327]: rust-lang/rust#38327
[38401]: rust-lang/rust#38401
[38413]: rust-lang/rust#38413
[38469]: rust-lang/rust#38469
[38559]: rust-lang/rust#38559
[38571]: rust-lang/rust#38571
[38580]: rust-lang/rust#38580
[38589]: rust-lang/rust#38589
[38670]: rust-lang/rust#38670
[38712]: rust-lang/rust#38712
[38726]: rust-lang/rust#38726
[38781]: rust-lang/rust#38781
[38798]: rust-lang/rust#38798
[38909]: rust-lang/rust#38909
[38920]: rust-lang/rust#38920
[38927]: rust-lang/rust#38927
[39048]: rust-lang/rust#39048
[39282]: rust-lang/rust#39282
[39379]: rust-lang/rust#39379
[`<*const T>::wrapping_offset`]: https://doc.rust-lang.org/std/primitive.pointer.html#method.wrapping_offset
[`<*mut T>::wrapping_offset`]: https://doc.rust-lang.org/std/primitive.pointer.html#method.wrapping_offset
[`Duration::checked_add`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.checked_add
[`Duration::checked_div`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.checked_div
[`Duration::checked_mul`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.checked_mul
[`Duration::checked_sub`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.checked_sub
[`File::set_permissions`]: https://doc.rust-lang.org/std/fs/struct.File.html#method.set_permissions
[`IpAddr::is_ipv4`]: https://doc.rust-lang.org/std/net/enum.IpAddr.html#method.is_ipv4
[`IpAddr::is_ipv6`]: https://doc.rust-lang.org/std/net/enum.IpAddr.html#method.is_ipv6
[`Result::unwrap_or_default`]: https://doc.rust-lang.org/std/result/enum.Result.html#method.unwrap_or_default
[`SocketAddr::is_ipv4`]: https://doc.rust-lang.org/std/net/enum.SocketAddr.html#method.is_ipv4
[`SocketAddr::is_ipv6`]: https://doc.rust-lang.org/std/net/enum.SocketAddr.html#method.is_ipv6
[`String::insert_str`]: https://doc.rust-lang.org/std/string/struct.String.html#method.insert_str
[`String::split_off`]: https://doc.rust-lang.org/std/string/struct.String.html#method.split_off
[`Vec::dedup_by_key`]: https://doc.rust-lang.org/std/vec/struct.Vec.html#method.dedup_by_key
[`Vec::dedup_by`]: https://doc.rust-lang.org/std/vec/struct.Vec.html#method.dedup_by
[`VecDeque::resize`]:  https://doc.rust-lang.org/std/collections/vec_deque/struct.VecDeque.html#method.resize
[`VecDeque::truncate`]: https://doc.rust-lang.org/std/collections/vec_deque/struct.VecDeque.html#method.truncate
[`str::repeat`]: https://doc.rust-lang.org/std/primitive.str.html#method.repeat
[`str::replacen`]: https://doc.rust-lang.org/std/primitive.str.html#method.replacen
[cargo/3296]: rust-lang/cargo#3296
[cargo/3301]: rust-lang/cargo#3301
[cargo/3443]: rust-lang/cargo#3443
[cargo/3511]: rust-lang/cargo#3511
[cargo/3515]: rust-lang/cargo#3515
[cargo/3534]: rust-lang/cargo#3534
[cargo/3546]: rust-lang/cargo#3546
[cargo/3557]: rust-lang/cargo#3557
[cargo/3604]: rust-lang/cargo#3604
[RFC 1623]: https://github.com/rust-lang/rfcs/blob/master/text/1623-static.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants