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 a context for making things #109186

Closed
wants to merge 8 commits into from
Closed

Conversation

WaffleLapkin
Copy link
Member

@WaffleLapkin WaffleLapkin commented Mar 15, 2023

r? @oli-obk
cc @nnethercote (I've touched these files recently)

TL;DR: replace tcx.mk_something() with tcx.mk().something() in order to improve documentation (less stuff in the TyCtxt impls)

Future work:

  • Remove mk_* methods from TyCtxtAt maybe
  • Better document MkCtxt's methods
  • Move methods that are missing mk_ or have _mk_ in the middle

I'm not 100% sure this is a good idea, but generally de-bloating TyCtxt seems good.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Mar 15, 2023
@rustbot
Copy link
Collaborator

rustbot commented Mar 15, 2023

Some changes occured in rustc_ty_utils::consts.rs

cc @BoxyUwU

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in engine.rs, potentially modifying the public API of ObligationCtxt.

cc @lcnr, @compiler-errors

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

@compiler-errors
Copy link
Member

compiler-errors commented Mar 15, 2023

Huh -- not sure if I like this better than just normalizing all the method names (make sure they start with mk_, like mk_ty_projection) and moving them to a separate module (ty/mk.rs here). Basically I'm not a huge fan of having to add .mk() on every invocation.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 15, 2023

I do like it 😃

But it does seem affect enough code to warrant an MCP (and then compiler-errors and I can fight it out in a zulip thread 😃).

The MCP should probably be more about the general idea, as it seems applicable to other things, too

@WaffleLapkin WaffleLapkin removed the WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) label Mar 15, 2023
@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added the WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) label Mar 15, 2023
@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
    Checking rustc_traits v0.0.0 (/checkout/compiler/rustc_traits)
    Checking rustc_ty_utils v0.0.0 (/checkout/compiler/rustc_ty_utils)
    Checking rustc_passes v0.0.0 (/checkout/compiler/rustc_passes)
    Checking rustc_mir_build v0.0.0 (/checkout/compiler/rustc_mir_build)
error[E0599]: no method named `mk_fn_sig` found for struct `TyCtxt<'_>` in the current scope
     |
     |
2225 |         let expected_sig = tcx.mk_fn_sig(
     |                                ^^^^^^^^^ help: there is a method with a similar name: `fn_sig`
For more information about this error, try `rustc --explain E0599`.
error: could not compile `rustc_passes` due to previous error
warning: build failed, waiting for other jobs to finish...
error: could not compile `rustc_passes` due to previous error

@bors
Copy link
Contributor

bors commented Mar 16, 2023

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

@nnethercote
Copy link
Contributor

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. I feel like this cure is worse than the disease.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 16, 2023

We could use const generics of Symbol type and do tcx.mk<thing>(args). Tho that would mean multiple args would need to get tupled. Tho at that point we can go all the way and infer the (tupled if multiple) args from the return type and just have tcx.mk(args)

@WaffleLapkin WaffleLapkin added the needs-mcp This change is large enough that it needs a major change proposal before starting work. label Mar 16, 2023
@WaffleLapkin
Copy link
Member Author

@oli-obk @compiler-errors I've opened an MCP, now fight rust-lang/compiler-team#603

@WaffleLapkin
Copy link
Member Author

Closed as the MCP was not accepted.

@WaffleLapkin WaffleLapkin deleted the mk() branch April 13, 2023 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-mcp This change is large enough that it needs a major change proposal before starting work. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants