rustc panic after merge of PR #76814 (refactor Binder) #83737
Assignees
Labels
C-bug
Category: This is a bug.
glacier
ICE tracked in rust-lang/glacier.
P-high
High priority
regression-from-stable-to-nightly
Performance or correctness regression from stable to nightly.
T-compiler
Relevant to the compiler team, which will review and decide on the PR/issue.
Comments
|
Can you link to the code that replicates this bug? It seems better to fix the bug rather than revert straight away, especially since #76814 was a pretty big refactor. |
jyn514
added
E-needs-mcve
rustbot
added
the
I-prioritize
|
The referenced line/function is here: The proc macro definition is here: |
|
Urgh. So, a MCVE would be super helpful here. And this error is popping up in a bit of code I'm not at all familiar with. But just by guessing for the few minutes I've looked at this, we might need to erase late bound regions here. As to exactly what's happening, I don't know. Because I would definitely prefer not to revert #76814, but if we need to, I'll live. |
|
@rustbot label +T-compiler |
rustbot
added
the
T-compiler
|
@richkadel if I were to get build artifacts with my potential fix from above, would you be able to check whether or not it works? |
|
One thing that might be helpful is to run with |
|
I'm not building this locally. The error is from our CI system that dryruns rustc updates. So there's a bit of a setup, for me to build against a version of rust we haven't transitioned to. I will try to get it set up though. If you push a draft PR with your proposed patch, I can cherry-pick that on top of a pull with the breaking change and try to validate your fix. Running with |
|
I'm ready for your patch Jack so send it my way as soon as you can and I
will try it.
…On Thu, Apr 1, 2021, 6:32 AM Rich Kadel ***@***.***> wrote:
I'm not building this locally. The error is from our CI system that
dryruns rustc updates. So there's a bit of a setup, for me to build against
a version of rust we haven't transitioned to. I will try to get it set up
though. If you push a draft PR with your proposed patch, I can cherry-pick
that on top of a pull with the breaking change and try to validate your fix.
Running with -Zverbose is going to be nearly equal effort for me to set
up.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#83737 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA5GMYSKFNXYPWGCPTHOK53TGRYXDANCNFSM42GCCLCA>
.
|
|
@richkadel here's a patch: jackh726@b0bd48c |
|
got it. I just tested locally with the latest rust without your patch and confirmed I can reproduce the error locally. I'm rebuilding with your patch now. Thanks! |
|
Unfortunately that didn't work. But I can try adding Here's what I'm getting as-is, with the patch: |
This comment has been minimized.
This comment has been minimized.
|
I need to try that again (the |
|
OK, this has |
|
Okay weird. which makes sense, because we erased all bound vars But witness has which looks the same except for the late bound regions, which I would have thought would have been replaced by |
|
@jackh726 |
|
I mean, the bound vars in the witness are definitely bound correctly:
So, really not sure. Maybe the |
|
@tmandry suggested sharing the output of https://gist.github.com/6d27a91c1c8e402a181ddc3d47bca747 |
|
@richkadel can I get you to try another patch for me? jackh726@fb97c61 I'm not expecting this to change anything, but running with I appreciate it! |
|
It's important to note: the output that we saw above with from the decl that was erased is actually coming directly from |
|
not found in umm...hashing issue? |
|
I believe I have also run into this issue compiling Tide as a dependency of tide-sqlx. log copied for posterityI'm not quite sure if I can get |
|
Some extra info - that was from running |
|
@jyn514 Fuchsia is not a tier 1 target, so we run our own CI and build Fuchsia against every bors merge. We don't use nightly, but instead try to update to the latest tip-of-tree once a week. We do the same for clang. A project of this size can exercise a lot of edge cases in the compiler, and we regularly find bugs in rustc and LLVM. The cost of fixing bugs is usually much lower right after they're introduced, so getting bug reports like this one up sooner than later is a big help (to everyone, hopefully). Without this we have ended up in a situation where we're blocked from updating the compiler for many weeks. The other reason we do this is because we're doing active feature development in the compiler (source-based code coverage) and are testing it out in our own infrastructure. So breakage can create a situation where we're blocked from testing out those changes. Breaking our rustc CI is not a huge deal, and we can always tolerate not updating the compiler one week. But the longer our CI is red and the further behind our production compiler gets, the more bugs can "sneak in" that will have to be unwound later. So we prefer to get it green as soon as we can. I think this is helpful to the project overall. But of course it has to be balanced against other concerns, like the overall impact and (as in this case) the difficulty of reverting and re-landing a big change. To be clear, I don't want us to be putting undue pressure on the project to revert a change because it inconveniences us. But if a fix isn't already in sight, it's worth asking the question. A clearer policy around when to revert might be helpful here; I think the release team has floated the idea of writing one before. |
|
Update: problem identified, just have to figure out best fix |
|
Assigning priority as discussed as part of the Prioritization Working Group procedure and removing @rustbot label -I-prioritize +P-high |
rustbot
added
P-high
|
@jackh726 @nikomatsakis - We found a second compiler failure that only crops up when enabling It breaks when compiling a third-party crate--arbitrary--for at least some versions of that crate. We are currently locking in version $ curl -L https://github.com/richkadel/arbitrary/archive/refs/tags/0.4.1.tar.gz --output arbitrary-0.4.1.tar.gz
$ tar xf arbitrary-0.4.1.tar.gz
$ arbitrary-0.4.1/src/lib.rs --crate-type rlib --edition=2018 --cfg=feature=\"derive\" --cfg=feature=\"derive_arbitrary\" -Zinstrument-coverage --extern derive_arbitrary=location/of/previously/built/libderive_arbitrary-707386e28fd45df8.so
error: symbol `_RINvMNtNtCsdUfrJSq0CiO_4core3ptr7mut_ptrOINtNtCsf2kEqRrZ9O4_5alloc3vec3VecINtNtBI_5boxed3BoxDNtNtNtNtB7_4iter6traits8iterator8Iteratorp4ItemcEL_EE4castuECshGpAVYOtgW1_3lib` is already defined
error: aborting due to previous errorThe mangled name demangles to: Is this error resolved by your proposed fix as well? |
|
Cross posting from https://github.com/rust-lang/rust/pull/83870/files#issuecomment-814514317 We found the code that was triggering this. A closure was being coerced to a function pointer in an async fn. The unused let run_server: fn(
stream: ControlRequestStream,
control: Arc<Mutex<dyn ControlTrait>>,
) -> Pin<Box<dyn Future<Output = Result<(), Error>>>> =
|stream, control| Box::pin(run_brightness_server(stream, control)); |
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
Apr 9, 2021
…atsakis Don't concatenate binders across types Partially addresses rust-lang#83737 There's actually two issues that I uncovered in rust-lang#83737. The first is that we are concatenating bound vars across types, i.e. in ``` F: Fn(&()) -> &mut (dyn Future<Output = ()> + Unpin) ``` the bound vars on `Future` get set as `for<anon>` since those are the binders on `Fn(&()`. This is obviously wrong, since we should only concatenate directly nested trait refs. This is solved here by introducing a new `TraitRefBoundary` scope, that we put around the "syntactical" trait refs and basically don't allow concatenation across. Now, this alone *shouldn't* be a super terrible problem. At least not until you consider the other issue, which is a much more elusive and harder to design a "perfect" fix. A repro can be seen in: ``` use core::future::Future; async fn handle<F>(slf: &F) where F: Fn(&()) -> &mut (dyn for<'a> Future<Output = ()> + Unpin), { (slf)(&()).await; } ``` Notice the `for<'a>` around `Future`. Here, `'a` is unused, so the `for<'a>` Binder gets changed to a `for<>` Binder in the generator witness, but the "local decl" still has it. This has heavy intersections with region anonymization and erasing. Luckily, it's not *super* common to find this unique set of circumstances. It only became apparently because of the first issue mentioned here. However, this *is* still a problem, so I'm leaving rust-lang#83737 open. r? `@nikomatsakis`
fanninpm
added a commit
to fanninpm/glacier
that referenced
this issue
Apr 9, 2021
Issue: rust-lang/rust#83737
Alexendoo
removed
the
E-needs-mcve
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this issue
Apr 29, 2021
Add HAS_RE_LATE_BOUND if there are bound vars Fixes rust-lang#83737 Fixes rust-lang#84604 I can rename `HAS_RE_LATE_BOUND`, to something like `HAS_LATE_BOUND_VARS`. r? `@nikomatsakis`
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this issue
Apr 29, 2021
Add HAS_RE_LATE_BOUND if there are bound vars Fixes rust-lang#83737 Fixes rust-lang#84604 I can rename `HAS_RE_LATE_BOUND`, to something like `HAS_LATE_BOUND_VARS`. r? ``@nikomatsakis``
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this issue
Apr 29, 2021
Add HAS_RE_LATE_BOUND if there are bound vars Fixes rust-lang#83737 Fixes rust-lang#84604 I can rename `HAS_RE_LATE_BOUND`, to something like `HAS_LATE_BOUND_VARS`. r? ```@nikomatsakis```
jackh726
added a commit
to jackh726/rust
that referenced
this issue
Apr 29, 2021
…r=nikomatsakis Don't rebind in `transitive_bounds_that_define_assoc_type` Fixes rust-lang#83737 Fixes rust-lang#84604 Also fixes another issue that I don't have a test for, popped up in [zulip](https://rust-lang.zulipchat.com/#narrow/stream/144729-wg-traits/topic/Duplicate.20symbol.20error.20.2384604/near/236570445) r? `@nikomatsakis`
jackh726
added a commit
to jackh726/rust
that referenced
this issue
Apr 29, 2021
…r=nikomatsakis Don't rebind in `transitive_bounds_that_define_assoc_type` Fixes rust-lang#83737 Fixes rust-lang#84604 Also fixes another issue that I don't have a test for, popped up in [zulip](https://rust-lang.zulipchat.com/#narrow/stream/144729-wg-traits/topic/Duplicate.20symbol.20error.20.2384604/near/236570445) r? ``@nikomatsakis``
jackh726
added a commit
to jackh726/rust
that referenced
this issue
Apr 29, 2021
…r=nikomatsakis Don't rebind in `transitive_bounds_that_define_assoc_type` Fixes rust-lang#83737 Fixes rust-lang#84604 Also fixes another issue that I don't have a test for, popped up in [zulip](https://rust-lang.zulipchat.com/#narrow/stream/144729-wg-traits/topic/Duplicate.20symbol.20error.20.2384604/near/236570445) r? ```@nikomatsakis```
jackh726
added a commit
to jackh726/rust
that referenced
this issue
Apr 29, 2021
…r=nikomatsakis Don't rebind in `transitive_bounds_that_define_assoc_type` Fixes rust-lang#83737 Fixes rust-lang#84604 Also fixes another issue that I don't have a test for, popped up in [zulip](https://rust-lang.zulipchat.com/#narrow/stream/144729-wg-traits/topic/Duplicate.20symbol.20error.20.2384604/near/236570445) r? ````@nikomatsakis````
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
It looks PR #76814 introduced a bug. Is it possible to revert this change until resolved?
cc: @jackh726 , @nikomatsakis
Note, line 154 starts a function declared within a proc macro:
cc: @tmandry
The text was updated successfully, but these errors were encountered: