Migrate more things in the new solver to specific DefIds
#146111
Conversation
rustbot
added
S-waiting-on-review
| self.found_non_local_ty(ty) | ||
| } | ||
| } | ||
| ty::Coroutine(did, ..) => { |
There was a problem hiding this comment.
This is the only place in the PR I think there is a (slight) regression in code quality. The reason is that now the types of the variables are different. It's possible to avoid if we unify ClosureId, CoroutineId and CoroutineClosureId. This will be slightly worse for r-a and slightly better here. Given that the code duplicated is very small, and for r-a the effects of unifying them will be bigger, I decided to duplicate the code here, but I agree to reverse my decision if the reviewer disagrees.
There was a problem hiding this comment.
Could you do
(ty::Closure(did, ..) if let did = did.into()) |
...
I don't remember if this got implemented - being able to write guards for each pattern. (Even if it is, maybe you need a turbofish here, idk).
You could also remove the duplication by moving this into a closure and then calling check(did.into()) for each arm
There was a problem hiding this comment.
Doesn't seem to work: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2024&gist=33963a324a2ffe51340b0ca533c710ef.
I can extract to a closure if wanted, I just didn't because the duplicated code is really small.
There was a problem hiding this comment.
Yeah, so guard_patterns seems implemented? But doesn't support if let. Not sure if that's planned or not - sure seems possibly useful.
In this case, duplicating is probably fine, but again would defer to lcnr.
|
Looking over this, seems fine to me modulo the one bit you already pointed out, if we're committing to this in principle (which I am not opposed to). That being said, I would rather just punt this one over to lcnr |
| @@ -152,20 +152,20 @@ where | |||
| } | |||
|
|
|||
| ecx.probe_trait_candidate(CandidateSource::Impl(impl_def_id)).enter(|ecx| { | |||
| let impl_args = ecx.fresh_args_for_item(impl_def_id); | |||
| let impl_args = ecx.fresh_args_for_item(impl_def_id.into()); | |||
There was a problem hiding this comment.
Do you have a specific reason to want that? This will make the method generic, meaning monomorphization costs, and the into() is a very small cost to pay.
There was a problem hiding this comment.
using .into is kinda ugly 😁 that's my only reason
| ecx.record_impl_args(impl_args); | ||
| let impl_trait_ref = impl_trait_ref.instantiate(cx, impl_args); | ||
|
|
||
| ecx.eq(goal.param_env, goal.predicate.trait_ref, impl_trait_ref)?; | ||
| let where_clause_bounds = cx | ||
| .predicates_of(impl_def_id) | ||
| .predicates_of(impl_def_id.into()) |
| .iter_instantiated(cx, impl_args) | ||
| .map(|pred| goal.with(cx, pred)); | ||
| ecx.add_goals(GoalSource::ImplWhereBound, where_clause_bounds); | ||
|
|
||
| // For this impl to be `const`, we need to check its `[const]` bounds too. | ||
| let const_conditions = cx | ||
| .const_conditions(impl_def_id) | ||
| .const_conditions(impl_def_id.into()) |
|
Not sure I'm the right person to comment, but this is definitely a desirable change from rust-analyzer side 😅 |
|
@ShoyuVanilla I tagged you not for this PR, but to hear ideas what we can do about |
|
@bors r+ rollup |
bors
added
S-waiting-on-bors
Migrate more things in the new solver to specific `DefId`s Continuation of #145377. I migrated the rest of the types, except aliases. Aliases are problematic because opaques and associated types share the same type in the new solver. `@jackh726,` `@lcnr,` `@ShoyuVanilla` I'd like to hear ideas here. Anyway, even if we do nothing with them we already got a substantial improvement. r? types
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
A job failed! Check out the build log: (web) (plain enhanced) (plain) Click to see the possible cause of the failure (guessed by this bot) |
|
timeout @bors retry |
bors
added
S-waiting-on-bors
Rollup of 7 pull requests Successful merges: - #146111 (Migrate more things in the new solver to specific `DefId`s) - #146298 (GVN: Ensure indirect is first projection in try_as_place.) - #146299 (docs(std): add error docs for path canonicalize) - #146310 (Allow static regions in `type_name`.) - #146313 (Some `rustc_middle` cleanups) - #146319 (Fix typo in default.rs) - #146320 (rustc-dev-guide subtree update) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #146111 - ChayimFriedman2:more-ns-specific-defid, r=lcnr Migrate more things in the new solver to specific `DefId`s Continuation of #145377. I migrated the rest of the types, except aliases. Aliases are problematic because opaques and associated types share the same type in the new solver. `@jackh726,` `@lcnr,` `@ShoyuVanilla` I'd like to hear ideas here. Anyway, even if we do nothing with them we already got a substantial improvement. r? types
Continuation of #145377. I migrated the rest of the types, except aliases.
Aliases are problematic because opaques and associated types share the same type in the new solver. @jackh726, @lcnr, @ShoyuVanilla I'd like to hear ideas here. Anyway, even if we do nothing with them we already got a substantial improvement.
r? types