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.

Sign up for GitHub

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

Jump to bottom

Accumulate typechecking methods in impl TypeCheckContext #5050

Closed
anton-trunov opened this issue Aug 28, 2023 · 1 comment
Closed

Accumulate typechecking methods in impl TypeCheckContext #5050

anton-trunov opened this issue Aug 28, 2023 · 1 comment
Assignees
@anton-trunov
Labels
compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen

Comments

@anton-trunov
Copy link
Contributor

anton-trunov commented Aug 28, 2023 •
edited
Loading

There is a bunch of methods in Namespace, TypeCheckContext and some other related modules that take several components of TypeCheckContext as parameters (like Engines, Namespace). By consolidating all such methods under impl TypeCheckContext we simplify the signatures of those methods and provide a stepping stone for resolving #5036 (the fix proposed there includes propagating the typechecking context into a lot of methods which makes the Rust borrow checker unhappy, e.g. when there is a mutable ctx.namespace and mutable ctx passed to some namespace methods.

This issue will be resolved as a series of PRs:
@anton-trunov anton-trunov self-assigned this Aug 28, 2023
@anton-trunov anton-trunov added the compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen label Aug 28, 2023
@anton-trunov anton-trunov mentioned this issue Aug 28, 2023
Closed
anton-trunov added a commit that referenced this issue Sep 4, 2023
@anton-trunov
Accumulate typechecking methods in impl TypeCheckContext
d42359b 
All methods from `impl Namespace` that also take `Engines` as a parameter
have been moved into `impl TypeCheckContext`.

List of changes:

- `ctx.namespace.find_method_for_type` -> `ctx.find_method_for_type`
- `ctx.namespace.find_constant_for_type` -> `ctx.find_constant_for_type`
- `ctx.namespace.get_items_for_type_and_trait_name` -> `ctx.get_items_for_type_and_trait_name`
- `ctx.namespace.insert_trait_implementation` -> `ctx.insert_trait_implementation`
- `ctx.namespace.variant_import` -> `ctx.variant_import`
- `ctx.namespace.item_import` -> `ctx.item_import`
- `ctx.namespace.self_import` -> `ctx.self_import`
- `ctx.namespace.variant_star_import` -> `ctx.variant_star_import`
- `ctx.namespace.star_import` -> `ctx.star_import`
- `ctx.namespace.find_items_for_type` -> `ctx.find_items_for_type`
- `ctx.namespace.resolve_call_path_with_visibility_check` -> `ctx.resolve_call_path_with_visibility_check`
- `ctx.namespace.resolve_type_with_self` -> `ctx.resolve_type_with_self` (and the previous specialized version in `ctx` removed)
- `ctx.namespace.resolve_type_without_self` -> `ctx.resolve_type_without_self` (and the previous specialized version in `ctx` removed)

Partially resolves issue #5050
anton-trunov added a commit that referenced this issue Sep 4, 2023
@anton-trunov
Accumulate typechecking methods in impl TypeCheckContext
c5b3bb2 
All methods from `impl Namespace` that also take `Engines` as a parameter
have been moved into `impl TypeCheckContext`.

List of changes:

- `ctx.namespace.find_method_for_type` -> `ctx.find_method_for_type`
- `ctx.namespace.find_constant_for_type` -> `ctx.find_constant_for_type`
- `ctx.namespace.get_items_for_type_and_trait_name` -> `ctx.get_items_for_type_and_trait_name`
- `ctx.namespace.insert_trait_implementation` -> `ctx.insert_trait_implementation`
- `ctx.namespace.variant_import` -> `ctx.variant_import`
- `ctx.namespace.item_import` -> `ctx.item_import`
- `ctx.namespace.self_import` -> `ctx.self_import`
- `ctx.namespace.variant_star_import` -> `ctx.variant_star_import`
- `ctx.namespace.star_import` -> `ctx.star_import`
- `ctx.namespace.find_items_for_type` -> `ctx.find_items_for_type`
- `ctx.namespace.resolve_call_path_with_visibility_check` -> `ctx.resolve_call_path_with_visibility_check`
- `ctx.namespace.resolve_type_with_self` -> `ctx.resolve_type_with_self` (and the previous specialized version in `ctx` removed)
- `ctx.namespace.resolve_type_without_self` -> `ctx.resolve_type_without_self` (and the previous specialized version in `ctx` removed)

Partially resolves issue #5050
@anton-trunov anton-trunov mentioned this issue Sep 4, 2023
Merged
7 tasks
anton-trunov added a commit that referenced this issue Sep 4, 2023
@anton-trunov
Accumulate typechecking methods in impl TypeCheckContext
c070619 
All methods from `impl Namespace` that also take `Engines` as a parameter
have been moved into `impl TypeCheckContext`.

List of changes:

- `ctx.namespace.find_method_for_type` -> `ctx.find_method_for_type`
- `ctx.namespace.find_constant_for_type` -> `ctx.find_constant_for_type`
- `ctx.namespace.get_items_for_type_and_trait_name` -> `ctx.get_items_for_type_and_trait_name`
- `ctx.namespace.insert_trait_implementation` -> `ctx.insert_trait_implementation`
- `ctx.namespace.variant_import` -> `ctx.variant_import`
- `ctx.namespace.item_import` -> `ctx.item_import`
- `ctx.namespace.self_import` -> `ctx.self_import`
- `ctx.namespace.variant_star_import` -> `ctx.variant_star_import`
- `ctx.namespace.star_import` -> `ctx.star_import`
- `ctx.namespace.find_items_for_type` -> `ctx.find_items_for_type`
- `ctx.namespace.resolve_call_path_with_visibility_check` -> `ctx.resolve_call_path_with_visibility_check`
- `ctx.namespace.resolve_type_with_self` -> `ctx.resolve_type_with_self` (and the previous specialized version in `ctx` removed)
- `ctx.namespace.resolve_type_without_self` -> `ctx.resolve_type_without_self` (and the previous specialized version in `ctx` removed)

Partially resolves issue #5050
anton-trunov added a commit that referenced this issue Sep 5, 2023
@anton-trunov @JoshuaBatty
Accumulate typechecking methods in impl TypeCheckContext, part 1 (#…
36ccf52 
…5087)

## Description

Partially resolves issue #5050

All methods from `impl Namespace` from the file `namespace.rs` that also
take `Engines` as a parameter have been moved into `impl
TypeCheckContext`.

I'm splitting the refactoring into several PRs to solicit early
feedback, make review process a bit easier and potentially create fewer
merge conflicts.

List of changes:

- `ctx.namespace.find_method_for_type` -> `ctx.find_method_for_type`
- `ctx.namespace.find_constant_for_type` -> `ctx.find_constant_for_type`
- `ctx.namespace.get_items_for_type_and_trait_name` ->
`ctx.get_items_for_type_and_trait_name`
- `ctx.namespace.insert_trait_implementation` ->
`ctx.insert_trait_implementation`
- `ctx.namespace.variant_import` -> `ctx.variant_import`
- `ctx.namespace.item_import` -> `ctx.item_import`
- `ctx.namespace.self_import` -> `ctx.self_import`
- `ctx.namespace.variant_star_import` -> `ctx.variant_star_import`
- `ctx.namespace.star_import` -> `ctx.star_import`
- `ctx.namespace.find_items_for_type` -> `ctx.find_items_for_type`
- `ctx.namespace.resolve_call_path_with_visibility_check` ->
`ctx.resolve_call_path_with_visibility_check`
- `ctx.namespace.resolve_type_with_self` -> `ctx.resolve_type_with_self`
(and the previous specialized version in `ctx` removed)
- `ctx.namespace.resolve_type_without_self` ->
`ctx.resolve_type_without_self` (and the previous specialized version in
`ctx` removed)

## Checklist

- [x] I have linked to any relevant issues.
- [x] I have commented my code, particularly in hard-to-understand
areas.
- [x] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [x] I have added tests that prove my fix is effective or that my
feature works.
- [x] I have added (or requested a maintainer to add) the necessary
`Breaking*` or `New Feature` labels where relevant.
- [x] I have done my best to ensure that my PR adheres to [the Fuel Labs
Code Review
Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [x] I have requested a review from the relevant team or maintainers.

Co-authored-by: Joshua Batty <[email protected]>
anton-trunov added a commit that referenced this issue Sep 5, 2023
@anton-trunov
Partially resolves issue #5050
5221ba3 
All the `impl TypeEngine` methods from the file `type_system/engine.rs` that also
take `Engines` and `Namespace` as parameters have been moved into `impl TypeCheckContext`.

All the `impl Root` methods from the file `namespace/root.rs` that also
take `Engines` as parameters have been moved into `impl TypeCheckContext`.

I'm splitting the refactoring into several PRs to solicit early
feedback, make review process a bit easier and potentially create fewer
merge conflicts.

List of changes:

- `type_engine.monomorphize` -> `ctx.monomorphize_with_modpath`
- `type_engine.resolve` -> `ctx.resolve`
- `type_engine.resolve_with_self` -> `ctx.resolve_with_self`
- `type_engine.MonomorphizeHelper` -> `ctx.MonomorphizeHelper`
- `type_engine.EnforceTypeArguments` -> `ctx.EnforceTypeArguments`
- `namespace.root.resolve_call_path_with_visibility_check` -> `ctx.resolve_call_path_with_visibility_check_and_modpath`
@anton-trunov anton-trunov mentioned this issue Sep 5, 2023
Merged
7 tasks
anton-trunov added a commit that referenced this issue Sep 6, 2023
@anton-trunov
Accumulate typechecking methods in impl TypeCheckContext, part 2 (#…
e00df8a 
…5088)

## Description

Partially resolves issue #5050.

All the `impl TypeEngine` methods from the file `type_system/engine.rs`
that also take `Engines` and `Namespace` as parameters have been moved
into `impl TypeCheckContext`.

All the `impl Root` methods from the file `namespace/root.rs` that also
take `Engines` as parameters have been moved into `impl
TypeCheckContext`.

I'm splitting the refactoring into several PRs to solicit early
feedback, make review process a bit easier and potentially create fewer
merge conflicts.

List of changes:

- `type_engine.monomorphize` -> `ctx.monomorphize_with_modpath`
- `type_engine.resolve` -> `ctx.resolve`
- `type_engine.resolve_with_self` -> `ctx.resolve_with_self`
- `type_engine.MonomorphizeHelper` -> `ctx.MonomorphizeHelper`
- `type_engine.EnforceTypeArguments` -> `ctx.EnforceTypeArguments`
- `namespace.root.resolve_call_path_with_visibility_check` ->
`ctx.resolve_call_path_with_visibility_check_and_modpath`

## Checklist

- [x] I have linked to any relevant issues.
- [x] I have commented my code, particularly in hard-to-understand
areas.
- [x] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [x] I have added tests that prove my fix is effective or that my
feature works.
- [x] I have added (or requested a maintainer to add) the necessary
`Breaking*` or `New Feature` labels where relevant.
- [x] I have done my best to ensure that my PR adheres to [the Fuel Labs
Code Review
Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [x] I have requested a review from the relevant team or maintainers.
@anton-trunov
Copy link
Contributor Author

I found a different approach to solve issue #5036 and this accumulation issue is not relevant anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@anton-trunov anton-trunov

Labels
compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@anton-trunov