Try all stable method candidates first before trying unstable ones by nbdd0121 · Pull Request #90329 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation21 Commits3 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Currently we try methods in this order in each step:

Stable by value
Unstable by value
Stable autoref
Unstable autoref
...

This PR changes it to first try pick methods without any unstable candidates, and if none is found, try again to pick unstable ones.

Fix #90320
CC #88971, hopefully would allow us to rename the "unstable_*" methods for integer impls back.

@rustbot label T-compiler T-libs-api

r? @nagisa

(rust-highfive has picked a reviewer for you, use r? to override)

rustbot added T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

T-libs-api

Relevant to the library API team, which will review and decide on the PR/issue.

labels

Oct 26, 2021

This comment has been minimized.

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

⌛ Trying commit dc459da71aef6031e787b78b7f240f5b1d0618f0 with merge bb00c9b6276be74811d40eea0b7b6da7a6780c42...

☀️ Try build successful - checks-actions
Build commit: bb00c9b6276be74811d40eea0b7b6da7a6780c42 (bb00c9b6276be74811d40eea0b7b6da7a6780c42)

Finished benchmarking commit (bb00c9b6276be74811d40eea0b7b6da7a6780c42): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

jyn514 changed the title~~Try all stable candidates first before trying unstable ones~~ Try all stable candidates for autoref first before trying unstable ones

Oct 27, 2021

Seems reasonable to me! I look forward to a PR un-renaming the integer rounding methods.

camelid changed the title~~Try all stable candidates for autoref first before trying unstable ones~~ Try all stable method candidates first before trying unstable ones

Oct 30, 2021

Choose a reason for hiding this comment

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

I don't see an inherent issue with the idea – the unstable methods are… unstable, so I don't see a huge problem in de-emphasizing them, even if it could change the behaviour of the code that targets nightly rustc.

With that in mind, though, I would like to see a couple other changes to the PR:

First is a -Z debug flag to revert to the previous order of pick priority. Such a flag would help significantly with a potential situation where we receive a bug about a behaviour change to somebody's nightly code.

And then I would love to see a test that verifies this new pick order between stable and unstable items expanded significantly. The existing test seems pretty anemic to me in that it only verifies a stable autorefd method is preferred to an unstable by-value one. But then there are cases like mutable vs immutable autoref (e.g. do we still pick an unstable method autoref if a mutable autoref necessary for a stable method is not valid?), const ptr methods, etc.

I don't see an inherent issue with the idea – the unstable methods are… unstable, so I don't see a huge problem in de-emphasizing them, even if it could change the behaviour of the code that targets nightly rustc.

No; this wouldn't change the behaviour. A unstable method with corresponding feature enabled is not considered an "unstable_candidate"; only unstable methods with feature gates not enabled are considered unstable candidates. So code that already compiles shouldn't be affected.

With that in mind, though, I would like to see a couple other changes to the PR:

First is a -Z debug flag to revert to the previous order of pick priority. Such a flag would help significantly with a potential situation where we receive a bug about a behaviour change to somebody's nightly code.

Added.

And then I would love to see a test that verifies this new pick order between stable and unstable items expanded significantly. The existing test seems pretty anemic to me in that it only verifies a stable autorefd method is preferred to an unstable by-value one. But then there are cases like mutable vs immutable autoref (e.g. do we still pick an unstable method autoref if a mutable autoref necessary for a stable method is not valid?), const ptr methods, etc.

