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.

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

Introduce DeclRef and remove excessive clones to DeclId #4037

Merged
merged 7 commits into from
Feb 14, 2023

Conversation

emilyaherbert
Copy link
Contributor

@emilyaherbert emilyaherbert commented Feb 9, 2023

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:
    #[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 Perform monomorphization and constraint solving after type checking #3744)
    /// 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 Perform monomorphization and constraint solving after type checking #3744)
        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 Perform monomorphization and constraint solving after type checking #3744.
  5. Changes other necessary variants, aside from TyDeclaration::FunctionDeclaration and TyExpressionVariant::FunctionApplication.
  6. Changes the DeclEngine API to take &T where ..., allowing us to remove more unnecessary instances of .clone().

Checklist

  • 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.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@emilyaherbert emilyaherbert self-assigned this Feb 9, 2023
@emilyaherbert emilyaherbert added code quality compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen labels Feb 9, 2023
@emilyaherbert emilyaherbert requested a review from a team February 9, 2023 20:00
@emilyaherbert emilyaherbert marked this pull request as ready for review February 9, 2023 20:00
@emilyaherbert emilyaherbert enabled auto-merge (squash) February 9, 2023 20:00
mohammadfawaz
mohammadfawaz previously approved these changes Feb 9, 2023
Copy link
Contributor

@mohammadfawaz mohammadfawaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.

Copy link
Contributor

@tritao tritao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is fine, if we're keeping the same design.

But since you're working on this area again, do we really need to keep the span and ident on a DeclRef/DeclId?

If I remember correctly that span is only used for a diagnostic which I've never even seen before. And can't we get the name from the declaration linked by the ID anyway?

@JoshuaBatty
Copy link
Member

I would ask that Spans and Idents be included wherever possible as they are the primary type that we rely on in the language server for doing anything.

@emilyaherbert
Copy link
Contributor Author

If I remember correctly that span is only used for a diagnostic which I've never even seen before. And can't we get the name from the declaration linked by the ID anyway?

Also the DeclRef type will get at least one additional field in #3744, so it's convenient to have the distinction. I'm trying to split up the refactoring bit of that PR before I submit "the real PR" ™️ with the core logic 😅

@emilyaherbert emilyaherbert marked this pull request as draft February 9, 2023 22:40
auto-merge was automatically disabled February 9, 2023 22:40

Pull request was converted to draft

@emilyaherbert emilyaherbert marked this pull request as ready for review February 13, 2023 21:51
@emilyaherbert emilyaherbert enabled auto-merge (squash) February 13, 2023 21:51
Copy link
Contributor

@otrho otrho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK. Can see most of it is DeclId -> DeclRef, LGTM.

Copy link
Member

@JoshuaBatty JoshuaBatty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, looks good!

@emilyaherbert emilyaherbert merged commit 30cdb10 into master Feb 14, 2023
@emilyaherbert emilyaherbert deleted the emilyaherbert/decl-ref branch February 14, 2023 03:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants