I think it should go further than that and have better suggestions about the specific implementations found. When annotations needed, look at impls for more accurate suggestions by estebank · Pull Request #128653 · rust-lang/rust · GitHub does that, but it hasn't landed yet. We should also special handle when we detect the blanket impls, and provide additional help when encountering them.
3 Likes
Edit: Eh I'm just repeating things @dlight already suggested.
I have no idea how feasible it is to make use of, but trait implementations in std are tagged with the version they became stable in. Perhaps cargo fix at least could make use of that.[1] A more extreme approach to rustc itself would be the ability to configure the single-impl inference based on stabilization version.
cargo fix --from-version=1.79↩︎
2 Likes
Are there some flagship examples of inference breakage across versions that didn't stem from the "only one possible implementation" rule? I'm not sure if we need an elaborate mechanism that attempts to take the version an impl was introduced in into account; ideally we could solve this in a version-independent manner.
3 Likes
I think we need to uplift clippy::useless_conversions and make it more robust so that we can detect when we're going through any blanket impl unnecessarily. per-platform cfgs are an issue for accurate detection (because of the implementation decision not to track type aliases, my understanding is that that might be changing thanks to generalized TAIT). I even found a bunch of cases in rustc itself where we were calling .into() unnecessarily. This is not a complete solution, it is only something that slightly reduces the likelihood of the case at hand from happening again, but all blanket impls, including those from other crates and not just std can continue to cause issues. Maybe we should add a diagnostic annotation that we can put on the blanket impl for us to give a hint about recommending using that one over any other, so that we have something useful to say when a blanket impl stops working, and with the lint gently push people away from useless calls to the blanket impl.
7 Likes
This is probably the simplest solution that can/should be done in the immediate future. Making it more robust is definitely necessary, as without checking I would imagine that I have that lint enabled for time (I tend to prefer quite strict linting).
5 Likes
Unfortunately, labeling a PR with relnotes does not automatically communicate, amongst the hundred other PRs labeled relnotes, what should actually go in the relnote. There needs to be a more active communication to T-release of what we want instead of assuming they can just invest the labour-hours to deal with a vague request, here, now that rust-lang/rust is where it's at.
2 Likes
Context was provided in the comment that added the label.
If T-release can only look at the labels and can't check the context due to the amount then perhaps the label itself is a too-low-bandwidth channel. But then what are our alternatives?
2 Likes
I tried to explain to T-compiler, some time ago, that opaque number sequences ("issue numbers") are not very informative about what is behind that pointer, and pretending they are is harmful. People could provide, instead, actual reasons for things, which do not require dereferencing random pointers, which thrashes cache.
I feel that has been demonstrated here.
But yes, I think probably we need to rethink the label-assigning approach. Teams could take it on themselves to submit an actual report of what they'd like to see in the release notes, for instance.
9 Likes
I encourage everyone that is about to argue about what breaking changes are technically considered permissible (and perhaps it would be best to have that argument elsewhere?) to first read RFC1105: API evolution and RFC1122: language semver.
2 Likes
One must also be aware that RFCs are not kept up to date, so there are also breaking changes not mentioned in those RFCs, be it due to new language features, or clarification from other RFCs.
2 Likes
There's more shades of gray there than that: we can agree that a breaking change is allowed and even desirable, while still wanting the rollout to be done with more grace. We even affected other ecosystems (that could arguably also have done a better job with their own rollout). As I said earlier, there were multiple things we could have done (delay landing for the ecosystem to organically move to the new time version, release dot-releases of time for prior versions, add a branch to the error telling people to update their time, add a lint to detect problematic <T as Into<T>::into(_) cases, teaching cargo to have a MOTD mechanism for specific crate-versions/cargo linting of deps a-la RustSec, abusing RustSec for this notice), each individually helping a bit, and on aggregate making the transition much smoother than it was.
I hope this doesn't come across as too argumentative, but why is it a bad idea to discuss a category of problems in a thread about one specific example of the problem? (Particularly when there might be disagreement that it is a problem in the first place.)
23 Likes
I really want this, and for other libraries too. (Though #[stable] attribute is, ironically, not stable.)
I've already suggested it elsewhere. Basically if you have something like max_tested_rust_version in Cargo.toml all items that are in later version and accessed from the crate get warning and trait impl resolution is affected to pick old ones (but still warn, so that crate authors know how to fix them).
I'm imagining this could have other uses too.
2 Likes
Isn't this an Edition?
3 Likes
IIRC there was some discussion about potentially generalizing the array IntoIterator resolution hack in the 2015 edition to apply to all trait implementations, effectively running name resolution twice (once with all impls and once without any impls added after the edition baseline) and treating a change in resolution similarly to how a name conflict with unstable items are treated today (i.e. if at the same precedence, pick the one selected without the "unstable" name present and emit a future compatibility warning). There was some significant pushback at the time to anything that looks like config dependent name resolution, since it both pushes protects to upgrade when they shouldn't need to (all features should be available to all editions; editions aren't dialects frozen in time) and slows down adoption (can't have a feature to use new functionality if it's hidden; dilutes the release train by putting more things on the edition gate, even if only partially).
Personally, I think it'd be a good idea to make name resolution for stable std act to as-if std had exactly just the stable API provided with the configured package.rust-version, but I understand that's much stronger of a position than most project contributors', let alone other developers', who are less cognizant of the level of stability concerns std is saddled with (i.e. as here, and that since std is tightly coupled to rustc, you can't lockfile/downgrade std without using an unsupported and out of date compiler toolchain).
7 Likes
Hm, slowing down the adoption of new features would be a real result of this. I have worked professionally with C++ and only in the last year or two has C++17 started becoming widely adopted. C++20 and 23 is still exotic. And a lot of code is still on 14 or 11.
I would not want to see that slowness in Rust, so it would be better if another option can be found.
1 Like
Here's a way to remedy this:
More discussion:
6 Likes
Unsure where to record this (so as to not pester teams who can't act on it and make them feel ganged up on) but one of the problems with this change that I've not seen discussed is the ability to git bisect a project. Of course, if you don't have a Cargo.lock committed then thats not a problem in this case but one of the reasons we encouraged it was to help in bisecting a lot of other cases.
7 Likes
I hope this doesn't come across as too argumentative, but why is it a bad idea to discuss a category of problems in a thread about one specific example of the problem? (Particularly when there might be disagreement that it is a problem in the first place.)
I only mentioned it because I felt a slight concern that the conversation could take a hard turn towards the "meta-level" argument about whether it is "permissible" and remain on that course somewhat pointlessly, precisely because there has been an flagrant violation of every other tenet of making the transition relatively smooth if possible and in general just good engineering practices.
8 Likes
I feel a bit thick. Can you elaborate slightly about what bisection you're thinking of? Are you referring to cargo bisect-rustc specifically?
It affects bisection of users' own projects that were using the time crate. Old lockfiles in vcs history are going to be locked to previous versions of time that don't build any more. This complicates building and testing previous versions of software.
11 Likes