fix-155516: Don't suggest wrong unwrap expect

[lokirithm]

View all comments

Fixes #155516

r? Kivooeo

[Kivooeo]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 8977e63 (8977e632bc55f12c46173d6e4acccc5bcad7378b, parent: b954122bbe6c4a244c511a9a168c6d5438d8f58f)

[rust-timer]

Finished benchmarking commit (8977e63): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.0%	[0.0%, 0.0%]	1
Improvements ✅ (primary)	-0.7%	[-0.7%, -0.7%]	1
Improvements ✅ (secondary)	-0.3%	[-0.4%, -0.1%]	2
All ❌✅ (primary)	-0.7%	[-0.7%, -0.7%]	1

Max RSS (memory usage)

Results (secondary -0.9%)

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)	2.6%	[2.5%, 2.8%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.7%	[-4.2%, -2.1%]	4
All ❌✅ (primary)	-	-	0

Cycles

Results (secondary 2.2%)

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)	3.8%	[2.2%, 5.9%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-6.9%	[-6.9%, -6.9%]	1
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 514.377s -> 509.852s (-0.88%)
Artifact size: 400.61 MiB -> 402.59 MiB (0.50%)

[lokirithm]

What does it say? The code is not optimal? I mean, is it taking more time or less time to compile/run programs?

[Kivooeo]

for some reason perf is good 🤷‍♂️

because of early return likely

[Kivooeo]

this needs another pair of eyes honestly, i'm not 100% confident about it but can't say why

@rustbot reroll

[lokirithm]

because of early return likely

But, afaik, there was only a single test in the whole test suite that was using the early return. also, the number of tests is increased by 1. So, intuitively the time taken should have increased, because changes aren't affecting the other tests (or at least increasing the time take because of the for loop).

[teor2345]

Looks ok to me, but are there any other reasons to terminate the upward search for assignments?

For example, maybe we want to terminate at function/closure boundaries, module boundaries, or similar.

because of early return likely

But, afaik, there was only a single test in the whole test suite that was using the early return. also, the number of tests is increased by 1. So, intuitively the time taken should have increased, because changes aren't affecting the other tests (or at least increasing the time take because of the for loop).

The benchmarks measure the speed of specific crates and custom benchmark builds. Most tests aren't benchmarked.

In this case the most significant change is nalgebra, which seems reasonable.

View changes since this review

[teor2345]

Looks like a spelling typo?
(There's another instance of this typo below.)

Suggested change

	// desugering a destructuring assignment
	// desugaring a destructuring assignment

[lokirithm]

are there any other reasons to terminate the upward search for assignments?

No, i guess. At least, I can't think of any other terminating condition.

[teor2345]

are there any other reasons to terminate the upward search for assignments?

No, i guess. At least, I can't think of any other terminating condition.

Sorry, my comment wasn't clear.

The for loop that searches for parent items will go all the way up to the crate root. This is a waste of compiler time.

It should terminate at the first of:

• the enclosing function: the first function, method, or closure definition item encountered in the search
• the enclosing constant (for code running in a const context)
• if there's any other items that can contain code, the enclosing item
• the enclosing module (which might not ever happen, because there should be another item first)

[lokirithm]

• the enclosing function: the first function, method, or closure definition item encountered in the search

I guess, we don't require to go up to the enclosing function.

So, I think, the code is doing what you are saying, but a little bit earlier.

Instead of going up to the function, it stops at the first assignment expression which was not generated by the compiler to desugar the destructuring assignment syntax (and it checks this by checking whether the LHS and RHS of the expression have overlapping spans; it's impossible to write actual code or expression whose spans overlap).

Assuming a general case, an assignment expression will come before the enclosing function when we are going up the HIR tree. I mean, It's hard to imagine a function which no assignment expressions, unless very simple functions, for example sum(a, b) { a + b }. And I am unsure, if such a function can also have an assignment expression generated by the compiler during HIR creation. Even if there are no assignment expressions in the HIR of such simple functions, it's highly likely that these the result of such functions will be immediately assigned to some variable.

Actually, I myself have some doubts, but as per my understanding [I used AI to understand the concept, but not to write code], I feel it's correct.

[cjgillot]

The terminating condition for the upwards search is 1. By definition, rhs.hir_id == expr.hir_id means that e is the parent of expr. No need to go higher. The loop can be simplified to a single call to tcx.parent_hir_node(expr.hir_id).

[cjgillot]

For a generic named struct, do we still suggest anything? Should we?

struct NamedStruct<O, R> {
    opt_field: O,
    res_field: R,
}

fn main() {
    let a: usize;
    let b: usize;

    NamedStruct {
        opt_field: a,
        res_field: b,
    } = NamedStruct {
        opt_field: Some(0),
        res_field: Ok(42_usize),
    };
}

[lokirithm]

do we still suggest anything?

Yes. The compiler suggests the same.

Should we?

As per my understanding, suggesting .expect() in LHS in a named struct would not make sense in any scenario.

[cjgillot]

@bors r+ rollup

[rust-bors]

📌 Commit 9633071 has been approved by cjgillot

It is now in the queue for this repository.