-
Notifications
You must be signed in to change notification settings - Fork 13.8k
rename select_where_possible
and select_all_or_error
#147111
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
base: master
Are you sure you want to change the base?
rename select_where_possible
and select_all_or_error
#147111
Conversation
Some changes occurred in compiler/rustc_codegen_ssa Some changes occurred to the CTFE machinery Some changes occurred in engine.rs, potentially modifying the public API of Some changes occurred to constck cc @fee1-dead Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor Some changes occurred in compiler/rustc_passes/src/check_attr.rs Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
I feel like the "registered" part of these methods names is redundant, and removing it would make the method names appreciably shorter. Not sure if there's any value in being explicit about only evaluating "registered" obligations. |
This comment has been minimized.
This comment has been minimized.
I've been clippy'd 😔 @compiler-errors I was worried it might imply the methods accept a list of obligations to evaluate, but I think you're right that we could just drop the
|
Some changes occurred in src/tools/clippy cc @rust-lang/clippy rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead. cc @rust-lang/rust-analyzer |
ah I should revert the r-a changes, I didn't realise r-a had its own fullfillment ctxt impl |
Could also use lgtm |
We'll probably want to rename it there, so the code stays closer to what's in rustc. |
r? @lcnr
I find that people get confused by what these methods do. The verb "select" is not really that helpful and is just a reference to somewhat of an implementation detail of the trait solvers that doesn't even apply to most obligation kinds.
I went with
try_evaluate_registered_obligations
andevaluate_registered_obligations_or_error
. I'm not necessarily the biggest fan as its quite long name wise. The new solver's "added goals" instead of "registered obligations" is much nicer :> but I think it is worth maintaining consistency with methods such asregister_obligation
and the fact that we useObligation
all over the API of these types. Anything else would be confusing imo.I'm not sure "evaluate" is the right verb, it's atleast what we use inside of the new solver so feels more accurate than "select" especially as we move towards getting rid of the old solver. I think it might be nice to rename all the new solver
compute_x_goal
methods toevaluate_x_goal
but 🤷♀️Also unsure if we intend to change the "public" APIs of trait solving once old solver is gone and that'll affect naming here in a way that might make it desirable to hold off for now 🤔