Rename astconv::AstConv and related items · Issue #723 · rust-lang/compiler-team (original) (raw)

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
• in module hir_ty_lowering
  • trait AstConv → HirTyLowerer
    * allow_ty_infer → allow_infer
    * astconv → lowerer
    * get_type_parameter_bounds → probe_ty_param_bounds
    * projected_ty_from_poly_trait_ref → lower_assoc_ty
  • struct PathSeg → GenericPathSegment
• methods on formally dyn AstConv (sorted alphabetically)
  • add_bounds → lower_poly_bounds
  • add_implicitly_sized → add_sized_bound
  • add_predicates_for_ast_type_binding → lower_assoc_item_binding2
  • associated_path_to_ty → lower_assoc_path
  • ast_path_args_for_ty → lower_generic_args_of_path_segment
  • ast_path_to_mono_trait_ref → lower_mono_trait_ref
  • ast_path_to_ty → lower_path_segment
  • ast_region_to_region → lower_lifetime
  • ast_ty_to_ty → lower_ty
  • ast_ty_to_ty_in_path → lower_ty_in_path
  • ast_ty_to_ty_inner → lower_ty_common
  • compute_bounds → lower_mono_bounds
  • conv_object_ty_poly_trait_ref → lower_trait_object_ty
  • create_args_for_associated_item → lower_generic_args_of_assoc_item
  • create_args_for_ast_path → lower_generic_args_of_path
  • def_ids_for_value_path_segments → probe_generic_path_segments
  • find_bound_for_assoc_item → probe_single_ty_param_bound_for_assoc_ty
  • hir_id_to_bound_const → lower_const_param
  • hir_id_to_bound_ty → lower_ty_param
  • impl_trait_ty_to_ty → lower_opaque_ty
  • instantiate_mono_trait_ref → lower_impl_trait_ref3
  • instantiate_poly_trait_ref → lower_poly_trait_ref3
  • lookup_assoc_ty → probe_assoc_ty4
  • lookup_assoc_ty_unchecked → probe_assoc_ty_unchecked4
  • lookup_inherent_assoc_ty → probe_inherent_assoc_ty4
  • one_bound_for_assoc_item → probe_single_bound_for_assoc_item
  • prohibit_assoc_ty_binding → prohibit_assoc_item_binding2
  • prohibit_generics → prohibit_generic_args
  • qpath_to_ty → lower_qpath
  • report_ambiguous_associated_type → report_ambiguous_assoc_type
  • res_to_ty → lower_path
  • trait_defines_associated_item_named → probe_trait_that_defines_assoc_item
  • ty_from_delegation → lower_delegation_ty
  • ty_of_arg → lower_arg_ty
  • ty_of_fn → lower_fn_ty
• generic args lowering
  • trait CreateSubstsForGenericArgsCtxt → GenericArgsLowerer
  • local struct SubstsForAstPathCtxt → GenericArgsCtxt (in rustc_hir_analysis)
  • local struct MethodSubstsCtxt → GenericArgsCtxt5 (in rustc_hir_typeck)
  • local struct CreateCtorSubstsContext → CtorGenericArgsCtxt
• in rustc_trait_selection::traits
  • astconv_object_safety_violations → hir_ty_lowering_object_safety_violations
• ConstrainedCollectorPostAstConv → ConstrainedCollectorPostHirTyLowering
• in generics
  • create_args_for_parent_generic_args → lower_generic_args6
• on ItemCtxt
  • to_ty → lower_ty
  • type_parameter_bounds_in_generics → probe_ty_param_bounds_in_generics
• methods on FnCtxt (sorted alphabetically)
  • array_length_to_const → lower_array_length
  • const_arg_to_ty → lower_const_arg
  • to_ty → lower_ty
  • to_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 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.

1. 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. ↩
2. Not only applies to (assoc) type bindings but also assoc const bindings and RTN (return type notation) ↩ ↩2
3. It's not instantiating anything in the usual sense, it's just lowering. ↩ ↩2
4. 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
5. It's not only used for assoc fns but also for assoc consts ↩
6. Before the “substs → generic args” renaming, this used to be called create_substs_for_generic_args. The new name create_args_for_parent_generic_args is absolutely incorrect and therefore confusing! We're not “creating generic args for parent generics args” but we're lowering generic args for the item whose DefId was provided (with the help of a GenericArgsLowerer) which also requires the pre-lowered parent generic args to be passed. Respectfully, I think the author of the renaming PR saw the parent_args parameter and thought that the parent args were being lowered. ↩