I expanded the test a bit. For unstable autoref vs a stable mutable autoref on a non-mutable variable scenario, the code will now pick the stable mutable autoref instead of the unstable autoref (method picking doesn't have this information; actually whether Deref or DerefMut is used depends on which method gets picked). So the error will change in this scenario. But as explained above, this wouldn't affect code that currently compiles.

Nice, thanks!

@bors r+ rollup=never

📌 Commit 6ad626f has been approved by nagisa

bors added the S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

label

Nov 19, 2021

Is my understanding correct that we may want to consider this for a backport to beta?

Is my understanding correct that we may want to consider this for a backport to beta?

We have #90320 which is deemed acceptable breakage by libs team, and #88971 which is reverted in #89184, so I don't think this needs backport.

Finished benchmarking commit (ce3f3a5): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Dec 19, 2021

Revert "Temporarily rename int_roundings functions to avoid conflicts"

This reverts commit 3ece63b.

This should be okay because rust-lang#90329 has been merged.

r? @joshtriplett

tmandry added a commit to tmandry/rust that referenced this pull request

Jan 5, 2022

wip-sync pushed a commit to NetBSD/pkgsrc-wip that referenced this pull request

Jan 22, 2022

Pkgsrc changes:

Bump available bootstraps to 1.57.0.
For some reason, the vendor/libc checksums don't need fixing.
Bump required external LLVM to 12.0, according to upstream change log.
Adapt the Darwin linker patch.

(For some reason I've not figured out yet, cargo is a lot more verbose while building, echoes the rustc invocation.)

Upstream changes:

Version 1.58.1 (2022-01-19)

Fix race condition in std::fs::remove_dir_all (CVE-2022-21658)
Handle captured arguments in the useless_format Clippy lint
Move non_send_fields_in_send_ty Clippy lint to nursery
Fix wrong error message displayed when some imports are missing
Fix rustfmt not formatting generated files from stdin

Version 1.58.0 (2022-01-13)

Language

Format strings can now capture arguments simply by writing {ident} in the string. This works in all macros accepting format strings. Support for this in panic! (panic!("{ident}")) requires the 2021 edition; panic invocations in previous editions that appear to be trying to use this will result in a warning lint about not having the intended effect.
*const T pointers can now be dereferenced in const contexts.
The rules for when a generic struct implements Unsize have been relaxed.

Compiler

Add LLVM CFI support to the Rust compiler
Stabilize -Z strip as -C strip. Note that while release builds already don't add debug symbols for the code you compile, the compiled standard library that ships with Rust includes debug symbols, so you may want to use the strip option to remove these symbols to produce smaller release binaries. Note that this release only includes support in rustc, not directly in cargo.
Add support for LLVM coverage mapping format versions 5 and 6
Emit LLVM optimization remarks when enabled with -Cremark
Update the minimum external LLVM to 12
Add x86_64-unknown-none at Tier 3*
Build musl dist artifacts with debuginfo enabled. When building release binaries using musl, you may want to use the newly stabilized strip option to remove these debug symbols, reducing the size of your binaries.
Don't abort compilation after giving a lint error
Error messages point at the source of trait bound obligations in more places

* Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support.

Libraries

[All remaining functions in the standard library have #[must_use] annotations where appropriate][89692], producing a warning when ignoring their return value. This helps catch mistakes such as expecting a function to mutate a value in place rather than return a new value.
Paths are automatically canonicalized on Windows for operations that support it
Re-enable debug checks for copy and copy_nonoverlapping
Implement RefUnwindSafe for Rc<T>
Make RSplit<T, P>: Clone not require T: Clone
Implement Termination for Result<Infallible, E>. This allows writing fn main() -> Result<Infallible, ErrorType>, for a program whose successful exits never involve returning from main (for instance, a program that calls exit, or that uses exec to run another program).

Stabilized APIs

These APIs are now usable in const contexts:

Cargo

Rustdoc

Compatibility Notes

Try all stable method candidates first before trying unstable ones. This change ensures that adding new nightly-only methods to the Rust standard library will not break code invoking methods of the same name from traits outside the standard library.
Windows: std::process::Command will no longer search the current directory for executables.
All proc-macro backward-compatibility lints are now deny-by-default.
proc_macro: Append .0 to unsuffixed float if it would otherwise become int token
Refactor weak symbols in std::sys::unix. This optimizes accesses to glibc functions, by avoiding the use of dlopen. This does not increase the minimum expected version of glibc. However, software distributions that use symbol versions to detect library dependencies, and which take weak symbols into account in that analysis, may detect rust binaries as requiring newer versions of glibc.
rustdoc now rejects some unexpected semicolons in doctests

Internal Changes

These changes provide no direct user facing benefits, but represent significant improvements to the internals and overall performance of rustc and related tools.

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request

Mar 1, 2022

Pkgsrc changes:

Bump bootstrap kit version to 1.57.0.
Bump require external LLVM to 12.0, according to upstream change log.
Adjust patches as needed, adjust line numbers.
Update checksum adjustments. For some reason the vendor/libc checksum doesn't need fixing, apparently, it remains as commented out.
Add makefile to do all the NetBSD boostrap/cross builds (do-cross.mk). Allow passing in additions to CONFIGURE_ARGS via ADD_CONFIGURE_ARGS.

Upstream changes:

Version 1.58.1 (2022-01-19)

Fix race condition in std::fs::remove_dir_all (CVE-2022-21658)
Handle captured arguments in the useless_format Clippy lint
Move non_send_fields_in_send_ty Clippy lint to nursery
Fix wrong error message displayed when some imports are missing
Fix rustfmt not formatting generated files from stdin

Version 1.58.0 (2022-01-13)

Language

Format strings can now capture arguments simply by writing {ident} in the string. This works in all macros accepting format strings. Support for this in panic! (panic!("{ident}")) requires the 2021 edition; panic invocations in previous editions that appear to be trying to use this will result in a warning lint about not having the intended effect.
*const T pointers can now be dereferenced in const contexts.
The rules for when a generic struct implements Unsize have been relaxed.

Compiler

Add LLVM CFI support to the Rust compiler
Stabilize -Z strip as -C strip. Note that while release builds already don't add debug symbols for the code you compile, the compiled standard library that ships with Rust includes debug symbols, so you may want to use the strip option to remove these symbols to produce smaller release binaries. Note that this release only includes support in rustc, not directly in cargo.
Add support for LLVM coverage mapping format versions 5 and 6
Emit LLVM optimization remarks when enabled with -Cremark
Update the minimum external LLVM to 12
Add x86_64-unknown-none at Tier 3*
Build musl dist artifacts with debuginfo enabled. When building release binaries using musl, you may want to use the newly stabilized strip option to remove these debug symbols, reducing the size of your binaries.
Don't abort compilation after giving a lint error
Error messages point at the source of trait bound obligations in more places

* Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support.

Libraries

[All remaining functions in the standard library have #[must_use] annotations where appropriate][89692], producing a warning when ignoring their return value. This helps catch mistakes such as expecting a function to mutate a value in place rather than return a new value.
Paths are automatically canonicalized on Windows for operations that support it
Re-enable debug checks for copy and copy_nonoverlapping
Implement RefUnwindSafe for Rc<T>
Make RSplit<T, P>: Clone not require T: Clone
Implement Termination for Result<Infallible, E>. This allows writing fn main() -> Result<Infallible, ErrorType>, for a program whose successful exits never involve returning from main (for instance, a program that calls exit, or that uses exec to run another program).

Stabilized APIs

These APIs are now usable in const contexts:

Cargo

Rustdoc

Compatibility Notes

Try all stable method candidates first before trying unstable ones. This change ensures that adding new nightly-only methods to the Rust standard library will not break code invoking methods of the same name from traits outside the standard library.
Windows: std::process::Command will no longer search the current directory for executables.
All proc-macro backward-compatibility lints are now deny-by-default.
proc_macro: Append .0 to unsuffixed float if it would otherwise become int token
Refactor weak symbols in std::sys::unix. This optimizes accesses to glibc functions, by avoiding the use of dlopen. This does not increase the minimum expected version of glibc. However, software distributions that use symbol versions to detect library dependencies, and which take weak symbols into account in that analysis, may detect rust binaries as requiring newer versions of glibc.
rustdoc now rejects some unexpected semicolons in doctests

Internal Changes

These changes provide no direct user facing benefits, but represent significant improvements to the internals and overall performance of rustc and related tools.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request

Feb 21, 2023

…unstable_flag, r=oli-obk

remove unstable pick_stable_methods_before_any_unstable flag

This flag was only added in rust-lang#90329 in case there was any issue with the impl so that it would be easy to tell nightly users to use the flag to disable the new logic to fix their code. It's now been enabled for two years and also I can't find any issues corresponding to this new functionality? This flag made it way harder to understand how this code works so it would be nice to remove it and simplify what's going on.

cc @nbdd0121

r? @oli-obk

Labels

merged-by-bors

This PR was explicitly merged by bors.

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

T-libs-api

Relevant to the library API team, which will review and decide on the PR/issue.