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

[regression] type inference on parse() #73242

Closed
Mark-Simulacrum opened this issue Jun 11, 2020 · 5 comments · Fixed by #73304
Closed

[regression] type inference on parse() #73242

Mark-Simulacrum opened this issue Jun 11, 2020 · 5 comments · Fixed by #73304
Labels
regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Mark-Simulacrum
Copy link
Member

https://play.rust-lang.org/?version=beta&mode=debug&edition=2018&gist=cbe4ae71cf9d88e5c0fdf6d06af3502e as a minimal example.

Unclear whether this is a new trait impl or some regression elsewhere.

@Mark-Simulacrum Mark-Simulacrum added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. A-inference Area: Type inference labels Jun 11, 2020
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jun 11, 2020
@lcnr
Copy link
Contributor

lcnr commented Jun 11, 2020

PartialEq<SocketAddr> has been added in 1.45 which would explain this: https://doc.rust-lang.org/nightly/std/net/struct.SocketAddrV4.html#impl-PartialEq%3CSocketAddr%3E

@Mark-Simulacrum
Copy link
Member Author

There were roughly ~15-20 crates that failed in the crater run due to this, mostly in tests. I'm not sure whether we think that's sufficient reasoning to drop the impl, I get that it can be useful...

cc @rust-lang/libs (and relabeling as this is not an inference regression)

@Mark-Simulacrum Mark-Simulacrum added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed A-inference Area: Type inference I-prioritize Issue: Indicates that prioritization has been requested for this issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 11, 2020
@withoutboats
Copy link
Contributor

withoutboats commented Jun 11, 2020

Can we add PartialEq<str> to the SocketAddr types? That would provide a path to patch the regression.

Being able to easily compare a socketaddr to a string seems pretty valuable for testing, I'd rather see we make it easier than require the turbofish.

#72239 for the PR that added these impls

@SimonSapin
Copy link
Contributor

Can we add PartialEq<str> to the SocketAddr types?

I’m not in favor since this would blur the difference between "this string does not parse as a valid socket address" v.s. "it does, but one not equal to that one". Code that doesn’t care about that difference can already use SocketAddr::to_string and compare strings.

@dtolnay
Copy link
Member

dtolnay commented Jun 13, 2020

#72239 mostly cared about PartialOrd and Ord impls (as you can tell from the PR title). Unlike PartialEq, none existed before on SocketAddr so they won't be problematic. I think the PR included new PartialEq impls only in support of heterogeneous PartialOrd (see #72239 (comment)) which was ultimately removed from the PR anyway.

I opened #73304 to revert the new PartialEq impls, leaving the PartialOrd and Ord impls from #72239.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jun 13, 2020
Revert heterogeneous SocketAddr PartialEq impls

Originally added in rust-lang#72239.

These lead to inference regressions (mostly in tests) in code that looks like:

```rust
let socket = SocketAddrV4::new(Ipv4Addr::new(127, 0, 0, 1), 8080);
assert_eq!(socket, "127.0.0.1:8080".parse().unwrap());
```

That compiles as of stable 1.44.0 but fails in beta with:

```console
error[E0284]: type annotations needed
 --> src/main.rs:3:41
  |
3 |     assert_eq!(socket, "127.0.0.1:8080".parse().unwrap());
  |                                         ^^^^^ cannot infer type for type parameter `F` declared on the associated function `parse`
  |
  = note: cannot satisfy `<_ as std::str::FromStr>::Err == _`
help: consider specifying the type argument in the method call
  |
3 |     assert_eq!(socket, "127.0.0.1:8080".parse::<F>().unwrap());
  |
```

Closes rust-lang#73242.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jun 14, 2020
Revert heterogeneous SocketAddr PartialEq impls

Originally added in rust-lang#72239.

These lead to inference regressions (mostly in tests) in code that looks like:

```rust
let socket = SocketAddrV4::new(Ipv4Addr::new(127, 0, 0, 1), 8080);
assert_eq!(socket, "127.0.0.1:8080".parse().unwrap());
```

That compiles as of stable 1.44.0 but fails in beta with:

```console
error[E0284]: type annotations needed
 --> src/main.rs:3:41
  |
3 |     assert_eq!(socket, "127.0.0.1:8080".parse().unwrap());
  |                                         ^^^^^ cannot infer type for type parameter `F` declared on the associated function `parse`
  |
  = note: cannot satisfy `<_ as std::str::FromStr>::Err == _`
help: consider specifying the type argument in the method call
  |
3 |     assert_eq!(socket, "127.0.0.1:8080".parse::<F>().unwrap());
  |
```

Closes rust-lang#73242.
RalfJung added a commit to RalfJung/rust that referenced this issue Jun 15, 2020
Revert heterogeneous SocketAddr PartialEq impls

Originally added in rust-lang#72239.

These lead to inference regressions (mostly in tests) in code that looks like:

```rust
let socket = SocketAddrV4::new(Ipv4Addr::new(127, 0, 0, 1), 8080);
assert_eq!(socket, "127.0.0.1:8080".parse().unwrap());
```

That compiles as of stable 1.44.0 but fails in beta with:

```console
error[E0284]: type annotations needed
 --> src/main.rs:3:41
  |
3 |     assert_eq!(socket, "127.0.0.1:8080".parse().unwrap());
  |                                         ^^^^^ cannot infer type for type parameter `F` declared on the associated function `parse`
  |
  = note: cannot satisfy `<_ as std::str::FromStr>::Err == _`
help: consider specifying the type argument in the method call
  |
3 |     assert_eq!(socket, "127.0.0.1:8080".parse::<F>().unwrap());
  |
```

Closes rust-lang#73242.
@bors bors closed this as completed in 202499f Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-libs-api Relevant to the library API 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