5 ms·

I’d love someone to comment on this who’s knowledgeable. I know we have a few rust experts in HN. I’m curious if this is a bug with the update. My novice brain

by tyleo 2y ago

I’d love someone to comment on this who’s knowledgeable. I know we have a few rust experts in HN. I’m curious if this is a bug with the update. My novice brain tells me this shouldn’t have happened.

steveklabnik 2y ago
So, one area in which Rust is okay with "breaking" source compatibility is if some inference gets confused, and you need to add a type annotation. This issue was discovered before the release, and a new version of time was released to fix it. cargo update -p time and you're done. This doesn't happen very often. It bit one of my projects too, and it took me longer to realize that it was a new version release that caused the problem than it did to fix the problem. It's been so long since something like this happened to me I cannot remember when it happened last. Here's the upstream issue, if you're curious https://github.com/time-rs/time/issues/693 https://github.com/time-rs/time/issues/693
- nequo 2y ago
  I was wondering why and it looks like a new trait implementation in std made a type placeholder ambiguous in older versions of time? time was caught in a crater run when a new trait implementation was added to the standard library. While it is a breaking change, it is considered acceptable breakage because the alternative is never adding any trait implementations or methods. https://github.com/time-rs/time/issues/681#issuecomment-2143130755 https://github.com/time-rs/time/issues/681#issuecomment-2143...
  - comex 2y ago
    I wish rustc had a way to 'pin' the output of type inference, in the same way that Cargo.lock pins the versions of dependencies. That way, old code would continue to compile based on what the types were originally inferred as, even if the code retroactively became ambiguous. This wouldn't be a perfect solution. Due to conditional compilation, any given variable in the source code might not exist on certain platforms, or it might even have a different type depending on the platform or configuration. So the question of "what type does this variable have" doesn't always have a single answer to record in a lockfile. And macros make things even harder. But even as an imperfect solution, I think it still has the potential to avert most real-world cases of breakage caused by Rust upgrades (and even dependency upgrades). As a bonus, the same mechanism could potentially also be used to detect breaking API changes.
    - estebank 2y ago
      I think the closest to what you're asking for is to build with a contemporary rustc version (have a rust-toolchain.toml in your repo). This isn't a satisfactory answer, but it is an available option nonetheless.
    - mtndew4brkfst 2y ago
      I wish rustc had a way to 'pin' the output of type inference Would that not be "take the outcome of said type inference, write it into the LHS as a type declaration or into the RHS as a generic parameter", aka make the inferred type permanently declared? We're not totally powerless for addressing the practical implication here, and I don't know how we could have stable yet still implicit type inference across compiler versions unless that type inference logic never changed ever again.
- shotnothing 2y ago
  sounds like they should have added a warning
- OutOfHere 2y ago
  Whenever I see this message in a repo: > An owner of this repository has limited the ability to comment to users that have contributed to this repository in the past. I immediately know it should never be used. time-rs is such a repo. They don't want you or need you there, and they don't care about your bug reports or even your questions.
  - mtndew4brkfst 2y ago
    This shows a stark lack of empathy for maintainers. Per a maintainer’s Mastodon profile, the comment lock is temporary and specifically in response to driveby commenters dogpiling a bug report where the issue in question has a working fix that was already released months ago. Said abusive comment traffic probably often originated from blog posts and comment threads similar to this one.
    - OutOfHere 2y ago
      That hardly justifies the block. Users deserve empathy too, considering they have to bear the consequences if things go wrong. One now doesn't even know and cannot even estimate the number of other issues that must have gone unreported. It's not safe or wise to use a package that is so shrouded in mystery. It is in fact foolhardy. If maintainers don't want to address issues, they only have to ignore them. At least other users can then still discuss the issues in-between themselves. A lack of empathy for maintainers does not come into the picture. No one should have to go to Mastodon or wherever to find out about the author's stance. The readme of the repo is where the stance belongs. The readme is currently grossly misleading, asking users to use Discussions, but this is not possible. Overall it seems like a hazardous project to use.
      - hluska 2y ago
        This is an open source tool meaning that maintainers can communicate however they like. If you don’t like it, you can fork it and do things your own way. But there is absolutely nothing wrong with temporarily disabling comments. If anything, that’s a sign of self care and this wisdom. A little more empathy wouldn’t hurt you.
