On partial uninit error point at where we need init #98360

estebank · 2022-06-21T22:57:20Z

When a binding is declared without a value, borrowck verifies that all
codepaths have one assignment to them to initialize them fully. If
there are any cases where a condition can be met that leaves the binding
uninitialized or we attempt to initialize a field of an uninitialized
binding, we emit E0381.

We now look at all the statements that initialize the binding, and use
them to explore branching code paths that don't and point at them. If
we find no potential places where an assignment to the binding might
be missing, we display the spans of all the existing initializers to
provide some context.

Fix #97956.

rust-highfive · 2022-06-21T22:57:24Z

r? @oli-obk

(rust-highfive has picked a reviewer for you, use r? to override)

estebank · 2022-06-21T23:12:44Z

src/test/ui/rfc-2497-if-let-chains/chains-without-let.stderr

+error[E0381]: used binding `z` isn't initialized in all conditions
  --> $DIR/chains-without-let.rs:15:36
   |
+LL |     let z;
+   |         - binding declared here but left uninitialized
 LL |     if false || { z = 3; false} || z == 3 {}
-   |                                    ^ use of possibly-uninitialized `z`
+   |                   -----            ^ `z` used here but it isn't initialized in all conditions
+   |                   |
+   |                   binding initialized here in some conditions


@oli-obk is this intended or just an unfortunate side-effect of the current machinery? It feels to me that for or binops this code should succeed (while it might not for and binops if the use is outside of the binop chain.)

riking

You should add that big test file you demonstrated as a new UI test.

src/test/ui/nll/match-cfg-fake-edges.stderr

oli-obk · 2022-06-22T09:17:10Z

src/test/ui/async-await/no-non-guaranteed-initialization.stderr

@@ -1,8 +1,13 @@
-error[E0381]: use of possibly-uninitialized variable: `y`
+error[E0381]: used binding `y` isn't initialized in all conditions


Not too happy with the in all conditions phrase.

Would be ok I think if the label were kept as ^ use of possibly-uninitialized and the label on the if condition would say

if this condition is false, y is not initialized.

The message about missing an else arm is a proposal to fix, not really an explanation of what's wrong. Cluttering more by adding a further label at the } of the if seems not great tho.... idk, what's your opinion on these ramblings?

What about "binding y is used when possibly uninitialized" for the message?

I like your proposal for the if condition label, it's more straight forward to understand.

Regarding the "missing else" label, I can reword it to be similar to the other if/else labels, pointing at the } feels less great when rendered out, but I can try it. Pointing at the whole if could get verbose quickly, but I'm not too worried about that.

oli-obk · 2022-06-22T09:19:56Z

src/test/ui/borrowck/borrowck-if-with-else.stderr

+LL |     let x: isize;
+   |         - binding declared here but left uninitialized
+LL |     if 1 > 2 {
+   |        ----- `x` is uninitialized if this condition is met


yea, I would expect this message (or its inverse) no matter whether there's an else arm or not.

Why? Because the condition's value can be statically determined?

I think what you're saying is that in the case where

if y { x = 10; } else { println!("whoops"); }

The same message can be shown as for

if y { x = 10; }

But that is not the case because for the former we point at the else, which we don't have, that's why that case is special cased.

oli-obk · 2022-06-22T09:20:46Z

src/test/ui/borrowck/borrowck-init-op-equal.stderr

 LL |     v += 1;
-   |     ^^^^^^ use of possibly-uninitialized `v`
+   |     ^^^^^^ `v` used here but it isn't initialized


we should probably improve the span here at some point

compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs

oli-obk · 2022-06-22T09:45:49Z

compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs

@@ -326,6 +312,100 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
        }
    }

