- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1.9k
fix: Properly support opaques #20906
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
d7912ad    to
    4e6c5c7      
    Compare
  
    By letting the solver take control of them (reveal them when needed and define them when needed), by providing them in the `TypingMode` plus few helpers.
4e6c5c7    to
    537b31b      
    Compare
  
    | Fixed the panic, now this can be reviewed. | 
| pub(crate) struct InferenceTable<'db> { | ||
| pub(crate) db: &'db dyn HirDatabase, | ||
| pub(crate) trait_env: Arc<TraitEnvironment<'db>>, | ||
| pub(crate) tait_coercion_table: Option<FxHashMap<InternedOpaqueTyId, Ty<'db>>>, | 
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.
I'm really happy that my opaque hack here can be finally removed 🎉
| ); | ||
| } | ||
|  | ||
| #[ignore = "FIXME(next-solver): TAIT support was removed, need to rework it to work with `#[define_opaque]`"] | 
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.
👍
| impl<'db> InferenceTable<'db> { | ||
| pub(crate) fn new(db: &'db dyn HirDatabase, trait_env: Arc<TraitEnvironment<'db>>) -> Self { | ||
| /// Inside hir-ty you should use this for inference only, and always pass `owner`. | ||
| /// Outside it, always pass `owner = None`. | 
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.
Expressing this with a new enum type would be great, though it is stated well in the comment and I think this is okay as it is
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.
I hope this is temporary, since I plan to remove all usages of InferenceTable except inference (its role was to do what InferCtxt does now).
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.
Yeah, InferenceTable feels quite awkward now 😅
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.
👍
| I'll add this to the merge queue, as the implementation feels right and is relatively less complex compared to other big next-solver changes 😄 | 
By letting the solver take control of them (reveal them when needed and define them when needed), by providing them in the
TypingModeplus few helpers.