Skip to content
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

Cleanup and consolidate operator/method dispatch code #18741

Open
nikomatsakis opened this issue Nov 7, 2014 · 18 comments
Open

Cleanup and consolidate operator/method dispatch code #18741

nikomatsakis opened this issue Nov 7, 2014 · 18 comments
Labels
A-type-system Area: Type system C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

Once upon a time, both operator dispatch and method lookup went through the same tangled, twisty paths. It was a mess. But lo, they were severed and made two. Each could follow its own path. Overall, this is progress. But there is still room for improvement:

  1. The second half of "operator dispatch" is basically the same as confirm::confirm, but with some slight differences.
  2. The autoderef loop for [] is basically the same as probe, but with some slight differences (e.g., at each step it consists builtin [] as well).
  3. The probe loop, which uses check::autoderef, isn't able to be part of an inference transaction between check::autoderef uses operator dispatch which adds things into the main fulfillment context, thus leaking inference types etc outside the transaction.

It feels like things could still be cleaned up a bit further, allowing for more code reuse and happiness all around.

Some FIXMEs are scattered about at relevant points of the code.

@nikomatsakis nikomatsakis added E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. A-type-system Area: Type system C-cleanup Category: PRs that clean code up or issues documenting cleanup. labels Nov 7, 2014
@nikomatsakis
Copy link
Contributor Author

I am happy to mentor.

@steveklabnik
Copy link
Member

@nikomatsakis is this ticket still valid?

@nikomatsakis
Copy link
Contributor Author

On Thu, Dec 31, 2015 at 10:26:12AM -0800, Steve Klabnik wrote:

@nikomatsakis is this ticket still valid?

yes

@Phyllostachys
Copy link

Inspired by this and having been a long time fan of Rust and wanting to contribute, I'm interested in solving this issue. With that said, is there anything I need to know to get started? Also, where can I start looking?

@nikomatsakis
Copy link
Contributor Author

@Phyllostachys to be honest it's a bit out of cache for me. I'd have to go read into the code some to see where things stand. Anyway, usually a good place to start is to try and touch base at some point on IRC or over e-mail.

@Phyllostachys
Copy link

I'll have to jump on IRC tonight. Who would I email if I emailed someone?

@nikomatsakis
Copy link
Contributor Author

@Phyllostachys me, my e-mail is on my github page

@ahmedcharles
Copy link
Contributor

@nikomatsakis Are you still willing to mentor this?

@Mark-Simulacrum
Copy link
Member

@ahmedcharles Are you still interested? We can try to connect you with niko if so

@nikomatsakis
Copy link
Contributor Author

Well, it's quite out of cache, but let me start by giving a few very basic pointers. The relevant code is now found at src/librustc_typeck/check/method/mod.rs -- the fns in question are lookup_method() and lookup_method_in_trait_adjusted(). You may find the README.md in that same directory useful.

@citizen428
Copy link
Contributor

@Mark-Simulacrum @nikomatsakis It's been a while since anyone showed any interest in this. If it's not picked up soon, I'll give it a shot, though I'm fairly certain I'll need more mentoring than what has a been provided so far.

@Mark-Simulacrum
Copy link
Member

Let us know if you want further instructions and we'll do our best to connect you with @nikomatsakis.

@citizen428
Copy link
Contributor

Work got in the way a bit, but I'm still planning on tackling this. Started looking through the README and code a bit, will get back to you if I need more info.

@citizen428
Copy link
Contributor

Sorry for the long radio silence. Anyway, I just looked at this again and I don't think that this issue is a good fit for my current level of knowledge regarding rustc internals. Therefore I'm also not quite sure if commits like de0ffad already addressed some of the issues outlined here.

@jonas-schievink jonas-schievink added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 12, 2020
@sergey-melnychuk
Copy link
Contributor

@nikomatsakis is this still relevant? I'm not afraid to give it a try :)

Seems like FIXMEs are still there:

// FIXME(#18741) -- this is almost but not quite the same as the

// FIXME(#18741): it seems likely that we can consolidate some of this

@nikomatsakis
Copy link
Contributor Author

To be quite honest, I don't know! It's been an awfully long time. I imagine it's still relevant, though how much things can be improved I'm not sure.

@koivunej
Copy link

koivunej commented Apr 5, 2021

Came across this issue through E-Mentor, looked up the above mentioned FIXME locations now more than a year later just in case someone is interested to do a more thorough peek:

// FIXME(#18741) -- this is almost but not quite the same as the
// autoderef that normal method probing does. They could likely be
// consolidated.

// FIXME(#18741): it seems likely that we can consolidate some of this
// code with the other method-lookup code. In particular, the second half
// of this method is basically the same as confirmation.

@nikomatsakis nikomatsakis removed the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Apr 9, 2021
@nikomatsakis
Copy link
Contributor Author

I'm removing E-mentor not because I wouldn't want to mentor but because i prefer to use the tag only for issues with some decent mentoring instructions, and I don't think this quite qualifies.

@fmease fmease added A-type-system Area: Type system T-types Relevant to the types team, which will review and decide on the PR/issue. and removed A-type-system Area: Type system labels Dec 21, 2024
lnicola pushed a commit to lnicola/rust that referenced this issue Dec 23, 2024

Unverified

The signature in this commit could not be verified. Someone may be trying to trick you.
fix: Delay initial flycheck until after build scripts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-type-system Area: Type system C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

10 participants