Move fulfillment into rustc_next_trait_solver - #160485
Conversation
rustbot
added
S-waiting-on-author
This comment has been minimized.
This comment has been minimized.
499da05 to
247c257
Compare
|
Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor |
rustbot
added
S-waiting-on-review
|
rustbot has assigned @ShoyuVanilla. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
r? lcnr |
| use super::delegate::SolverDelegate; | ||
| use crate::traits::{FulfillmentError, ScrubbedTraitError}; | ||
|
|
||
| #[path = "fulfill/derive_errors.rs"] |
There was a problem hiding this comment.
Why do we need this?
There was a problem hiding this comment.
I renamed the wrapper file. needed because the rustc-specific wrapper module was named rustc_fulfill while its file was still solve/fulfill.rs
| mod fulfill; | ||
| pub mod inspect; | ||
| mod normalize; | ||
| #[path = "solve/fulfill.rs"] |
There was a problem hiding this comment.
Not sure I quite understand this
There was a problem hiding this comment.
this was selecting file while exposing it locally as rustc_fulfill which wrapper renamed now
| } | ||
| } | ||
|
|
||
| fn into_rustc_solver_error<'tcx>( |
There was a problem hiding this comment.
Could this be a trait?
There was a problem hiding this comment.
that is cleaner , I did
| Overflow(O), | ||
| } | ||
|
|
||
| type PendingObligations<I, O> = ThinVec<(O, Option<GoalStalledOn<I>>)>; |
There was a problem hiding this comment.
Firstly I'm not overly familiar with this area of codebase, however, if I am not mistaken it looks like the type is PredicateObligation<'tcx>? Or an Obligation do we have something that O needs to implement or be constrained by? This applies to all instances of O, not exclusively this type definition.
There was a problem hiding this comment.
As shared engine only require the <I> while PredicateObligation<'tcx> not required.
I moved that bound onto FulfillmentCtxt<I, O> itself instead
There was a problem hiding this comment.
I think you cut yourself off mid-sentence?
There was a problem hiding this comment.
Sorry for the unclear wording :)
for rustc O is PredicateObligation<'tcx> but the shared engine only requires O: FulfillmentObligation<I> which moved that bound onto FulfillmentCtxt<I, O> itself now and it applies to every instance of the context.
There was a problem hiding this comment.
A few nits! Sadly I don't feel comfortable/confident to r+ this myself but figured I'd still comment a bit on this since folks asked me to~
My main thought is checking if someone from rust-analyzer has seen/glanced over this, and/or if you've attempted to migrate rust-analyzer to it to validate the approach works (even if that's not posted yet, due to how rust-analyzer consumes crates.io rather than in-tree).
One nit: Could you squash and/or generally clean the commits up a bit before merging? Not super important I think, idk.
cc83dbe to
9c18ee3
Compare
Signed-off-by: Amirhossein Akhlaghpour <m9.akhlaghpoor@gmail.com>
9c18ee3 to
9222b8b
Compare
Thanks for taking a look! Thanks again for the feedback |
Any deviation from rustc in r-a in things like that are a bug (that might have occurred from previous rustc versions). You do not need to allow both behaviors. |
There was a problem hiding this comment.
This seems fine to me r-a wise. Our current implementation is a bit different but that is due to old rustc versions we copied from and optimizations we applied that are probably no longer applicable (and if they are, we should apply them to rustc as well).
|
☔ The latest upstream changes (presumably #160160) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. |
8 participants
It moves the rustc fulfillment implementation into the shared crate
migrating rust-analyzer to the shared implementation will require the updated shared crates to be published and may need frontend specific adjustments
Part of #159654