Skip to content

Conversation

@Veykril
Copy link
Member

@Veykril Veykril commented Jan 7, 2024

No description provided.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 7, 2024
Comment on lines 1018 to +1020
pub enum StatementKind {
TraitEnvBlockEnter(hir_def::BlockId),
TraitEnvBlockExit,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not too happy about having to add extra statements for this, but I can't think of a better way to encode this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to be making comments from the sidelines since I don't have the whole context, but it's kind of surprising to me that there's any trait solving / method resolution happening during MIR eval at all 🤔

Copy link
Member Author

@Veykril Veykril Jan 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is surprising, but the reason for that is that we currently try to resolve the concrete method of a trait function "late". That is inference result only resolves the trait function, but MIR later wants the function of the actual trait implementation which happens in mir lowering/eval currrently (and in the IDE layer for goto def etc). I guess we should be able to move that logic into type check (and get rid of the lookup_impl_method_query).

Also comments like this are very much appreciated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do it during MIR lowering?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can get rid of the lookup_impl_method_query in MIR eval completely without changing the evaulation of dyn Trait which currently only stores the type id in the metadata part, and resolve the impl method on demand. Though I think we can switch to a more direct vtable representation, and then do all the method resolutions in the monomorphization.

Does the current implementation handle block local trait impls for dyn Trait? For example:

trait Foo {
    fn foo(&self);
}

fn some_dyn_foo() -> Box<dyn Foo> {
    struct St;
    impl Foo for St { fn foo(&self) { println!("foo"); } }
    Box::new(St)
}

fn main() {
    let x = some_dyn_foo();
    x.foo();
}

If not, is it possible to handle it in future under current architecture?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though I think we can switch to a more direct vtable representation, and then do all the method resolutions in the monomorphization.

Yes, IMO that's what we should do.

@Veykril Veykril changed the title fix: Fix mir evaluator not respecting block local items fix: Fix mir evaluator not respecting block local trait impls Jan 7, 2024
@Veykril Veykril requested a review from HKalbasi January 7, 2024 10:39
@Veykril Veykril marked this pull request as draft January 8, 2024 12:15
@bors
Copy link
Contributor

bors commented Jan 9, 2024

☔ The latest upstream changes (presumably #16329) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants