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

Perform monomorphization and constraint solving after type checking #3744

Closed
wants to merge 13 commits into from
Closed

Perform monomorphization and constraint solving after type checking #3744

wants to merge 13 commits into from

Conversation

emilyaherbert
Copy link
Contributor

@emilyaherbert emilyaherbert commented Jan 10, 2023 •
edited
Loading

This PR:

  1. Make the DeclIds unique with a 1:1 ratio for data structure definition. This means that we can compare DeclIds directly, reduces the number of DeclIds that we produce, and reduces the number of times that we need to perform lookups in the DeclEngine
    a. Modify PartialEqWithEngines to no longer compare the DeclIds in the AST directly
    b. Remove the "parents" abstraction in the DeclEngine
  2. Refactors monomorphization into a separate step following type checking
    a. This include trait constraint solving

Closes #3403
Closes #1267
Closes #2636

Blocked by:

@emilyaherbert emilyaherbert added enhancement New feature or request P: high Should be looked at if there are no critical issues left compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen labels Jan 10, 2023
@emilyaherbert emilyaherbert self-assigned this Jan 10, 2023
@emilyaherbert emilyaherbert force-pushed the emilyaherbert/param-stack branch from 7ae3fb5 to 465e696 Compare January 12, 2023 23:04
@emilyaherbert emilyaherbert force-pushed the emilyaherbert/param-stack branch 3 times, most recently from 277805d to 2f8ac02 Compare January 30, 2023 18:04
@emilyaherbert emilyaherbert added P: critical Should be looked at before anything else compiler: collection Everything to do with graph collection, type collection, and the collection context. and removed P: high Should be looked at if there are no critical issues left labels Jan 31, 2023
@emilyaherbert emilyaherbert changed the title Use a type param stack instead of deep recursive copying, to reduce compile times Perform monomorphization and constraint solving after type checking Jan 31, 2023
@tritao
Copy link
Contributor

tritao commented Jan 31, 2023

Do you think we can keep the parents abstraction? I am using it in #3878

The problem there is that I need to go from the monomorphized type back to the parent non-monomorph type. Or should we come up with something else for that?

@emilyaherbert
Copy link
Contributor Author

Do you think we can keep the parents abstraction? I am using it in #3878
The problem there is that I need to go from the monomorphized type back to the parent non-monomorph type. Or should we come up with something else for that?

Thanks for letting me know @tritao! Currently I'm planning to remove the parents abstraction as it will no longer be necessary for monomorphization or type checking. I think we could either come up with something else clever, or in the interim we could construct a slimmed down version of the parents abstraction during monomorphization.

