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

Suggest dereference operations when a deref coercion won't work #39029

Closed
Manishearth opened this issue Jan 13, 2017 · 6 comments · Fixed by #72456
Closed

Suggest dereference operations when a deref coercion won't work #39029

Manishearth opened this issue Jan 13, 2017 · 6 comments · Fixed by #72456
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Manishearth
Copy link
Member

Deref coercions are tricky. &x will coerce to &***x if used in a place that expects a value of the type of &***x, however, this does not work for trait-bound generics.

The example that came up recently was:

use std::net::TcpListener;
let owned = "foobar".to_string(); // owned string came from somewhere
let works = TcpListener::bind("some string");
let doesn't = TcpListener::bind(&owned);

This errors with

error[E0277]: the trait bound `std::string::String: std::net::ToSocketAddrs` is not satisfied
 --> <anon>:5:18
  |
5 |     let listen = TcpListener::bind(&owned);
  |                  ^^^^^^^^^^^^^^^^^ the trait `std::net::ToSocketAddrs` is not implemented for `std::string::String`
  |
  = note: required because of the requirements on the impl of `std::net::ToSocketAddrs` for `&std::string::String`
  = note: required by `std::net::TcpListener::bind`

because while &owned would usually coerce to an &str, in this case it doesn't because even though TcpListener::bind would accept a &str, it doesn't accept only that type and the deref coercion won't work.

Programmers should not have to know these details to get their code to work. It would be nice if deref coercions worked in places where a generic bound type is expected, but there are other issues there (what happens if the trait gets implemented on String?) so it's not a clear win and would need an rfc anyway.

At the very least, we should suggest the usage of a dereference operator here.

The heuristics are:

  • you have a trait bound error
  • The error is on a reference expression &x
  • The expression is used in one of the allowed places for deref coercions (may not be necessary, really)
  • The trait isn't implemented by &x
  • The trait is implemented by &***x for some amount of * operations

then, suggest the correct thing with the dereference operators.

cc @jonathandturner @GuillaumeGomez

@Manishearth Manishearth added the A-diagnostics Area: Messages for errors, warnings, and lints label Jan 13, 2017
@durka
Copy link
Contributor

durka commented Jan 13, 2017

If you can find the deref well enough to put it in the error message, might as well fix the actual bug and apply the coercion. Maybe that needs an RFC.

@Manishearth
Copy link
Member Author

If you can find the deref well enough to put it in the error message, might as well fix the actual bug and apply the coercion.

No, that's a language change, and leads to hidden changes in semantics as traits get implemented. It's not a clear win.

The rfc explicitly calls this out (https://github.com/rust-lang/rfcs/blob/master/text/0401-coercions.md, see "Note that we do not perform coercions when matching traits". This RFC doesn't actually talk of deref coercions which were introduced earlier, but the semantics are the same and the reasoning probably applies too)

If we want this, we need to rfc it.

@GuillaumeGomez
Copy link
Member

For now, I added (still waiting for help from @nikomatsakis) ref suggestions, so I guess this is the next step.

lambda added a commit to lambda/rust that referenced this issue Jan 14, 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 rust-lang#39029 suggests better error messages to make it
more discoverable).

Rather than making people stumble over this, just implement
`ToSocketAddrs` for `String`.
bors added a commit that referenced this issue 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`.
@steveklabnik steveklabnik added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 9, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 26, 2017
@estebank
Copy link
Contributor

This code now builds.

@estebank
Copy link
Contributor

estebank commented Sep 2, 2019

@Manishearth could you come up with a different repro case for this? I believe this was fixed in the meantime by making deref coercions work, but I'm not sure. If it has been fixed, then we can close this ticket.

@Manishearth
Copy link
Member Author

I don't think this has been fixed, but i don't have the bandwidth to figure this out

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 C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants