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

the trait bound &[u8]: std::convert::From<&[_; 0]> is not satisfied #113238

Closed
Mark-Simulacrum opened this issue Jul 1, 2023 · 31 comments
Closed
Labels
P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Milestone

Comments

@Mark-Simulacrum Mark-Simulacrum added regression-from-stable-to-beta Performance or correctness regression from stable to beta. E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc labels Jul 1, 2023
@Mark-Simulacrum Mark-Simulacrum added this to the 1.71.0 milestone Jul 1, 2023
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jul 1, 2023
@hameerabbasi
Copy link
Contributor

This seems to already have been fixed on the main branch and also on cca7ee5 (which is what I get if I run cargo-bisect-rustc with the HEAD of the beta branch).

@compiler-errors
Copy link
Member

@hameerabbasi how did you build this? You need to build all the tests to reproduce this, too.

@compiler-errors
Copy link
Member

This seems to have regressed in #106704. From the UI test at the bottom of that PR, it seems to suggest that we've added a new &[u8]: From<gimli::read::endian_slice::EndianSlice<'_, Endian>>> implementation, which may have affected the inference/coercion somewhere in that program.

@compiler-errors compiler-errors removed the E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc label Jul 2, 2023
@Mark-Simulacrum
Copy link
Member Author

Mark-Simulacrum commented Jul 2, 2023

Seems plausible, and seems like that trait impl might be removable - though maybe a breaking change for gimli?

@cuviper
Copy link
Member

cuviper commented Jul 7, 2023

This is reproducible on stable if gimli 0.27.* is pulled in at all:

extern crate gimli;
fn main() {
    let _ = <&[u8]>::from(&[]);
}
error[E0277]: the trait bound `&[u8]: From<&[_; 0]>` is not satisfied
 --> src/main.rs:3:27
  |
3 |     let _ = <&[u8]>::from(&[]);
  |             ------------- ^^^ the trait `From<&[_; 0]>` is not implemented for `&[u8]`
  |             |
  |             required by a bound introduced by this call
  |
  = help: the trait `From<EndianSlice<'input, Endian>>` is implemented for `&'input [u8]`

But now that std is pulling that version of gimli, everyone gets to feel the pain. Without that impl, &[u8] would only have the reflexive From, driving the &[] into an unsizing coercion. An uncoerced From<&[T; N]> for &[T] seems totally plausible though - I wonder if we've tried that before? (It's way too late to add new impls to 1.71 though.)

seems like that trait impl might be removable - though maybe a breaking change for gimli?

I didn't look into how it's used, but EndianSlice is a public type, so yes that would be breaking to remove it.

Reverting #106704 is going to be hard too, because the new gimli is coming from backtrace (submodule in std) -> addr2line 0.19 -> gimli 0.27, and there are other backtrace updates that I expect we want to keep.

@cuviper
Copy link
Member

cuviper commented Jul 7, 2023

Hmm, actually it seems to build ok if I just downgrade to addr2line 0.18 (which also drops to gimli 0.26) here:

addr2line = { version = "0.19.0", optional = true, default-features = false }

So I guess it doesn't really need APIs of the newer version, but I don't know if we'll miss fixes or functionality. I can PR this as a release band-aid if others agree that it makes sense...

@Mark-Simulacrum
Copy link
Member Author

Hm, I think some folks have been more actively poking at backtrace lately, I don't know what we'd lose by downgrading there - cc @rust-lang/crate-maintainers (broader group but hopefully reaches the right people).

I guess the question is: are we comfortable downgrading addr2line as suggested by @cuviper for 1.70 (releasing next week)?

@Noratrieb
Copy link
Member

What's the plan for the future? I don't think we want to stay on old gimli forever. Hope that gimli removes the impl in a breaking release, which we could then pull in?

@cuviper
Copy link
Member

cuviper commented Jul 7, 2023

So I guess it doesn't really need APIs of the newer version,

Actually, there were no API changes in addr2line 0.18 to 0.19, only the dependency changes:
gimli-rs/addr2line@0.18.0...0.19.0

for 1.70 (releasing next week)?

That's 1.71.0.

What's the plan for the future?

Unknown. I can see if adding From<&[T; N]> for &[T] is actually feasible, but it's possible that this may introduce other ambiguities. Another (hacky) possibility is to get gimli to only remove that impl under its "rustc-dep-of-std" feature, since they document this feature as not part of their stable API, assuming any internal use can be converted to other means.

@workingjubilee
Copy link
Member

@Mark-Simulacrum The backtrace crate is going to need that gimli and object to be capable of later supporting AIX and also probably more recent platforms as well, as some object fixes were fixing platforms like watchOS. And it needs that gimli and addr2line to support the new DWARF formats which finally allow split debuginfo so we can maybe finally not have every Rust program be a hugely bloated binary on Linux and other Unix-y platforms.

Downgrading is an acceptable hotfix for stopping a regression from hitting stable, not for the long term.

@workingjubilee
Copy link
Member

Also we're actually on addr2line 0.20 now.

@cuviper
Copy link
Member

cuviper commented Jul 7, 2023

Also we're actually on addr2line 0.20 now.

The master branch is, but I'm only thinking about beta -> 1.71.0 at the moment. We can take the next release cycle to figure out a "better" change, or else decide to make everyone eat the inference failure.

@workingjubilee
Copy link
Member

workingjubilee commented Jul 7, 2023

Of course.

Anyone who is building for the targets we improved support for (I mean, aside from all Split DWARF, which was part of the 0.19 to 0.20 change) is probably building on nightly anyways, no worry about that.

@cuviper
Copy link
Member

cuviper commented Jul 7, 2023

FWIW, this one is actually a deref coercion, so it would need explicit From<&Vec<T>> for &[T] instead, if we're going to resolve these long-term with more standard library impls.

@cuviper
Copy link
Member

cuviper commented Jul 7, 2023

[INFO] [stdout] error[E0283]: type annotations needed
[INFO] [stdout]    --> src/program.rs:131:61
[INFO] [stdout]     |
[INFO] [stdout] 131 |                     camera_position: <[f32; 3]>::from(self.camera.position.into()),
[INFO] [stdout]     |                                      ----------------                      ^^^^
[INFO] [stdout]     |                                      |
[INFO] [stdout]     |                                      required by a bound introduced by this call
[INFO] [stdout]     |
[INFO] [stdout]     = note: multiple `impl`s satisfying `[f32; 3]: From<_>` found in the `core` crate:
[INFO] [stdout]             - impl<T> From<!> for T;
[INFO] [stdout]             - impl<T> From<(T, T, T)> for [T; 3];
[INFO] [stdout]             - impl<T> From<T> for T;

This one is a consequence of #97594, because Point3 can convert to either [T; 3] or (T, T, T), and now the array can convert from either of those as well. But I think the from was already redundant?

@workingjubilee
Copy link
Member

I can't figure out what is being exposed to make this a problem: rust-lang/backtrace-rs@ef961e2

Is it really just a failure to wall off private impls in std from inference?

@cuviper
Copy link
Member

cuviper commented Jul 7, 2023

Is it really just a failure to wall off private impls in std from inference?

Is there such a thing as a private impl? Even if these weren't pub types, dyn/impl Trait is possible. Not in inference though, I guess.

Anyway, this is the gimli change in 0.27, and it was "just" a clippy appeasement:
gimli-rs/gimli@6190562#diff-04070dfc44eacc67115d519df82d3dab3aa3472f857a9fddd6bb1684d19f5b94

@workingjubilee
Copy link
Member

Anyways I think we should land the downgrade of the dep for 1.71's sake but I also think that Rust's inference algorithm resolving if there's a single possible impl but not if there's 2, and not finding another way to find the actual impl to use, is a problem.

@cuviper
Copy link
Member

cuviper commented Jul 7, 2023

This general phenomenon is not new, e.g. #51916.

@workingjubilee
Copy link
Member

Correct, I am mostly saying that it's a problem that we have merely accepted it.

@workingjubilee
Copy link
Member

Though perhaps it was less of a problem when Clippy did not demand as many sacrifices.

@Mark-Simulacrum
Copy link
Member Author

Mark-Simulacrum commented Jul 8, 2023

I've included a downgrade to addr2line 1.18 in 1dcfc26 (targeting beta, 1.71).

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Jul 8, 2023
This prevents rust-lang#113238 from hitting stable while we sort out options for avoiding it. The downgrade
is expected to not affect any users on stable, since it primarily affects tier 3 targets.
@klensy
Copy link
Contributor

klensy commented Jul 9, 2023

Interesting that #111076 fixed similar (observing impls from private deps) issue with miniz_oxide, but not with gimli. miniz_oxide is direct dependency, while gimli indirect (addr2line->gimli).

Oh, its beta, so maybe it works here too.

MabezDev pushed a commit to esp-rs/rust that referenced this issue Jul 13, 2023
This prevents rust-lang#113238 from hitting stable while we sort out options for avoiding it. The downgrade
is expected to not affect any users on stable, since it primarily affects tier 3 targets.
@philipc
Copy link
Contributor

philipc commented Jul 28, 2023

Sorry, I wasn't aware that trait implementations can could cause this sort of problem. I'm happy to revert that change in gimli if it helps here.

Actually, there were no API changes in addr2line 0.18 to 0.19, only the dependency changes:

gimli is part of the public API of addr2line, so an API change in gimli is an API change in addr2line.

@cuviper
Copy link
Member

cuviper commented Jul 28, 2023

I'm updating the milestone so we don't forget that this still needs to be addressed in 1.72.

@cuviper cuviper modified the milestones: 1.71.0, 1.72.0 Jul 28, 2023
@apiraino
Copy link
Contributor

apiraino commented Aug 1, 2023

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Aug 1, 2023
@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 1, 2023
@wesleywiser wesleywiser added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Aug 3, 2023
@wesleywiser
Copy link
Member

Visited during T-compiler's weekly triage meeting. While this doesn't seem to be strictly under our purview, we are a bit concerned that the 1.72 release is coming up and this doesn't seem to have been mitigated in 1.72 yet.

Personally, I think if gimli is willing to remove the impl and ship a new major release (and coordinate that into a new addr2line release) that seems like the best medium term option. We may already be too late in the beta release cycle for that though so perhaps backporting the addr2line downgrade to 1.72 to ensure this regression does not accidentally get released makes sense?

xobs pushed a commit to betrusted-io/rust that referenced this issue Aug 5, 2023
This prevents rust-lang#113238 from hitting stable while we sort out options for avoiding it. The downgrade
is expected to not affect any users on stable, since it primarily affects tier 3 targets.
@cuviper
Copy link
Member

cuviper commented Aug 12, 2023

@philipc thanks for gimli-rs/gimli#669. Are you planning to make a release? (with addr2line as well if semver bumped)

We need to figure out the mitigation on 1.72-beta soon, but a downgrade to addr2line 0.18 doesn't work this time:

error[E0432]: unresolved imports `addr2line::LookupContinuation`, `addr2line::LookupResult`
   --> library/std/src/../../backtrace/src/symbolize/gimli.rs:163:25
    |
163 |         use addr2line::{LookupContinuation, LookupResult};
    |                         ^^^^^^^^^^^^^^^^^^  ^^^^^^^^^^^^ no `LookupResult` in the root
    |                         |
    |                         no `LookupContinuation` in the root

error[E0412]: cannot find type `SplitDwarfLoad` in crate `addr2line`
   --> library/std/src/../../backtrace/src/symbolize/gimli/elf.rs:461:22
    |
461 |     load: addr2line::SplitDwarfLoad<EndianSlice<'data, Endian>>,
    |                      ^^^^^^^^^^^^^^ not found in `addr2line`

error[E0599]: no method named `make_dwo` found for struct `Dwarf` in the current scope
   --> library/std/src/../../backtrace/src/symbolize/gimli/elf.rs:488:27
    |
488 |                 dwo_dwarf.make_dwo(&load.parent);
    |                           ^^^^^^^^ method not found in `Dwarf<EndianSlice<'_, LittleEndian>>`

Maybe those changes can be reverted, but I didn't try.

edit: going back to library/backtrace 4245978ca8169c40c088ff733825e4527f7b914c like 1.71 works.

@philipc
Copy link
Contributor

philipc commented Aug 12, 2023

gimli release PR is up. I'll do some testing and release gimli and addr2line tomorrow.

@cuviper
Copy link
Member

cuviper commented Aug 13, 2023

Upgrading beta std to addr2line v0.21.0 -> gimli v0.28.0 works! So I think we could do just that as a targeted fix for 1.72-beta, and a fuller update following rust-lang/backtrace-rs#557 on master for 1.73-nightly.

cuviper added a commit to cuviper/rust that referenced this issue Aug 14, 2023
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Aug 15, 2023
Upgrade std to gimli 0.28.0

Gimli 0.28 removed its `From<EndianSlice> for &[u8]` that was the root cause of rust-lang#113238.

This dependency update mirrors rust-lang/backtrace-rs#557, but since that doesn't require any code changes in `backtrace`, we can also apply that right away for our nested `std/backtrace` feature.
@Mark-Simulacrum
Copy link
Member Author

#114955 / #114825 fixed this, so closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests