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

Add TyCtxt wrappers to sort its methods into groups and improve documentation #603

Closed
1 of 3 tasks
WaffleLapkin opened this issue Mar 17, 2023 · 2 comments
Closed
1 of 3 tasks
Labels
major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@WaffleLapkin
Copy link
Member

Proposal

Current TyCtxt documentation is quite hard to navigate, particularly because of the sheer amount of methods. The same goes for autocomplete, etc.

I propose to add transparent wrapper(s) around TyCtxt in order to improve this situation. Then, instead of .some_method() you'd write .group().some_method(). As an example, See rust-lang/rust#109186, which adds a wrapper for "make" methods, such that you write .mk().slice(t) instead of .mk_slice(t):

#[derive(Copy, Clone)]
pub struct MkCtxt<'tcx> {
    tcx: TyCtxt<'tcx>,
}

impl<'tcx> TyCtxt<'tcx> {
    /// ...
    pub fn mk(self) -> MkCtxt<'tcx> { MkCtxt { tcx: self } }
}

impl<'tcx> MkCtxt<'tcx> {
    pub fn slice(self, ty: Ty<'tcx>) -> Ty<'tcx> {
        self.ty_from_kind(Slice(ty))
    }

    // many more methods here
}

Pros:

  • Better documentation
  • Such grouping should hopefully make discovering rustc internals for begginner contributors a tad easier

Cons:

  • The code becomes technically longer, because of the .group()
  • @nnethercote mentions in Add a context for making things rust#109186 (comment) that "mk_foo is a verb phrase with an obvious meaning, which is good for function names. mk().foo() is... not that. mk should really be called mcx to follow the existing convention for things like tcx and icx, but mcx().foo() doesn't work."
    • It's not clear if/how this would apply to other wrappers, as we haven't yet discussed/thought about them
    • For me personally it does make sense, you say "make" and then "what to make"... but maybe that's just my warped perception of things
  • Potentially this adds more code for LLVM/rustc to chew through
  • This solution can steer as away from the actual solution of properly scoped methods/slaying the god object (it's unclear if we'll ever be able/want to do that)

Mentors or Reviewers

@oli-obk

Process

The main points of the Major Change Process are as follows:

  • File an issue describing the proposal.
  • A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
    • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
    • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
  • Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

@WaffleLapkin WaffleLapkin added T-compiler Add this label so rfcbot knows to poll the compiler team major-change A proposal to make a major change to rustc labels Mar 17, 2023
@rustbot
Copy link
Collaborator

rustbot commented Mar 17, 2023

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

cc @rust-lang/compiler @rust-lang/compiler-contributors

@rustbot rustbot added the to-announce Announce this issue on triage meeting label Mar 17, 2023
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Mar 23, 2023
@pnkfelix
Copy link
Member

As noted in zulip discussion, I do not think this proposal is going to be seconded as it is currently proposed.

There are other alternative proposals in the zulip thread, especially one by @lcnr that seemed more likely to have positive reception from the team. I encourage people to try to make a new MCP based on the ideas there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Development

No branches or pull requests

4 participants