+    fn report_use_of_uninitialized(


I think this function and its dependencies could live in another module so they're grouped together

We're already in a diagnostics module, right? I can add a new mod if you want with the whole thing, sure.

oli-obk · 2022-06-22T09:56:14Z

compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs

+        let body = map.body(body_id);
+
+        let mut visitor = ConditionVisitor { spans: &spans, name: &name, errors: vec![] };
+        visitor.visit_body(&body);


I wonder if we could generalize the logic by walking the MIR instead. In MIR you can write one dataflow pass that finds all the branches that do not initialize the variable, irrespective of whether the branch came from an if, a match or any other control flow.

This also means you would not have to rely on spans, but can use the MIR location directly (potentially helping with desugarings and macros). And since you can map back to a hir id from a location or source info (via https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/struct.SourceScopeData.html#structfield.local_data), you can still generate high quality diagnostics by checking what kind of hir item some MIR thing belongs to.

I'm not sure if that effort is worth it, and we could do it as a separate PR after this one is merged, but it feels a bit cleaner than span fudging.

I agree with this, I was just held back by my lack of MIR knowledge to change the logic to accomplish this. I would like to leave that as a follow up for someone else though :)

src/test/ui/nll/issue-21232-partial-init-and-erroneous-use.stderr

oli-obk · 2022-06-23T06:55:20Z

@bors r+

bors · 2022-06-23T06:55:22Z

📌 Commit 8495c64 has been approved by oli-obk

bors · 2022-06-23T06:55:23Z

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

estebank · 2022-06-23T17:42:53Z

@bors r=oli-obk

bors · 2022-06-23T17:42:55Z

📌 Commit 6dd32f2 has been approved by oli-obk

bors · 2022-06-23T17:42:56Z

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

On partial uninit error point at where we need init When a binding is declared without a value, borrowck verifies that all codepaths have *one* assignment to them to initialize them fully. If there are any cases where a condition can be met that leaves the binding uninitialized or we attempt to initialize a field of an uninitialized binding, we emit E0381. We now look at all the statements that initialize the binding, and use them to explore branching code paths that *don't* and point at them. If we find *no* potential places where an assignment to the binding might be missing, we display the spans of all the existing initializers to provide some context. Fix rust-lang#97956.

JohnTitor · 2022-06-26T04:12:05Z

Failed in rollup: #98505 (comment)
Some tests also need a bless.
@bors r-

When a binding is declared without a value, borrowck verifies that all codepaths have *one* assignment to them to initialize them fully. If there are any cases where a condition can be met that leaves the binding uninitialized or we attempt to initialize a field of an unitialized binding, we emit E0381. We now look at all the statements that initialize the binding, and use them to explore branching code paths that *don't* and point at them. If we find *no* potential places where an assignment to the binding might be missing, we display the spans of all the existing initializers to provide some context.

estebank · 2022-07-07T20:17:02Z

@bors r=oli-obk

bors · 2022-07-07T20:17:04Z

📌 Commit d6c59cf5bf4b59483b892ec830e6a49e7155e73b has been approved by oli-obk

It is now in the queue for this repository.

estebank · 2022-07-07T20:20:54Z

@bors r-

estebank · 2022-07-07T20:21:01Z

@bors r=oli-obk

bors · 2022-07-07T20:21:02Z

📌 Commit 2a2df9d has been approved by oli-obk

It is now in the queue for this repository.

bors · 2022-07-07T23:36:24Z

⌛ Testing commit 2a2df9d with merge 9b21131...

bors · 2022-07-08T02:17:03Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 9b21131 to master...

rust-timer · 2022-07-08T09:34:13Z

Finished benchmarking commit (9b21131): comparison url.

Instruction count

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

Max RSS (memory usage)

Results

Primary benchmarks: 😿 relevant regression found
Secondary benchmarks: 😿 relevant regression found

	mean¹	max	count²
Regressions 😿 (primary)	0.1%	0.1%	1
Regressions 😿 (secondary)	0.9%	0.9%	1
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	0.1%	0.1%	1

Cycles

Results

Primary benchmarks: no relevant changes found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	4.7%	4.7%	1
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-8.7%	-8.7%	1
All 😿🎉 (primary)	N/A	N/A	0

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

the arithmetic mean of the percent change ↩ ↩²
number of relevant changes ↩ ↩²

rust-highfive assigned oli-obk Jun 21, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 21, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 21, 2022

estebank commented Jun 21, 2022

View reviewed changes

estebank mentioned this pull request Jun 21, 2022

Possibly-uninitialized error message doesn't indicate where value isn't initialized #97956

Closed

riking reviewed Jun 22, 2022

View reviewed changes

src/test/ui/nll/match-cfg-fake-edges.stderr Outdated Show resolved Hide resolved

oli-obk reviewed Jun 22, 2022

View reviewed changes

oli-obk requested changes Jun 22, 2022

View reviewed changes

estebank commented Jun 22, 2022

View reviewed changes

src/test/ui/nll/issue-21232-partial-init-and-erroneous-use.stderr Outdated Show resolved Hide resolved

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 Jun 23, 2022

matthiaskrgr mentioned this pull request Jun 25, 2022

Rollup of 8 pull requests #98494

Closed

matthiaskrgr mentioned this pull request Jun 25, 2022

Rollup of 8 pull requests #98505

Closed

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 Jun 26, 2022

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 Jul 7, 2022

This comment has been minimized.

Sign in to view

estebank added 7 commits July 7, 2022 12:25

Tweak wording and spans

9cb1874

Review comments: wording

95923d1

Add test for for loop maybe initializing binding

8ab1cd9

Avoid misleading message/label in match-cfg-fake-edges.rs test

a86fa4f

Fix label on uninit binding field assignment

5f91614

Wording tweak

d09851c

estebank force-pushed the uninit-binding branch from 6b6f52b to d6c59cf Compare July 7, 2022 20:16

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 7, 2022

fix arm test

2a2df9d

estebank force-pushed the uninit-binding branch from d6c59cf to 2a2df9d Compare July 7, 2022 20:20

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 Jul 7, 2022

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 Jul 7, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 8, 2022

bors merged commit 9b21131 into rust-lang:master Jul 8, 2022

rustbot added this to the 1.64.0 milestone Jul 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

On partial uninit error point at where we need init #98360

On partial uninit error point at where we need init #98360

estebank commented Jun 21, 2022

rust-highfive commented Jun 21, 2022

estebank Jun 21, 2022

riking left a comment

oli-obk Jun 22, 2022

estebank Jun 22, 2022

oli-obk Jun 22, 2022

estebank Jun 22, 2022

oli-obk Jun 22, 2022

oli-obk Jun 22, 2022

estebank Jun 22, 2022

oli-obk Jun 22, 2022

estebank Jun 22, 2022

oli-obk commented Jun 23, 2022

bors commented Jun 23, 2022

bors commented Jun 23, 2022

estebank commented Jun 23, 2022

bors commented Jun 23, 2022

bors commented Jun 23, 2022

JohnTitor commented Jun 26, 2022

This comment has been minimized.

estebank commented Jul 7, 2022

bors commented Jul 7, 2022

estebank commented Jul 7, 2022

estebank commented Jul 7, 2022

bors commented Jul 7, 2022

bors commented Jul 7, 2022

bors commented Jul 8, 2022

rust-timer commented Jul 8, 2022

		@@ -1,8 +1,13 @@
		error[E0381]: use of possibly-uninitialized variable: `y`
		error[E0381]: used binding `y` isn't initialized in all conditions

On partial uninit error point at where we need init #98360

On partial uninit error point at where we need init #98360

Conversation

estebank commented Jun 21, 2022

rust-highfive commented Jun 21, 2022

Choose a reason for hiding this comment

riking left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Jun 23, 2022

bors commented Jun 23, 2022

bors commented Jun 23, 2022

estebank commented Jun 23, 2022

bors commented Jun 23, 2022

bors commented Jun 23, 2022

JohnTitor commented Jun 26, 2022

This comment has been minimized.

estebank commented Jul 7, 2022

bors commented Jul 7, 2022

estebank commented Jul 7, 2022

estebank commented Jul 7, 2022

bors commented Jul 7, 2022

bors commented Jul 7, 2022

bors commented Jul 8, 2022

rust-timer commented Jul 8, 2022

Footnotes