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

Make the DeclarationEngine a lazy_static as a semi-temporary bandaid #2675

Merged
merged 5 commits into from
Sep 1, 2022

Conversation

emilyaherbert
Copy link
Contributor

Per #2637 (comment), many standard traits like PartialEq need to be special-cased for DeclarationId because each DeclarationId needs to index into the DeclarationEngine (much the same as TypeEngine and TypeId). Of course, the standard traits like PartialEq don't allow for extra arguments, so we have to come up with another solution.

We could either:

  1. create a wrapper type CompileWrapper<'a, T> that takes a &'a T and &'a DeclarationEngine, for example CompileWrapper<'a, DeclarationId>. Then implement PartialEq on CompileWrapper<'a, DeclarationId>. The downside of this is that it introduces a layer of indirection in the compiler (draft branch here: https://github.com/FuelLabs/sway/tree/emilyaherbert/add-partial-eq)
  2. give DeclarationId a &'de DeclarationEngine field so that we can implement PartialEq on DeclarationId directly, meaning that DeclarationId would then require a lifetime annotation DeclarationId<'de>. The downside of this is that it requires the lifetime annotation to bubble up through everything that DeclarationId<'de> touches---including TypedExpression, TypeInfo, etc. (draft branch here: https://github.com/FuelLabs/sway/tree/emilyaherbert/update-declaration-wrapper)

Both of these are big PRs that will take some time to introduce and will create a blocker to the DeclarationEngine. So, I propose that we move the DeclarationEngine to being a lazy_static (similar to the TypeEngine). Then, in the future if/when we remove the lazy_static implementation of TypeEngine, we can do the same operation for DeclarationEngine.

@emilyaherbert emilyaherbert self-assigned this Aug 31, 2022
@emilyaherbert emilyaherbert added the compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen label Aug 31, 2022
@emilyaherbert emilyaherbert requested a review from a team August 31, 2022 20:19
@emilyaherbert emilyaherbert marked this pull request as ready for review August 31, 2022 20:19
@emilyaherbert emilyaherbert enabled auto-merge (squash) August 31, 2022 20:32
@emilyaherbert emilyaherbert requested a review from a team September 1, 2022 00:44
@emilyaherbert emilyaherbert merged commit 9076eb2 into master Sep 1, 2022
@emilyaherbert emilyaherbert deleted the emilyaherbert/de-as-lazy-static branch September 1, 2022 01:25
@mitchmindtree
Copy link
Contributor

The more that we lean into and depend on these global APIs, the trickier it's going to be to untangle their use throughout semantic analysis (Namespace alone was tricky enough). I think I would have recommended we focus on #2063 and your work in #2483 rather than adding more global state.

Per #2637 (comment), many standard traits like PartialEq need to be special-cased for DeclarationId because each DeclarationId needs to index into the DeclarationEngine (much the same as TypeEngine and TypeId). Of course, the standard traits like PartialEq don't allow for extra arguments, so we have to come up with another solution.

I think part of the issue here might be that we're talking about equivalence of the underlying declarations, and not the IDs themselves. I think we should be careful not to confuse the two.

Perhaps we could consider a 3rd option: rather than using a PartialEq implementation for DeclarationId to represent DeclarationWrapper equivalence, we instead use a dedicated trait or method for our particular use-case?

In the described case, we can't know if two declarations referred to by two separate IDs are equivalent without access to the DeclarationEngine. Rather than moving the DeclarationEngine to global state and having a PartialEq implementation for the DeclarationId type that is now side-effectful (due to the global lookup), perhaps we could consider adding a method to DeclarationEngine along the lines of decl_eq(&self, a: Id, b: Id) that can be used for checking declaration equivalence? I think having a method along these lines has the benefit of clearly indicating that we're comparing equivalence of the underlying declarations, and not just the IDs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants