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.

Sign up for GitHub

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

Jump to bottom

Implement overflow for infinite implied lifetime bounds #138676

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Implement overflow for infinite implied lifetime bounds #138676

wants to merge 1 commit into from

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Mar 18, 2025 •
edited
Loading

Not a great error message, but better than a hang

Fixes #138665

r? lcnr

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 18, 2025
@compiler-errors
Copy link
Member Author

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 18, 2025
@bors
Auto merge of rust-lang#138676 - compiler-errors:overflow-implied-bou…
b32fc44 
…nds, r=<try>

Implement overflow for infinite implied lifetime bounds

Not a great error message, but better than a hang

r? lcnr
@bors
Copy link
Contributor

bors commented Mar 18, 2025

⌛ Trying commit 7ed922a with merge b32fc44...

@fmease
Copy link
Member

fmease commented Mar 18, 2025 •
edited
Loading

I assume this also fixes #102966 and #115407? (Also CC closed PR #103309 which had a different and unsound approach)

@bors
Copy link
Contributor

bors commented Mar 18, 2025

☀️ Try build successful - checks-actions
Build commit: b32fc44 (b32fc44666c052e7cf07ab471bfb6f9ec571902f)

@compiler-errors
Copy link
Member Author

@rust-timer build b32fc44
@craterbot check

@rust-timer

This comment has been minimized.

Sign in to view
@craterbot
Copy link
Collaborator

👌 Experiment pr-138676 created and queued.
🤖 Automatically detected try build b32fc44
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 18, 2025
@craterbot
Copy link
Collaborator

🚧 Experiment pr-138676 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (b32fc44): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary 3.0%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 3.0% [3.0%, 3.0%] 1
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 3.0% [3.0%, 3.0%] 1

Cycles

Results (secondary 8.3%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 8.3% [8.3%, 8.3%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 776.359s -> 777.553s (0.15%)
Artifact size: 365.12 MiB -> 365.13 MiB (0.00%)

lcnr
lcnr reviewed Mar 19, 2025
View reviewed changes
compiler/rustc_hir_analysis/src/outlives/implicit_infer.rs
loop {
let mut predicates_added = false;
for i in 0.. {
let mut predicates_added: Option<Vec<_>> = None;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just use a Vec with .is_empty()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 tru

lcnr
lcnr reviewed Mar 19, 2025
View reviewed changes
tests/ui/implied-bounds/overflow.rs
@@ -0,0 +1,11 @@
trait Tailed<'a>: 'a {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls add doc comment/link to issue

lcnr
lcnr reviewed Mar 19, 2025
View reviewed changes
compiler/rustc_hir_analysis/src/outlives/implicit_infer.rs
)
.emit();
for id in ids {
global_inferred_outlives.shift_remove(&id.to_def_id());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that seems suboptimal 🤔 what happens if u don't remove the added implied bounds here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hangs in both the old and need solvers due to RECURSION_LIMIT deep types having to be normalized

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ill put a comment but it's necessary im p sure

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense, we get very deep assoc items. I feel like we can also have cycles of length 2 in which case we still get a hang as we don't remove the implied bounds for the other cycle participant? 🤔

vibes, but maybe just fatal error? 😁

lcnr
lcnr reviewed Mar 19, 2025
View reviewed changes
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:3 nits, but otherwise r=me

@craterbot
Copy link
Collaborator

🎉 Experiment pr-138676 is completed!
📊 2 regressed and 1 fixed (599834 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Mar 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lcnr lcnr lcnr left review comments

Assignees

@lcnr lcnr

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

rustc stuck during assoc normalization
7 participants
@compiler-errors @bors @fmease @rust-timer @craterbot @lcnr @rustbot