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

Detect non-lifetime binder params shadowing item params #128357

Merged
merged 1 commit into from
Jul 31, 2024

Conversation

compiler-errors
Copy link
Member

We should check that for<T> shadows T from an item in the same way that for<'a> shadows 'a from an item.

r? @petrochenkov since you're familiar w the nuances of rib kinds

@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 Jul 29, 2024

// Break at mod level, to account for nested items which are
// allowed to shadow generic param names.
if matches!(parent_rib.kind, RibKind::Module(..)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

The check for lifetime shadowing uses LifetimeRibKind::Item below, but we use RibKind::Module here. The only case where I expect this to matter is nested items, in which case RibKind::Module operates fine enough, but I may be missing some subtle case.

Comment on lines +15 to +16
fn repeated() where for<T, T> (): Sized {}
//~^ ERROR the name `T` is already used for a generic parameter
Copy link
Member Author

Choose a reason for hiding this comment

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

This one already existed, for the record. Just exercising it for good measure.

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 30, 2024

📌 Commit 454c600 has been approved by petrochenkov

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 Jul 30, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 30, 2024
…ime-binder, r=petrochenkov

Detect non-lifetime binder params shadowing item params

We should check that `for<T>` shadows `T` from an item in the same way that `for<'a>` shadows `'a` from an item.

r? `@petrochenkov` since you're familiar w the nuances of rib kinds
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 30, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#127543 (More unsafe attr verification)
 - rust-lang#128357 (Detect non-lifetime binder params shadowing item params)
 - rust-lang#128367 (CI: rfl: build the generated doctests and documentation)
 - rust-lang#128376 (Mark `Parser::eat`/`check` methods as `#[must_use]`)
 - rust-lang#128379 (the output in stderr expects panic-unwind)
 - rust-lang#128380 (make `///` doc comments compatible with naked functions)
 - rust-lang#128382 (cargo-miri: better error when we seem to run inside bootstrap but something is wrong)
 - rust-lang#128398 (tidy: Fix quote in error message)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 30, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#128357 (Detect non-lifetime binder params shadowing item params)
 - rust-lang#128367 (CI: rfl: build the generated doctests and documentation)
 - rust-lang#128376 (Mark `Parser::eat`/`check` methods as `#[must_use]`)
 - rust-lang#128379 (the output in stderr expects panic-unwind)
 - rust-lang#128380 (make `///` doc comments compatible with naked functions)
 - rust-lang#128382 (cargo-miri: better error when we seem to run inside bootstrap but something is wrong)
 - rust-lang#128398 (tidy: Fix quote in error message)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 40edd4f into rust-lang:master Jul 31, 2024
6 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Jul 31, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 31, 2024
Rollup merge of rust-lang#128357 - compiler-errors:shadowed-non-lifetime-binder, r=petrochenkov

Detect non-lifetime binder params shadowing item params

We should check that `for<T>` shadows `T` from an item in the same way that `for<'a>` shadows `'a` from an item.

r? ``@petrochenkov`` since you're familiar w the nuances of rib kinds
@Kobzol
Copy link
Contributor

Kobzol commented Jul 31, 2024

@rust-timer build 4822434

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4822434): comparison URL.

Overall result: ❌ regressions - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.8% [0.3%, 1.9%] 21
Regressions ❌
(secondary)
0.6% [0.2%, 2.6%] 8
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.8% [0.3%, 1.9%] 21

Max RSS (memory usage)

Results (primary -2.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
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.2% [-2.2%, -2.2%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.2% [-2.2%, -2.2%] 1

Cycles

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

Binary size

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

Bootstrap: 769.587s -> 768.835s (-0.10%)
Artifact size: 331.78 MiB -> 331.93 MiB (0.05%)

@rustbot rustbot added the perf-regression Performance regression. label Aug 1, 2024
@Kobzol
Copy link
Contributor

Kobzol commented Aug 1, 2024

Seems like this has regressed trait-heavy code (mostly diesel) somewhat. I don't think that we need to deal with that though, given that this just implements new/missing functionality.

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

Only walk ribs to collect possibly shadowed params if we are adding params in our new rib

No need to collect params from parent ribs if we literally have no params to declare in this new rib.

Attempt to win back some of the perf in rust-lang#128357 (comment).

Please review with whitespace *off*, the diff should be like 2 lines.

r? petrochenkov
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 8, 2024
…, r=petrochenkov

Only walk ribs to collect possibly shadowed params if we are adding params in our new rib

No need to collect params from parent ribs if we literally have no params to declare in this new rib.

Attempt to win back some of the perf in rust-lang#128357 (comment).

Please review with whitespace *off*, the diff should be like 2 lines.

r? petrochenkov
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 8, 2024
…, r=petrochenkov

Only walk ribs to collect possibly shadowed params if we are adding params in our new rib

No need to collect params from parent ribs if we literally have no params to declare in this new rib.

Attempt to win back some of the perf in rust-lang#128357 (comment).

Please review with whitespace *off*, the diff should be like 2 lines.

r? petrochenkov
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 8, 2024
…, r=petrochenkov

Only walk ribs to collect possibly shadowed params if we are adding params in our new rib

No need to collect params from parent ribs if we literally have no params to declare in this new rib.

Attempt to win back some of the perf in rust-lang#128357 (comment).

Please review with whitespace *off*, the diff should be like 2 lines.

r? petrochenkov
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Aug 12, 2024
…chenkov

Only walk ribs to collect possibly shadowed params if we are adding params in our new rib

No need to collect params from parent ribs if we literally have no params to declare in this new rib.

Attempt to win back some of the perf in rust-lang/rust#128357 (comment).

Please review with whitespace *off*, the diff should be like 2 lines.

r? petrochenkov
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf-regression Performance regression. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants