-
Notifications
You must be signed in to change notification settings - Fork 72
Description
Proposal
Problem Statement
The current naming scheme of the astconv module in crate rustc_hir_analysis is incredibly misleading.
The methods provided by astconv::AstConv do in fact not “[convert] from the AST representation of types to the ty.rs representation” (to quote the module-level documention) but from HIR constructs to ty.rs (rustc_middle::ty) constructs.
The name is very historical dating back to a time when the HIR didn't exist yet! Since the HIR was first introduced in rust-lang/rust#28138 roughly 8½ years ago, the name has never been adjusted!
This outdated nomenclature extends to methods (e.g., ast_ty_to_ty, ast_region_to_region, add_predicates_for_ast_type_binding) and local variables (e.g., ast_ty, ast_bounds) despite them operating on HIR types or being of HIR types, respectively.
This is not at all friendly to new rustc devs and I assume it's slightly annoying for seasoned rustc hackers.
Proposed Solution
I hereby propose renaming the module astconv together with its contents, most notably the trait AstConv, to something that actually reflects its role and functionality. Since “astconv” effectively lowers type-system entities from HIR to rustc_middle::ty representation I choose the terminology HIR ty lowering1 going forward with rustc_hir_analysis::astconv turning into rustc_hir_analysis::hir_ty_lowering and AstConv becoming HirTyLowerer (NB: the concrete names aren't necessarily fixed).
Additionally, I'd like to update the naming scheme of dyn AstConv methods from ast_XXX_to_XXX (and similar) to lower_XXX since method names should generally be verbs and since the current names are relatively hard to parse in my opinion (them consisting of multiple acronyms).
Local variables whose name is of the form ast_xxx and that are of a HIR type should be renamed to hir_xxx.
Note that I've an almost complete patch over at rust-lang/rust#120926 which implements the proposed changes.
Concrete Proposal
NOTE: The complete & normative proposal can be found here: https://fmease.dev/rustc-dev/astconv-no-mo.html (2024-03-22 archive link).
- in crate
rustc_hir_analysis- module
astconv→hir_ty_lowering - quasi-deprecated helper
hir_ty_to_ty→lower_ty
- module
- in module
hir_ty_lowering- trait
AstConv→HirTyLowererallow_ty_infer→allow_inferastconv→lowererget_type_parameter_bounds→probe_ty_param_boundsprojected_ty_from_poly_trait_ref→lower_assoc_ty
- struct
PathSeg→GenericPathSegment
- trait
- methods on formally
dyn AstConv(sorted alphabetically)add_bounds→lower_poly_boundsadd_implicitly_sized→add_sized_boundadd_predicates_for_ast_type_binding→lower_assoc_item_binding2associated_path_to_ty→lower_assoc_pathast_path_args_for_ty→lower_generic_args_of_path_segmentast_path_to_mono_trait_ref→lower_mono_trait_refast_path_to_ty→lower_path_segmentast_region_to_region→lower_lifetimeast_ty_to_ty→lower_tyast_ty_to_ty_in_path→lower_ty_in_pathast_ty_to_ty_inner→lower_ty_commoncompute_bounds→lower_mono_boundsconv_object_ty_poly_trait_ref→lower_trait_object_tycreate_args_for_associated_item→lower_generic_args_of_assoc_itemcreate_args_for_ast_path→lower_generic_args_of_pathdef_ids_for_value_path_segments→probe_generic_path_segmentsfind_bound_for_assoc_item→probe_single_ty_param_bound_for_assoc_tyhir_id_to_bound_const→lower_const_paramhir_id_to_bound_ty→lower_ty_paramimpl_trait_ty_to_ty→lower_opaque_tyinstantiate_mono_trait_ref→lower_impl_trait_ref3instantiate_poly_trait_ref→lower_poly_trait_ref3lookup_assoc_ty→probe_assoc_ty4lookup_assoc_ty_unchecked→probe_assoc_ty_unchecked4lookup_inherent_assoc_ty→probe_inherent_assoc_ty4one_bound_for_assoc_item→probe_single_bound_for_assoc_itemprohibit_assoc_ty_binding→prohibit_assoc_item_binding2prohibit_generics→prohibit_generic_argsqpath_to_ty→lower_qpathreport_ambiguous_associated_type→report_ambiguous_assoc_typeres_to_ty→lower_pathtrait_defines_associated_item_named→probe_trait_that_defines_assoc_itemty_from_delegation→lower_delegation_tyty_of_arg→lower_arg_tyty_of_fn→lower_fn_ty
- generic args lowering
- trait
CreateSubstsForGenericArgsCtxt→GenericArgsLowerer - local struct
SubstsForAstPathCtxt→GenericArgsCtxt(inrustc_hir_analysis) - local struct
MethodSubstsCtxt→GenericArgsCtxt5 (inrustc_hir_typeck) - local struct
CreateCtorSubstsContext→CtorGenericArgsCtxt
- trait
- in
rustc_trait_selection::traitsastconv_object_safety_violations→hir_ty_lowering_object_safety_violations
ConstrainedCollectorPostAstConv→ConstrainedCollectorPostHirTyLowering- in
genericscreate_args_for_parent_generic_args→lower_generic_args6
- on
ItemCtxtto_ty→lower_tytype_parameter_bounds_in_generics→probe_ty_param_bounds_in_generics
- methods on
FnCtxt(sorted alphabetically)array_length_to_const→lower_array_lengthconst_arg_to_ty→lower_const_argto_ty→lower_tyto_ty_saving_user_provided_ty→lower_ty_saving_user_provided_ty
- in
collect:convert_{item,trait_item,impl_item,variant_ctor,enum_variant_types,variant}→lower_$item
Mentors or Reviewers
Implementer: Myself.
Reviewers: @compiler-errors probably.
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 mergeon either the MCP or the PR.
- Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a
- 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.
Footnotes
-
Thanks, @compiler-errors, for the initial suggestion and thanks, @Nilstrieb, for the suggestion to incorporate the word ty to make it clear that this only concerns type-system entities, not MIR building. ↩
-
Not only applies to (assoc) type bindings but also assoc const bindings and RTN (return type notation) ↩ ↩2
-
It's not instantiating anything in the usual sense, it's just lowering. ↩ ↩2
-
Consistent with
probe_single_bound_for_assoc_{item,ty},probe_trait_that_defines_assoc_item,probe_traits_that_match_assoc_ty,probe_adt↩ ↩2 ↩3 -
It's not only used for assoc fns but also for assoc consts ↩
-
Before the “substs → generic args” renaming, this used to be called
create_substs_for_generic_args. The new namecreate_args_for_parent_generic_argsis absolutely incorrect and therefore confusing! We're not “creating generic args for parent generics args” but we're lowering generic args for the item whoseDefIdwas provided (with the help of aGenericArgsLowerer) which also requires the pre-lowered parent generic args to be passed. Respectfully, I think the author of the renaming PR saw theparent_argsparameter and thought that the parent args were being lowered. ↩