This was referenced Feb 7, 2023
Merged
Merged
Merged
emilyaherbert added a commit that referenced this pull request Feb 8, 2023
@emilyaherbert
Add a new name field on DeclId and remove some unnecessary lookup…
6b3ae49 
…s in the `DeclEngine` (#4028)

## Description

This PR is a subset of #3744. It adds a `name` field to `DeclId` which
contains the name of the declaration. This is helpful in that it allows
us to bypass lookups in the `DeclEngine` for trivial name info and is
also a subset of #3744.

This PR also removes/refactors some of these unnecessary lookups in the
`DeclEngine` in light of the new `name` field.

## Checklist

- [x] I have linked to any relevant issues.
- [ ] 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).
- [ ] 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: emilyaherbert <emily.herbert@fuel.sh>
@emilyaherbert emilyaherbert mentioned this pull request Feb 9, 2023
Merged
7 tasks
This was referenced Feb 9, 2023
Closed
Merged
emilyaherbert added a commit that referenced this pull request Feb 9, 2023
@emilyaherbert
Add HashWithEngines impls for more types (#4014)
1e37122 
## Description

This PR is a subset of #3744. It adds a `HashWithEngines` for the types
required in #3744. Note---this isn't a necessary change into `master`
right at this current moment, just simply a way of breaking up #3744 😄

This PR also makes a few corrections to some of the existing
implementations for `HashWithEngines` which were incorrectly comparing
`TypeId`s.

## Checklist

- [x] I have linked to any relevant issues.
- [ ] I have commented my code, particularly in hard-to-understand
areas.
- [ ] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [ ] I have added tests that prove my fix is effective or that my
feature works.
- [ ] 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: emilyaherbert <emily.herbert@fuel.sh>
emilyaherbert added a commit that referenced this pull request Feb 14, 2023
@emilyaherbert
Introduce DeclRef and remove excessive clones to DeclId (#4037)
30cdb10 
## Description

This PR is a subset of #3744. It introduces `DeclRef`, which is a smart
wrapper type around `DeclId` and contains additional information aside
from the `DeclId`. This allows us to make `DeclId` a copy type and
remove excessive clones of `DeclId` and `DeclRef`.

This PR is a subset of #3744, so while not explicitly necessary right
now on `master`, there will be additional fields added to `DeclRef` in
#3744.

In detail, this PR:
1. Changes `DeclId` to a copy type defined as:
    ```rust
    #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Ord, PartialOrd)]
    pub struct DeclId(usize);
    ```
2. Creates a new `DeclRef` type which is a handle to a use of a
declaration: (there will be more fields added to this in #3744)
    ```rust
    /// A reference to the use of a declaration.
    #[derive(Debug, Clone)]
    struct DeclRef {
        /// The name of the declaration.
        name: Ident,
        /// The index into the [DeclEngine].
        id: DeclId,
        /// The [Span] of the entire declaration.
        decl_span: Span,
    }
    ```
3. Changes the definition of the `TyDeclaration::FunctionDeclaration`
variant to contain additional fields: (there will be more fields added
to this in #3744)
    ```rust
        FunctionDeclaration {
            name: Ident,
            decl_id: DeclId,
            decl_span: Span,
        },
    ```
4. Changes the definiton of the
`TyExpressionVariant::FunctionApplication` variant to contain a
`DeclRef` instead of just the previous `DeclId`. The
`TyExpressionVariant::FunctionApplication` variant gets a `DeclRef`
because `DeclRef` is a handle to a declaration _usage_, while the
`TyDeclaration::FunctionDeclaration` variant does not get a `DeclRef`
because it is simply an AST node for the function declaration. This
distinction will be more clear/necessary in #3744.
5. Changes the `DeclEngine` API to take `&T where ...`, allowing us to
remove more unnecessary instances of `.clone()`.

## Checklist

- [x] I have linked to any relevant issues.
- [ ] 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).
- [ ] 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: emilyaherbert <emily.herbert@fuel.sh>
@emilyaherbert emilyaherbert mentioned this pull request Feb 14, 2023
Merged
7 tasks
@emilyaherbert emilyaherbert force-pushed the emilyaherbert/param-stack branch from dd5f936 to 7d63d45 Compare February 15, 2023 01:40
@emilyaherbert emilyaherbert changed the base branch from master to emilyaherbert/ord-with-engines February 15, 2023 01:41
@emilyaherbert emilyaherbert mentioned this pull request Feb 15, 2023
Merged
6 tasks
emilyaherbert added a commit that referenced this pull request Feb 15, 2023
@emilyaherbert
Small refactor to the method call to type check a list of `TypeParame…
77a1eea 
…ter` (#4089)

## Description

This PR is a small refactor to the method call to type check a list of
`TypeParameter`. This is to reduce the diff on #3744. This PR should
have no practical or logical effect on `master`.

## 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 (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: emilyaherbert <emily.herbert@fuel.sh>
Base automatically changed from emilyaherbert/ord-with-engines to master February 15, 2023 21:31
emilyaherbert added a commit that referenced this pull request Feb 15, 2023
@emilyaherbert
Implement OrdWithEngines for some types (#4082)
d3d6d33 
## Description

This PR is a subset of #3744. It adds a `OrdWithEngines` for the types
required in #3744. Note---this isn't a necessary change into master
right at this current moment, just simply a way of breaking up #3744 😄

## Checklist

- [x] I have linked to any relevant issues.
- [x] I have commented my code, particularly in hard-to-understand
areas.
- [ ] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [ ] 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.
@emilyaherbert emilyaherbert force-pushed the emilyaherbert/param-stack branch from 8f7cb42 to 47e4e98 Compare April 3, 2023 18:58
@emilyaherbert emilyaherbert mentioned this pull request Apr 3, 2023
Closed
7 tasks
@emilyaherbert emilyaherbert force-pushed the emilyaherbert/param-stack branch from 47e4e98 to ba9cc72 Compare April 3, 2023 21:37
@emilyaherbert emilyaherbert force-pushed the emilyaherbert/param-stack branch 3 times, most recently from 3e75134 to 634b587 Compare April 5, 2023 16:56
@emilyaherbert emilyaherbert force-pushed the emilyaherbert/param-stack branch from 634b587 to 7dd04c7 Compare April 18, 2023 19:36
@emilyaherbert emilyaherbert mentioned this pull request Jun 1, 2023
Open
@emilyaherbert emilyaherbert requested a review from IGI-111 June 1, 2023 23:14
@emilyaherbert emilyaherbert removed their assignment Jun 1, 2023
@emilyaherbert emilyaherbert requested review from IGI-111 and removed request for IGI-111 June 1, 2023 23:18
@emilyaherbert emilyaherbert force-pushed the emilyaherbert/param-stack branch from 7dd04c7 to 2e17cca Compare June 1, 2023 23:19
@IGI-111 IGI-111 added this to the October 2023 milestone Jul 6, 2023
@anton-trunov anton-trunov mentioned this pull request Jul 27, 2023
Closed
@IGI-111 IGI-111 closed this Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@IGI-111 IGI-111

Labels
blocked compiler: collection Everything to do with graph collection, type collection, and the collection context. compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen enhancement New feature or request P: critical Should be looked at before anything else
Projects
None yet
Milestone
0
4 participants
@emilyaherbert @tritao @anton-trunov @IGI-111