Refactored to use implicit insert and remove insert_type method from context object #3395

[mostafa630]

This solution resolves issue #3395 by refactoring the code to use implicit_insert_type instead of insert_type, improving clarity and eliminating the insert_type method from the context object.
Changed files:
* typecheck/rust-typecheck-context.cc
(TypeCheckContext::insert_type): Remove.
* typecheck/rust-hir-type-check.h: Remove declaration of insert_type method.
* typecheck/rust-tyty-subst.cc: Replace all insert_type with insert_implicit_type.
* typecheck/rust-tyty-util.cc: Likewise.
* typecheck/rust-tyty.cc: Likewise.
* typecheck/rust-substitution-mapper.cc: Likewise.
* typecheck/rust-hir-trait-resolve.cc: Likewise.
* typecheck/rust-hir-type-check-base.cc: Likewise.
* typecheck/rust-hir-type-check-enumitem.cc: Likewise.
* typecheck/rust-hir-type-check-expr.cc: use Likewise.
* typecheck/rust-hir-type-check-implitem.cc: Likewise.
* typecheck/rust-hir-type-check-item.cc: use Likewise.
* typecheck/rust-hir-type-check-pattern.cc: Likewise.
* typecheck/rust-hir-type-check-stmt.cc: Likewise.
* typecheck/rust-hir-type-check-struct.cc: Likewise.
* typecheck/rust-hir-type-check-type.cc: Likewise.
* typecheck/rust-hir-type-check.cc: Likewise.
* typecheck/rust-unify.cc: Likewise.

[powerboat9]

You don't have to keep recreating the PR, you can just force-push

[powerboat9]

Looks good, just missing the removal of a few insert_type calls

[mostafa630]

Looks good, just missing the removal of a few insert_type calls

Should I only remove the remaining ones, or should I replace them with insert_implicit_type?

[powerboat9]

Replace them with insert_implicit_type

[dkm]

FYI, changelog is not really correct. You should have things like:

   * typecheck/rust-substitution-mapper.cc (SubstMapperInternal::Resolve): use insert_implicit_type.
   * typecheck/rust-typecheck-context.cc (TypeCheckContext::insert_type): Remove.
   (TypeCheckContext::compute_inference_variables): use insert_implicit_type.

...
You're not supposed to only point at the file where you've made changes, but to more precise "item" (function, type definition, etc).
Also, instead of repeating the same thing in sequence, you can use the idiomatic "Likewise" (have a look at existing entries in git log). Do not hesitate to ask for help if needed :)

[mostafa630]

@dkm can I use clang-format-13 or I must use version 10 because I have some problems in installing version 10

[powerboat9]

I use version 19, which only very rarely disagrees with version 10. You can always try it with version 13 and see if the CI complains

[mostafa630]

I use version 19, which only very rarely disagrees with version 10. You can always try it with version 13 and see if the CI complains

@powerboat9 Thanks! I tried it with version 13, and it works well. All tests passed successfully.

[P-E-P]

@dkm I wonder if pseudonyms are allowed for sign off

[philberty]

LGTM can you just verify if we still need that node_id_refs map?

[philberty]

can you check to see if we can remove the node_id_refs map? Is it still used elsewhere?

[mostafa630]

The following functions utilize the node_id_refs map:

-TypeCheckContext::lookup_builtin(NodeId id, TyTy::BaseType **type)
-TypeCheckContext::insert_builtin(HirId id, NodeId ref, TyTy::BaseType *type)
-TypeCheckContext::insert_type_by_node_id(NodeId ref, HirId id)
-TypeCheckContext::lookup_type_by_node_id(NodeId ref, HirId *id)

but , these functions are only exist in two files:
-rust-hir-type-check.h
-rust-typecheck-context.cc

Additionally, while TypeCheckContext::lookup_builtin is used elsewhere, the version being used is not the one mentioned above. Instead, the used version is an overloaded function:
-TypeCheckContext::lookup_builtin (std::string name, TyTy::BaseType **type)

Given this, we can safely remove the node_id_refs map and all functions that depend on it, as doing so will not impact the code , unless the project requires them in the future or in a context that is not clear to me.

[mostafa630]

sorry after more research i found that there are files in other folders like resolve call some function that use
node_id_refs map
and that is example :
in the file : rust-name-resolver.cc in the resolve folder it call that :

tyctx->insert_builtin (unit_tyty->get_ref (), unit_type->get_node_id (),
			 unit_tyty);

and that insert_builtin uses node_id_refs map

[philberty]

no problem we can probably remove that i will make a new issue then

[philberty]

I think you need to fix your sign-off for DCO you are using a pseudonym

https://gcc.gnu.org/dco.html

[mostafa630]

I think you need to fix your sign-off for DCO you are using a pseudonym

https://gcc.gnu.org/dco.html

I changed the sign-off to match the DCO.

[dkm]

I think you need to fix your sign-off for DCO you are using a pseudonym
https://gcc.gnu.org/dco.html

I changed the sign-off to match the DCO.

erm, I thought they were going to relax this rule. At least, it was mentioned during last Cauldron I think.

(please note that my initial comment on changelog section still holds...)

[mostafa630]

I think you need to fix your sign-off for DCO you are using a pseudonym
https://gcc.gnu.org/dco.html

I changed the sign-off to match the DCO.

erm, I thought they were going to relax this rule. At least, it was mentioned during last Cauldron I think.

(please note that my initial comment on changelog section still holds...)

sorry , but do you mean that i should make this :
* typecheck/rust-hir-type-check.h: Remove declaration of insert_type method.
like that :
* typecheck/rust-typecheck-context.cc
(TypeCheckContext::insert_type): Remove.

or do you mean that i don't mentioned which function exactly i changed it :
* typecheck/rust-tyty-subst.cc: Replace all insert_type with insert_implicit_type.

(i don't mentioned functions because there are many of them so the change log will be huge)

[philberty]

Your git signoff line needs to mention your full name it cant be a username

[mostafa630]

Your git signoff line needs to mention your full name it cant be a username

Okay, I will use that: Mostafa Mahmoud Younis.
But is there any further update in the message change log?