fixed: differentiation between uninitialized and possibly uninitialized

[mazam-97]

View all comments

Fix: #157267

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @tiif (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: borrowck, compiler
• borrowck, compiler expanded to 73 candidates
• Random selection from 21 candidates

[Unique-Usman]

Thanks for the pr, can we change it commit message to something like "avoid reporting unreachable initializations for uninitialized uses" ?

[Unique-Usman]

Also, we need to add more test i.e one for match expression, one or two if we have multiple block of if or match expressions.

[Unique-Usman]

cc: @estebank

[Unique-Usman]

Here, we might want to have something like

"`x` used here, but all known initializations occur on control-flow paths that do not reach this use"  

or a shorter version

`x` used here, but it is not initialized on any path leading to this point

because, x was actually initialized but, it was initilized on another branch or block which will never reach the error block.

There would probably be another else if statement in this block -> https://github.com/rust-lang/rust/blob/main/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs#L820C86-L845C40

View changes since the review

[mazam-97]

I think what you say makes sense. The error should be more explicit. I will add one more block.

[mazam-97]

Thanks for the pr, can we change it commit message to something like "avoid reporting unreachable initializations for uninitialized uses" ?

Thanks for reviewing the PR @Unique-Usman. I will modify the commit messge as suggested.

[mazam-97]

Also, we need to add more test i.e one for match expression, one or two if we have multiple block of if or match expressions.

Sure, I will add more tests having match expression and if else blocks.

[Unique-Usman]

This should be something like

fn main() {
    let x;

    match true {
        true => x = 42,
        false =>  println!("{x}"), //~ ERROR E0381
    }
}

View changes since the review

[mazam-97]

Addressed.

[Unique-Usman]

if you want to have multiple match, do not nested it like this. You can have something like this.

enum PrimaryColor {
  RED,
  BLUE,
  GREEN
}

fn main() {
    let x;
    let pc = PrimaryColor::RED,

    match pc {
        PrimaryColor::RED => x = 1,
        PrimaryColor::BLUE => x = 3,
        PrimaryColor::GREEN =>  println!("{x}"); //~ ERROR E0381
    }

}

Also, something similar for the if and else i.e for multiple branch.

View changes since the review

[mazam-97]

Addressed. Added a test having an enum with match

[estebank]

As follow up work, we should point at the places where when a condition is true, the binding would be initialized, explaining that if that condition isn't true, the binding wouldn't be initialized. This is because there are some code patterns like state machines where you are doing work on a loop, and the first loop cycle a certain condition would be true, inicializing the binding, but the compiler/type system can't see that. The person that wrote the code might get confused when looking at the other branches and seeing that the binding is initialized elsewhere, so lets be proactive and point at the conditions.

Given:

let x;
if foo { x = 1; }
println!("{x}");

We should point at foo saying "if this condition isn't met, then x isn't initialized", and point at x = 1; saying "if foo isn't true, then this won't be reached". We should also account for multiple paths initializing x, and avoid having more than a handful of labels pointing at initializers (imagine 1000 places where x is initialized, and we missed one, we don't want to spam the output).

Doing a review on the code now, I feel like this is likely in a mergeable state already.

[mazam-97]

As follow up work, we should point at the places where when a condition is true, the binding would be initialized, explaining that if that condition isn't true, the binding wouldn't be initialized. This is because there are some code patterns like state machines where you are doing work on a loop, and the first loop cycle a certain condition would be true, inicializing the binding, but the compiler/type system can't see that. The person that wrote the code might get confused when looking at the other branches and seeing that the binding is initialized elsewhere, so lets be proactive and point at the conditions.

Given:

let x;
if foo { x = 1; }
println!("{x}");

We should point at foo saying "if this condition isn't met, then x isn't initialized", and point at x = 1; saying "if foo isn't true, then this won't be reached". We should also account for multiple paths initializing x, and avoid having more than a handful of labels pointing at initializers (imagine 1000 places where x is initialized, and we missed one, we don't want to spam the output).

Doing a review on the code now, I feel like this is likely in a mergeable state already.

I agree, It makes sense. Pointing at the conditions that guard initialization would likely make the diagnostic more actionable. We should work on it as a follow up.

[Unique-Usman]

pinging: @estebank

[tiif]

Thanks for your contribution! I took a close look at this PR, and it seems correct to me. But unfortunately I am not familiar enough with this part of the compiler to merge it, so

r? @estebank

since you took a look at it.

[tiif]

Also, since this is an extra analysis, it might have perf impact. I will run perf just in case.

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 8a523a6 (8a523a604b2ca1b50e5ba16e03a8d762207cbad2, parent: b5d46ecb51c3e4134b82570cfe718f093daa6390)

[rust-timer]

Finished benchmarking commit (8a523a6): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (primary -5.6%, secondary -1.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-5.6%	[-5.6%, -5.6%]	1
Improvements ✅ (secondary)	-1.5%	[-2.2%, -0.7%]	2
All ❌✅ (primary)	-5.6%	[-5.6%, -5.6%]	1

Cycles

This perf run didn't have relevant results for this metric.

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 522.814s -> 519.939s (-0.55%)
Artifact size: 400.99 MiB -> 401.44 MiB (0.11%)

[estebank]

@bors r+

Thank you for the contribution!

[rust-bors]

📌 Commit 417015d has been approved by estebank

It is now in the queue for this repository.