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

regression: reached recursion limit while instantiating #79909

Closed
Mark-Simulacrum opened this issue Dec 10, 2020 · 9 comments
Closed

regression: reached recursion limit while instantiating #79909

Mark-Simulacrum opened this issue Dec 10, 2020 · 9 comments
Assignees
Labels
E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example 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.
Milestone

Comments

@Mark-Simulacrum
Copy link
Member

Would be great to see if this is expected -- I think we've previously seen some issues with combine, so maybe just an old version.

cc @matthewjasper @nikomatsakis

@Mark-Simulacrum Mark-Simulacrum added the regression-from-stable-to-beta Performance or correctness regression from stable to beta. label Dec 10, 2020
@Mark-Simulacrum Mark-Simulacrum added this to the 1.49.0 milestone Dec 10, 2020
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Dec 10, 2020
@wesleywiser wesleywiser self-assigned this Dec 11, 2020
@LeSeulArtichaut LeSeulArtichaut added the E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc label Dec 11, 2020
@camelid camelid added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 11, 2020
@dillona
Copy link
Contributor

dillona commented Dec 12, 2020

searched nightlies: from nightly-2020-10-23 to nightly-2020-12-12
regressed nightly: nightly-2020-10-28
searched commits: from fd54259 to 07e968b
regressed commit: a4d30a7

bisected with cargo-bisect-rustc v0.6.0

Host triple: aarch64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc

@Mark-Simulacrum Mark-Simulacrum added E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example and removed E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc labels Dec 12, 2020
@Mark-Simulacrum
Copy link
Member Author

cc @tmiasko -- any idea how #77876 could cause that? It seems quite unexpected to me but maybe I'm missing something

@tmiasko
Copy link
Contributor

tmiasko commented Dec 12, 2020

Underneath the recursion limit error lies a success story. The combine crate generates a lot of dead code and a new implementation is much better at removing it. The most significant aspect is removal of dead dyn casts, since without them there are fewer codegen items.

The crate has a configured recursion limit 67, which is exactly what is necessary. Since the codegen item collector uses memoization, removal of dyn cast might increase required recursion depth, which after changes it is at 71.

@Mark-Simulacrum
Copy link
Member Author

Hm, and 67 is lower than the default? I am a bit worried at us exposing such low level details, but it sounds like this should be won't fix.

@apiraino apiraino removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Dec 16, 2020
@apiraino
Copy link
Contributor

Discussed on Zulip and removing I-prioritize

@Mark-Simulacrum
Copy link
Member Author

I am inclined to agree that this is not readily fixable. I am not sure whether we made the right call exposing such internal implementation details to end users - it feels like recursion limits are always going to be error prone for people to configure. However, the limit of 67 configured by this crate is below the minimum Rust by default sets (128 currently) so I am not inclined to be too worried.

cc @hirartara as the author of this regressed crate

@wesleywiser
Copy link
Member

@Mark-Simulacrum I think you meant to cc @hiratara

@hiratara
Copy link

Hello,

I made that crate before only to practice Rust, and it has almost no content. Therefore, we don't need to worry at all if we can't build this crate.

Moreover, as far as I remember, the default value of recursion_limit was 64 when I created this crate. Now, I can build the crate if I delete the recursion_limit annotation.

@Mark-Simulacrum
Copy link
Member Author

Closing as won't fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example 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.
Projects
None yet
Development

No branches or pull requests

9 participants