Check tainted_by_error in LateLint

[Shunpoco]

View all comments

Context

This PR continues from #138679 (comment).
In the last PR, I introduced typeck result's tainted_by_error in check_pat. But as we've discussed, I should put the check to a better place which all lints get benefit from the check.

Change

Since visit_nested_body in late.rs is the starting point of late lint for a nested body, I moved the error check to the function.
I also rename one ui test case which I introduced in the last PR. I think the new name describes what the test wants to check more.

This PR fixes #138361 .

Note that we need to use actually_rustdoc to call typeck_body() in visit_nested_body. Otherwise rustdoc returns an error. However, as its comment describes we shouldn't use actually_rustdoc if there is an alternative solution. So far I only come up with using actually_rustdoc (this change), or checking tainted_by_error in each check_xxx functions (e.g., check on check_pat in #138679, and on check_expr for #138361).

[Shunpoco]

r? @oli-obk

[rustbot]

oli-obk is not on the review rotation at the moment.
They may take a while to respond.

[rustbot]

This PR changes a file inside tests/crashes. If a crash was fixed, please move into the corresponding ui subdir and add 'Fixes #' to the PR description to autoclose the issue upon merge.

[oli-obk]

@bors try @rust-timer queue

I think this may cause major perf issues. Maybe we need to integrate it with the typeck_results method. Let's check

[rust-bors]

☀️ Try build successful (CI)
Build commit: b571264 (b5712648d4b50898a2b3edfb8a3de2940f029877, parent: d85276b256a8ab18e03b6394b4f7a7b246176db7)

[rust-timer]

Finished benchmarking commit (b571264): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@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.2%	[0.2%, 0.3%]	3
Improvements ✅ (primary)	-2.9%	[-2.9%, -2.9%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.9%	[-2.9%, -2.9%]	1

Max RSS (memory usage)

Results (primary 2.4%, secondary -2.7%)

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

	mean	range	count
Regressions ❌ (primary)	4.1%	[0.7%, 7.5%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.8%	[-0.8%, -0.8%]	1
Improvements ✅ (secondary)	-2.7%	[-2.7%, -2.7%]	1
All ❌✅ (primary)	2.4%	[-0.8%, 7.5%]	3

Cycles

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

Binary size

Results (primary -1.1%)

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)	-1.1%	[-1.1%, -1.1%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.1%	[-1.1%, -1.1%]	1

Bootstrap: 476.797s -> 473.568s (-0.68%)
Artifact size: 390.82 MiB -> 390.85 MiB (0.01%)

[oli-obk]

Considering this can be called unconditionally outside rustdoc, perform it further down where we do self.context.cached_typeck_results.set(None); and just always set it to Some unless in rustdoc mode or if tainted (keep returning in the tainted case of course).

The cache still needs to be an option as we also have lints outside of bodies

[Shunpoco]

Thanks,

Considering this can be called unconditionally outside rustdoc, perform it further down where we do self.context.cached_typeck_results.set(None);

Do you mean I should call typeck_body in the block which cached_typeck_results.set(None) is called or I should call typeck_body other place where rustdoc doesn't use?

[oli-obk]

you can call it there, but still behind an !actually_rustdoc check, and then just overwrite the freshly set None with a Some of the data you just loaded.

At that point I think we can even change the method for obtaining the cached typeck results to just unwrap the option

[Shunpoco]

Thanks, let me take time to investigate. I think I need to understand how typeck_body works more.

we can even change the method for obtaining~

Let me clarify, does "the method" point typeck_body?

[oli-obk]

Let me clarify, does "the method" point typeck_body?

No I meant maybe_typeck_results, which currently has an or_else, but could now just remove the or_else and trust that it was already filled in

[apiraino]

Switching to waiting on author to incorporate changes, I think this review is still standing.

Feel free to request a review again with @rustbot ready, thanks!

@rustbot author

[Dylan-DPC]

@Shunpoco any updates on this? thanks

[oli-obk]

@bors r- right squashing

[rust-bors]

This pull request was unapproved.

View changes since this unapproval

[Shunpoco]

@oli-obk Thanks for the review!

May I squash the commits myself since it looks rust-bors failed to squash because of my multi line commit message?

[oli-obk]

Yes please!

[Shunpoco]

Squash was done, and all CIs are green now 😄

[oli-obk]

@bors r+

Thanks!

[rust-bors]

📌 Commit db6e110 has been approved by oli-obk

It is now in the queue for this repository.