-
Couldn't load subscription status.
- Fork 13.9k
Fix MemCategorization and ExprUse visitors for new solver
#124859
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
Conversation
|
The job Click to see the possible cause of the failure (guessed by this bot) |
MemCategorization and ExprUse visitors for new solver
|
goddamn clippy |
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.
ignoring clippy CI issues, this looks good 👍 some further suggestions, then r=me
| self.tcx() | ||
| .dcx() | ||
| .span_delayed_bug(self.tcx().hir().span(id), "encountered type variable"); | ||
| Err(()) |
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.
do we ever return Err(()) in places which should successfully compile? If not, please change McResult to Result<T, ErrorGuaranteed>
| /// - `typeck_results` --- typeck results for the code being analyzed | ||
| pub fn new( | ||
| pub(crate) fn new( | ||
| delegate: &'a mut (dyn Delegate<'tcx> + 'a), |
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.
style nit: pls flip arguments: fcx, body_id, delegate instead of teh current order
| /// The def id of the body that is being categorized. | ||
| /// This is **different** from `fcx.body_id`. | ||
| pub inner_body_id: LocalDefId, |
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.
This feels odd, shouldn't we create a separate FnCtxt for nested bodies? I guess is issue is that the scoping is as follows?
typeck_outer {
typeck_inner {}
closure_capture_analysis_outer {}
closure_capture_analysis_inner {}
}
i.e. we already dropped the inner FnCtxt again? 🤔
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.
The old FnCtxt is long gone, though we could create a new one (we don't care about any of the fields in FnCtxt except for the body id).
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.
All of this is useless though, because we definitely cannot adapt this with the way that clippy is using ExprUseVisitor, so I'll have to completely rework this PR.
| Ok(ty) | ||
| } | ||
| } | ||
| // FIXME |
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.
existing: what's that FIXME?
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.
Probably due to not wanting to rely on InferCtxt taintedness for expr-use-analysis.
|
I'm going to put up a more involved PR that actually supports clippy's usecases |
…r=<try> Fix MemCategorization and ExprUse visitors for new solver (this time it's better) Best reviewed by each commit. Supersedes rust-lang#124859. r? lcnr
…r=<try> Fix MemCategorization and ExprUse visitors for new solver (this time it's better) Best reviewed by each commit. Supersedes rust-lang#124859. r? lcnr
…r=lcnr Fix MemCategorization and ExprUse visitors for new solver (this time it's better) Best reviewed by each commit. Supersedes rust-lang#124859. r? lcnr
Best reviewed per-commit.
MemCategorization.FnCtxtinto these visitors. Shouldn't change any behavior.Ty::kindandTy::builtin_deref.This fixes an ICE in icu4x when building it in the new trait solver.
r? lcnr