Skip to content

Backport new sized-hierarchy trait bounds in old ways #20100

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
Merged
merged 2 commits into from
Jun 26, 2025
Merged

Backport new sized-hierarchy trait bounds in old ways #20100

merged 2 commits into from
Jun 26, 2025

Conversation

ShoyuVanilla
Copy link
Member

@ShoyuVanilla ShoyuVanilla commented Jun 25, 2025 •
edited by rustbot
Loading

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 25, 2025
ShoyuVanilla
ShoyuVanilla commented Jun 25, 2025
View reviewed changes
ShoyuVanilla
ShoyuVanilla commented Jun 26, 2025
View reviewed changes
ChayimFriedman2
ChayimFriedman2 approved these changes Jun 26, 2025
View reviewed changes
Copy link
Contributor

@ChayimFriedman2 ChayimFriedman2 left a comment

Choose a reason for hiding this comment

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

Looks good! Did you check this indeed solves the type mismatches etc.? Also, can you measure the perf impact, just to be sure?

crates/ide-assists/src/handlers/generate_enum_is_method.rs
@@ -52,7 +54,7 @@ pub(crate) fn generate_enum_is_method(acc: &mut Assists, ctx: &AssistContext<'_>
let fn_name = format!("is_{}", &to_lower_snake_case(&variant_name.text()));

// Return early if we've found an existing new fn
let impl_def = find_struct_impl(ctx, &parent_enum, &[fn_name.clone()])?;
let impl_def = find_struct_impl(ctx, &parent_enum, slice::from_ref(&fn_name))?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this an application of a new Clippy lint or something?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure because this is from Lukas's commit in his minicore adjust branch but this looks better anyway 😄

Copy link
Member

Choose a reason for hiding this comment

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

Yes it was (hard erroring clippy lint)

@ShoyuVanilla
Copy link
Member Author

ShoyuVanilla commented Jun 26, 2025 •
edited
Loading

Looks good! Did you check this indeed solves the type mismatches etc.? Also, can you measure the perf impact, just to be sure?

I haven't tested this thoroughly but based on the result in CI's analysis-stats, most of the inference regressions are gone, but still some MIR evaluation failures on self and some perf regression on body lowering.
I'll check then more but it will take some hours since I'm at work now 😅

@Veykril Veykril mentioned this pull request Jun 26, 2025
Closed
@ShoyuVanilla ShoyuVanilla force-pushed the ignore-sized-hierarchy branch 3 times, most recently from 1c63a26 to ad12633 Compare June 26, 2025 14:21
@ShoyuVanilla ShoyuVanilla force-pushed the ignore-sized-hierarchy branch from ad12633 to 953e9d1 Compare June 26, 2025 14:22
@ShoyuVanilla
Copy link
Member Author

ShoyuVanilla commented Jun 26, 2025 •
edited
Loading

Without this PR on rustc 1.87.0(stable)

Database loaded:     39.59s, 437minstr, 85mb (metadata 1.85s, 338kinstr, 944kb; build 37.58s, 9005kinstr, 41kb)
  item trees: 1302
  dependency lines of code: 2_000_017, item trees: 4_588
  dependency item stats: traits: 1_376, impl: 22_125, mods: 5_251, macro calls: 7_004, macro rules: 1_845
Item Tree Collection: 4.14s, 45ginstr, 733mb
  Total Statistics:
    crates: 57, mods: 1152, decls: 29260, bodies: 28393, adts: 2043, consts: 1306
  Workspace:
    traits: 101, macro_rules macros: 20, proc_macros: 1
    lines of code: 477_654, item trees: 1_302
    usages: traits: 166, impl: 4_100, mods: 1_153, macro calls: 159, macro rules: 117
  Dependencies:
    lines of code: 2_000_017, item trees: 4_588
    declarations: traits: 1_376, impl: 22_125, mods: 5_251, macro calls: 7_004, macro rules: 1_845
Item Collection:     9.15s, 52ginstr, 938mb
Body lowering:       4.65s, 31ginstr, 609mb
  exprs: 863089, ??ty: 107 (0%), ?ty: 146 (0%), !ty: 9
  pats: 188487, ??ty: 7 (0%), ?ty: 10 (0%), !ty: 0
  panics: 0
Inference:           105.38s, 549ginstr, 1135mb
MIR lowering:        14.98s, 75ginstr, 393mb
Mir failed bodies: 1104 (4%)
Data layouts:        83.39ms, 334minstr, 16mb
Failed data layouts: 6 (0%)
Const evaluation:    51.37ms, 224minstr, 3928kb
Failed const evals: 5 (0%)
Total:               139.67s, 759ginstr, 2248mb

With this PR on rustc 1.89.0(beta.2)

Database loaded:     34.53s, 488minstr, 100mb (metadata 1.25s, 829kinstr, 1067kb; build 33.09s, 4947kinstr, 47kb)
  item trees: 1302
  dependency lines of code: 2_393_237, item trees: 5_034
  dependency item stats: traits: 1_488, impl: 23_863, mods: 5_680, macro calls: 8_103, macro rules: 1_973
Item Tree Collection: 4.73s, 52ginstr, 845mb
  Total Statistics:
    crates: 57, mods: 1152, decls: 29260, bodies: 28393, adts: 2043, consts: 1306
  Workspace:
    traits: 101, macro_rules macros: 20, proc_macros: 1
    lines of code: 477_654, item trees: 1_302
    usages: traits: 166, impl: 4_100, mods: 1_153, macro calls: 159, macro rules: 117
  Dependencies:
    lines of code: 2_393_237, item trees: 5_034
    declarations: traits: 1_488, impl: 23_863, mods: 5_680, macro calls: 8_103, macro rules: 1_973
Item Collection:     10.05s, 55ginstr, 1006mb
Body lowering:       5.84s, 40ginstr, 749mb
  exprs: 866208, ??ty: 105 (0%), ?ty: 145 (0%), !ty: 9
  pats: 190871, ??ty: 7 (0%), ?ty: 10 (0%), !ty: 0
  panics: 0
Inference:           104.19s, 550ginstr, 1033mb
MIR lowering:        19.30s, 94ginstr, 488mb
Mir failed bodies: 60 (0%)
Data layouts:        86.02ms, 336minstr, 16mb
Failed data layouts: 0 (0%)
Const evaluation:    45.92ms, 206minstr, 3178kb
Failed const evals: 6 (0%)
Total:               145.61s, 799ginstr, 2401mb

Diff

image

Ran on the same revision of rust-analyzer, 96a2531 for fair comparison

No idea why there was so many MIR failed bodies with before, as we didn't have so many before this beta regression but I think that most of the regressions are gone overall

ChayimFriedman2
ChayimFriedman2 approved these changes Jun 26, 2025
View reviewed changes
Copy link
Contributor

@ChayimFriedman2 ChayimFriedman2 left a comment

Choose a reason for hiding this comment

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

Okay let's merge this then!

@ChayimFriedman2 ChayimFriedman2 added this pull request to the merge queue Jun 26, 2025
Merged via the queue into rust-lang:master with commit a6c1fa0 Jun 26, 2025
14 checks passed
@ShoyuVanilla ShoyuVanilla deleted the ignore-sized-hierarchy branch June 27, 2025 00:30
@Veykril Veykril mentioned this pull request Jul 6, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Veykril Veykril Veykril left review comments

@ChayimFriedman2 ChayimFriedman2 ChayimFriedman2 approved these changes

Assignees
No one assigned
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ShoyuVanilla @Veykril @ChayimFriedman2 @rustbot