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

Try to point out when edition 2024 lifetime capture rules cause borrowck issues #131186

Merged
merged 2 commits into from
Oct 31, 2024

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Oct 3, 2024

Lifetime capture rules in 2024 are modified to capture more lifetimes, which sometimes lead to some non-local borrowck errors. This PR attempts to link these back together with a useful note pointing out the capture rule changes.

This is not a blocking concern, but I'd appreciate feedback (though, again, I'd like to stress that I don't want to block this PR on this): I'm worried about this note drowning in the sea of other diagnostics that borrowck emits. I was tempted to change the level of the note to .span_warn just so it would show up in a different color. Thoughts?

Fixes #130545

Opening as a draft first since it's stacked on #131183.
r? @ghost

@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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Oct 3, 2024
Applicability::MaybeIncorrect,
);
} else {
diag.span_help(
Copy link
Contributor

@estebank estebank Oct 3, 2024

Choose a reason for hiding this comment

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

Why isn't this a structured suggestion? Edit: because it's checking if it's not the current crate, got it.

@compiler-errors
Copy link
Member Author

r? @estebank, would you be willing to help out with the wording/etc. here? Otherwise, please feel free to reassign. I know the lifetime logic is kinda sketchy, but after all it's a diagnostic so 🤷

@compiler-errors compiler-errors marked this pull request as ready for review October 4, 2024 03:34
@rustbot
Copy link
Collaborator

rustbot commented Oct 4, 2024

HIR ty lowering was modified

cc @fmease

@compiler-errors
Copy link
Member Author

Let's see the cost of the cross-crate encoding of opaque origin.

@bors try @rust-timer

@bors
Copy link
Contributor

bors commented Oct 4, 2024

⌛ Trying commit 3b98411 with merge 17a60f6...

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 4, 2024
…rrowck, r=<try>

Try to point out when edition 2024 lifetime capture rules cause borrowck issues

Lifetime capture rules in 2024 are modified to capture more lifetimes, which sometimes lead to some non-local borrowck errors. This PR attempts to link these back together with a useful note pointing out the capture rule changes.

This is not a blocking concern, but I'd appreciate feedback (though, again, I'd like to stress that I don't want to block this PR on this): I'm worried about this note drowning in the sea of other diagnostics that borrowck emits. I was tempted to change the level of the note to `.span_warn` just so it would show up in a different color. Thoughts?

Fixes rust-lang#130545

Opening as a draft first since it's stacked on rust-lang#131183.
r? `@ghost`
@bors
Copy link
Contributor

bors commented Oct 4, 2024

☀️ Try build successful - checks-actions
Build commit: 17a60f6 (17a60f6d33bf58ecd032c2545d56965569adb3e8)

@compiler-errors
Copy link
Member Author

ugh lol

@rust-timer build 17a60f6

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (17a60f6): 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 (secondary -2.4%)

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)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.4% [-3.0%, -2.1%] 4
All ❌✅ (primary) - - 0

Cycles

Results (secondary 3.9%)

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)
3.9% [3.2%, 4.8%] 6
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

Results (primary 0.1%, secondary 0.2%)

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.1% [0.0%, 0.3%] 32
Regressions ❌
(secondary)
0.2% [0.2%, 0.2%] 6
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.1% [0.0%, 0.3%] 32

Bootstrap: 774.169s -> 772.785s (-0.18%)
Artifact size: 342.02 MiB -> 342.06 MiB (0.01%)

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Oct 24, 2024

☔ The latest upstream changes (presumably #131985) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

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

r=me after rebase

LL | println!("{a}");
| --- immutable borrow later used here
|
note: this call may capture more lifetimes since Rust 2024 has adjusted `impl Trait` lifetime capture rules
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of the following?

Suggested change
note: this call may capture more lifetimes since Rust 2024 has adjusted `impl Trait` lifetime capture rules
note: this call may be capturing more lifetimes than intended, because Rust 2024 has adjusted the `impl Trait` lifetime capture rules

|
LL | let a = display_len(&x);
| ^^^^^^^^^^^^^^^
help: add a `use<..>` precise capturing bound to avoid overcapturing
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd let the suggestion show the syntax and leave the text easier to read

Suggested change
help: add a `use<..>` precise capturing bound to avoid overcapturing
help: add a precise capturing bound to avoid overcapturing in Rust 2024 edition

Copy link
Member Author

Choose a reason for hiding this comment

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

I think saying "in Rust 2024" is redundant (that's the current edition enabled) and also going to be wrong as soon as we bump the edition further. I'll leave out the use<..> part tho.

@compiler-errors
Copy link
Member Author

@bors r=estebank

@bors
Copy link
Contributor

bors commented Oct 30, 2024

📌 Commit b2fa80e has been approved by estebank

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 30, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 30, 2024
…rrowck, r=estebank

Try to point out when edition 2024 lifetime capture rules cause borrowck issues

Lifetime capture rules in 2024 are modified to capture more lifetimes, which sometimes lead to some non-local borrowck errors. This PR attempts to link these back together with a useful note pointing out the capture rule changes.

This is not a blocking concern, but I'd appreciate feedback (though, again, I'd like to stress that I don't want to block this PR on this): I'm worried about this note drowning in the sea of other diagnostics that borrowck emits. I was tempted to change the level of the note to `.span_warn` just so it would show up in a different color. Thoughts?

Fixes rust-lang#130545

Opening as a draft first since it's stacked on rust-lang#131183.
r? `@ghost`
@bors
Copy link
Contributor

bors commented Oct 30, 2024

⌛ Testing commit b2fa80e with merge 3e04a23...

@bors bors mentioned this pull request Oct 30, 2024
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Oct 30, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 30, 2024
@compiler-errors
Copy link
Member Author

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 30, 2024
@bors
Copy link
Contributor

bors commented Oct 31, 2024

☔ The latest upstream changes (presumably #132371) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 31, 2024
@compiler-errors
Copy link
Member Author

@bors r=estebank

@bors
Copy link
Contributor

bors commented Oct 31, 2024

📌 Commit c145779 has been approved by estebank

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 31, 2024
@bors
Copy link
Contributor

bors commented Oct 31, 2024

⌛ Testing commit c145779 with merge c8b8378...

@bors
Copy link
Contributor

bors commented Oct 31, 2024

☀️ Test successful - checks-actions
Approved by: estebank
Pushing c8b8378 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 31, 2024
@bors bors merged commit c8b8378 into rust-lang:master Oct 31, 2024
7 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve borrow checking error in cases where + use<> could be used
6 participants