- cropcirclbureau 2y ago
  I work in a large code base with 1800+ crate dependencies (don't ask) with half of them being unstable crates not really published to crates.io. And yeah, we run into this issue a lot. The recent one was caused[0] by human error. It's usually human error but there's also a lot of cases due to Cargo's choice to go to pick the highest version that satisfies the constraints. It's a reasonable choice on Cargo's part but yes, our team has learned to be very conservative to diffs on `Cargo.lock` and most of our dependencies are pinned. [0]: https://github.com/denoland/deno_core/issues/797 https://github.com/denoland/deno_core/issues/797
- xyzzy_plugh 2y ago
  This is still a pretty astounding breakage possibility let alone that it actually broke. Such inference seems inherently brittle and inevitably will break. This sort of thing certainly does Rust no favours with regards to adoption. Typically this is solved by recording a "minimum" version supported by the dependency (e.g. in the dependency) and then accommodating by behaving differently. For example, by gating the change as an opt-in to a newer version. Instead everyone is forced to trip over this. What a disaster.
  - steveklabnik 2y ago
    > Such inference seems inherently brittle and inevitably will break. This is just one data point, but again, in 80 releases, I can't remember the last one that did this to me. Obviously that's not a universal experience, as one of your siblings seems to run into it more than me. > Typically this is solved by recording a "minimum" version supported by the dependency The issue is the opposite. The dependency needs to be updated to fix the issue, not held back. > What a disaster. From "wait why did my CI pass yesterday but fail today" to pushing a commit with the fix, it was like ten minutes. Five of that was realizing that the breakage was because of a new Rust release. This happens so rarely to me that I didn't even think of it at first. Alternatively, don't update to the latest Rust. If the Rust version was pinned, this bug wouldn't manifest either.
    - xyzzy_plugh 2y ago
      The fact that you can just `cargo update` is besides the point. The standard library should never make a breaking change that breaks running code. Why is this the exception? If the problem is that it wouldn't be possible to add new traits, then solve that by gating access to new traits on a minimum version. It seems like the change should have been made as part of a new edition. This PR seems relevant: https://github.com/rust-lang/std-dev-guide/pull/66 https://github.com/rust-lang/std-dev-guide/pull/66 edit: I can't help but feel like we're moving the goal posts here. "Just don't update Rust" was the sort of response you'd encounter with nightly when nightly was basically required to do anything serious, but this is stable. There is even an accommodation for such changes! Why wasn't it used? This all just reeks of laziness and disregard for users.

by tyleo 2y ago

steveklabnik 2y ago

So, one area in which Rust is okay with "breaking" source compatibility is if some inference gets confused, and you need to add a type annotation. This issue was discovered before the release, and a new version of time was released to fix it. cargo update -p time and you're done. This doesn't happen very often. It bit one of my projects too, and it took me longer to realize that it was a new version release that caused the problem than it did to fix the problem. It's been so long since something like this happened to me I cannot remember when it happened last. Here's the upstream issue, if you're curious https://github.com/time-rs/time/issues/693 https://github.com/time-rs/time/issues/693

nequo 2y ago
I was wondering why and it looks like a new trait implementation in std made a type placeholder ambiguous in older versions of time? time was caught in a crater run when a new trait implementation was added to the standard library. While it is a breaking change, it is considered acceptable breakage because the alternative is never adding any trait implementations or methods. https://github.com/time-rs/time/issues/681#issuecomment-2143130755 https://github.com/time-rs/time/issues/681#issuecomment-2143...
- comex 2y ago
  I wish rustc had a way to 'pin' the output of type inference, in the same way that Cargo.lock pins the versions of dependencies. That way, old code would continue to compile based on what the types were originally inferred as, even if the code retroactively became ambiguous. This wouldn't be a perfect solution. Due to conditional compilation, any given variable in the source code might not exist on certain platforms, or it might even have a different type depending on the platform or configuration. So the question of "what type does this variable have" doesn't always have a single answer to record in a lockfile. And macros make things even harder. But even as an imperfect solution, I think it still has the potential to avert most real-world cases of breakage caused by Rust upgrades (and even dependency upgrades). As a bonus, the same mechanism could potentially also be used to detect breaking API changes.
  - estebank 2y ago
    I think the closest to what you're asking for is to build with a contemporary rustc version (have a rust-toolchain.toml in your repo). This isn't a satisfactory answer, but it is an available option nonetheless.
  - mtndew4brkfst 2y ago
    I wish rustc had a way to 'pin' the output of type inference Would that not be "take the outcome of said type inference, write it into the LHS as a type declaration or into the RHS as a generic parameter", aka make the inferred type permanently declared? We're not totally powerless for addressing the practical implication here, and I don't know how we could have stable yet still implicit type inference across compiler versions unless that type inference logic never changed ever again.
shotnothing 2y ago
sounds like they should have added a warning
OutOfHere 2y ago
Whenever I see this message in a repo: > An owner of this repository has limited the ability to comment to users that have contributed to this repository in the past. I immediately know it should never be used. time-rs is such a repo. They don't want you or need you there, and they don't care about your bug reports or even your questions.
- mtndew4brkfst 2y ago
  This shows a stark lack of empathy for maintainers. Per a maintainer’s Mastodon profile, the comment lock is temporary and specifically in response to driveby commenters dogpiling a bug report where the issue in question has a working fix that was already released months ago. Said abusive comment traffic probably often originated from blog posts and comment threads similar to this one.
  - OutOfHere 2y ago
    That hardly justifies the block. Users deserve empathy too, considering they have to bear the consequences if things go wrong. One now doesn't even know and cannot even estimate the number of other issues that must have gone unreported. It's not safe or wise to use a package that is so shrouded in mystery. It is in fact foolhardy. If maintainers don't want to address issues, they only have to ignore them. At least other users can then still discuss the issues in-between themselves. A lack of empathy for maintainers does not come into the picture. No one should have to go to Mastodon or wherever to find out about the author's stance. The readme of the repo is where the stance belongs. The readme is currently grossly misleading, asking users to use Discussions, but this is not possible. Overall it seems like a hazardous project to use.
    - hluska 2y ago
      This is an open source tool meaning that maintainers can communicate however they like. If you don’t like it, you can fork it and do things your own way. But there is absolutely nothing wrong with temporarily disabling comments. If anything, that’s a sign of self care and this wisdom. A little more empathy wouldn’t hurt you.
cropcirclbureau 2y ago
I work in a large code base with 1800+ crate dependencies (don't ask) with half of them being unstable crates not really published to crates.io. And yeah, we run into this issue a lot. The recent one was caused[0] by human error. It's usually human error but there's also a lot of cases due to Cargo's choice to go to pick the highest version that satisfies the constraints. It's a reasonable choice on Cargo's part but yes, our team has learned to be very conservative to diffs on `Cargo.lock` and most of our dependencies are pinned. [0]: https://github.com/denoland/deno_core/issues/797 https://github.com/denoland/deno_core/issues/797
xyzzy_plugh 2y ago
This is still a pretty astounding breakage possibility let alone that it actually broke. Such inference seems inherently brittle and inevitably will break. This sort of thing certainly does Rust no favours with regards to adoption. Typically this is solved by recording a "minimum" version supported by the dependency (e.g. in the dependency) and then accommodating by behaving differently. For example, by gating the change as an opt-in to a newer version. Instead everyone is forced to trip over this. What a disaster.
- steveklabnik 2y ago
  > Such inference seems inherently brittle and inevitably will break. This is just one data point, but again, in 80 releases, I can't remember the last one that did this to me. Obviously that's not a universal experience, as one of your siblings seems to run into it more than me. > Typically this is solved by recording a "minimum" version supported by the dependency The issue is the opposite. The dependency needs to be updated to fix the issue, not held back. > What a disaster. From "wait why did my CI pass yesterday but fail today" to pushing a commit with the fix, it was like ten minutes. Five of that was realizing that the breakage was because of a new Rust release. This happens so rarely to me that I didn't even think of it at first. Alternatively, don't update to the latest Rust. If the Rust version was pinned, this bug wouldn't manifest either.
  - xyzzy_plugh 2y ago
    The fact that you can just `cargo update` is besides the point. The standard library should never make a breaking change that breaks running code. Why is this the exception? If the problem is that it wouldn't be possible to add new traits, then solve that by gating access to new traits on a minimum version. It seems like the change should have been made as part of a new edition. This PR seems relevant: https://github.com/rust-lang/std-dev-guide/pull/66 https://github.com/rust-lang/std-dev-guide/pull/66 edit: I can't help but feel like we're moving the goal posts here. "Just don't update Rust" was the sort of response you'd encounter with nightly when nightly was basically required to do anything serious, but this is stable. There is even an accommodation for such changes! Why wasn't it used? This all just reeks of laziness and disregard for users.