-
Notifications
You must be signed in to change notification settings - Fork 13.9k
Check tainted_by_error in LateLint #147876
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.
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
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
4d84ca4 to
34be164
Compare
…rror We started to check typeck result's tainted_by_errors in check_pat for LateLint, But ideally the check should be in a better place which all lints profit from it. visit_nested_body is the place because it's the starting point of linting body.
34be164 to
f61c361
Compare
|
r? @oli-obk |
|
|
|
This PR changes a file inside |
|
@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 |
This comment has been minimized.
This comment has been minimized.
Check tainted_by_error in LateLint
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (b571264): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking 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 @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
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.
CyclesResults (primary -3.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -1.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 476.797s -> 473.568s (-0.68%) |
| // The typeck_result shouldn't be tainted, otherwise it will cause ICE. | ||
| // If rustdoc is the caller of this function, we shouldn't run typeck_body here. | ||
| if !self.context.tcx.sess.opts.actually_rustdoc | ||
| && self.context.tcx.typeck_body(body_id).tainted_by_errors.is_some() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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).