-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Remove the trait selection impl in method::probe #43880
Conversation
LGTM (modulo how it fares in the wild, which is yet to be seen) r? @nikomatsakis |
8a32f1b
to
7aecbc3
Compare
@bors try |
⌛ Trying commit 7aecbc38c5c70f730eccb571290d02094cb9b6af with merge b96d5eea76e758bdd82fd4169eb4d5f7e3ce4fe2... |
☀️ Test successful - status-travis |
cc @rust-lang/infra Cargobomb requested. |
☔ The latest upstream changes (presumably #43710) made this pull request unmergeable. Please resolve the merge conflicts. |
4c09b45
to
9b54410
Compare
I think I'm comfortable with landing this refactor the way it is, provided crater agrees. |
Cargobomb run has started. |
I read over the changes and r=me once we see the cargobomb results. |
Cargobomb results - http://cargobomb-reports.s3.amazonaws.com/pr-43880/index.html |
I think this has to depend on #43690 because that should fix the |
LDAP report:
|
I think I fixed the LDAP problem with the new commit. |
f378da6
to
e77247c
Compare
☔ The latest upstream changes (presumably #43690) made this pull request unmergeable. Please resolve the merge conflicts. |
e77247c
to
e37c087
Compare
Now that the |
☔ The latest upstream changes (presumably #43076) made this pull request unmergeable. Please resolve the merge conflicts. |
the data serves no purpose - it can be recovered from the obligation - and I think may leak stale inference variables into global caches.
This fixes a few cases of inference misses, some of them regressions caused by the impl selected for a method not being immediately evaluated.
e37c087
to
15f6540
Compare
Ready for review, assuming higher-ranked function pointers will still impl |
@bors r+
That part in particular doesn't seem to be controversial, I think...? |
📌 Commit 15f6540 has been approved by |
It can't be, given that they are already |
Remove the trait selection impl in method::probe This removes the hacky trait selection reimplementation in `method::probe`, which occasionally comes and causes problems. There are 2 issues I've found with this approach: 1. The older implementation sometimes had a "guess" type from an impl, which allowed subtyping to work. This is why I needed to make a change in `libtest`: there's an `impl<A> Clone for fn(A)` and we're calling `<for<'a> fn(&'a T) as Clone>::clone`. The older implementation would do a subtyping between the impl type and the trait type, so it would do the check for `<fn(A) as Clone>::clone`, and confirmation would continue with the subtyping. The newer implementation directly passes `<for<'a> fn(&'a T) as Clone>::clone` to selection, which fails. I'm not sure how big of a problem that would be in reality, especially after #43690 would remove the `Clone` problem, but I still want a crater run to avoid breaking the world. 2. The older implementation "looked into" impls to display error messages. I'm not sure that's an advantage - it looked exactly 1 level deep. r? @eddyb
☀️ Test successful - status-appveyor, status-travis |
This broke Servo. With have a fix (servo/servo#18327), but this does reject real code that was previously accepted. This should at least be noted in release notes. |
@SimonSapin Indeed -- I'm trying to figure out what is going on in that PR. Can you dump the struct definition on which the traceable derive was failing? |
@SimonSapin I opened #44224 to discuss and decide on a resolution. Thanks for the regression report! |
Changelog: Version 1.21.0 (2017-10-12) ========================== Language -------- - [You can now use static references for literals.][43838] Example: ```rust fn main() { let x: &'static u32 = &0; } ``` - [Relaxed path syntax. Optional `::` before `<` is now allowed in all contexts.][43540] Example: ```rust my_macro!(Vec<i32>::new); // Always worked my_macro!(Vec::<i32>::new); // Now works ``` Compiler -------- - [Upgraded jemalloc to 4.5.0][43911] - [Enabled unwinding panics on Redox][43917] - [Now runs LLVM in parallel during translation phase.][43506] This should reduce peak memory usage. Libraries --------- - [Generate builtin impls for `Clone` for all arrays and tuples that are `T: Clone`][43690] - [`Stdin`, `Stdout`, and `Stderr` now implement `AsRawFd`.][43459] - [`Rc` and `Arc` now implement `From<&[T]> where T: Clone`, `From<str>`, `From<String>`, `From<Box<T>> where T: ?Sized`, and `From<Vec<T>>`.][42565] Stabilized APIs --------------- [`std::mem::discriminant`] Cargo ----- - [You can now call `cargo install` with multiple package names][cargo/4216] - [Cargo commands inside a virtual workspace will now implicitly pass `--all`][cargo/4335] - [Added a `[patch]` section to `Cargo.toml` to handle prepublication dependencies][cargo/4123] [RFC 1969] - [`include` & `exclude` fields in `Cargo.toml` now accept gitignore like patterns][cargo/4270] - [Added the `--all-targets` option][cargo/4400] - [Using required dependencies as a feature is now deprecated and emits a warning][cargo/4364] Misc ---- - [Cargo docs are moving][43916] to [doc.rust-lang.org/cargo](https://doc.rust-lang.org/cargo) - [The rustdoc book is now available][43863] at [doc.rust-lang.org/rustdoc](https://doc.rust-lang.org/rustdoc) - [Added a preview of RLS has been made available through rustup][44204] Install with `rustup component add rls-preview` - [`std::os` documentation for Unix, Linux, and Windows now appears on doc.rust-lang.org][43348] Previously only showed `std::os::unix`. Compatibility Notes ------------------- - [Changes in method matching against higher-ranked types][43880] This may cause breakage in subtyping corner cases. [A more in-depth explanation is available.][info/43880] - [rustc's JSON error output's byte position start at top of file.][42973] Was previously relative to the rustc's internal `CodeMap` struct which required the unstable library `libsyntax` to correctly use. - [`unused_results` lint no longer ignores booleans][43728] [42565]: rust-lang/rust#42565 [42973]: rust-lang/rust#42973 [43348]: rust-lang/rust#43348 [43459]: rust-lang/rust#43459 [43506]: rust-lang/rust#43506 [43540]: rust-lang/rust#43540 [43690]: rust-lang/rust#43690 [43728]: rust-lang/rust#43728 [43838]: rust-lang/rust#43838 [43863]: rust-lang/rust#43863 [43880]: rust-lang/rust#43880 [43911]: rust-lang/rust#43911 [43916]: rust-lang/rust#43916 [43917]: rust-lang/rust#43917 [44204]: rust-lang/rust#44204 [cargo/4123]: rust-lang/cargo#4123 [cargo/4216]: rust-lang/cargo#4216 [cargo/4270]: rust-lang/cargo#4270 [cargo/4335]: rust-lang/cargo#4335 [cargo/4364]: rust-lang/cargo#4364 [cargo/4400]: rust-lang/cargo#4400 [RFC 1969]: rust-lang/rfcs#1969 [info/43880]: rust-lang/rust#44224 (comment) [`std::mem::discriminant`]: https://doc.rust-lang.org/std/mem/fn.discriminant.html
check::method - unify receivers before normalizing method signatures Normalizing method signatures can unify inference variables, which can cause receiver unification to fail. Unify the receivers first to avoid that. Fixes #36701. Fixes #45801. Fixes #45855. r? @eddyb beta-nominating because #43880 made this ICE happen in more cases (the code in that issue ICEs post-#43880 only, but the unit test here ICEs on all versions).
check::method - unify receivers before normalizing method signatures Normalizing method signatures can unify inference variables, which can cause receiver unification to fail. Unify the receivers first to avoid that. Fixes rust-lang#36701. Fixes rust-lang#45801. Fixes rust-lang#45855. r? @eddyb beta-nominating because rust-lang#43880 made this ICE happen in more cases (the code in that issue ICEs post-rust-lang#43880 only, but the unit test here ICEs on all versions).
This removes the hacky trait selection reimplementation in
method::probe
, which occasionally comes and causes problems.There are 2 issues I've found with this approach:
libtest
: there's animpl<A> Clone for fn(A)
and we're calling<for<'a> fn(&'a T) as Clone>::clone
. The older implementation would do a subtyping between the impl type and the trait type, so it would do the check for<fn(A) as Clone>::clone
, and confirmation would continue with the subtyping. The newer implementation directly passes<for<'a> fn(&'a T) as Clone>::clone
to selection, which fails. I'm not sure how big of a problem that would be in reality, especially after Generate builtin impls forClone
#43690 would remove theClone
problem, but I still want a crater run to avoid breaking the world.r? @eddyb