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

Common Context objects for phases on core and lifted #204

Merged
merged 16 commits into from
Dec 19, 2022

Conversation

b-studios
Copy link
Collaborator

No description provided.

@b-studios b-studios mentioned this pull request Dec 6, 2022
1 task
This is intended to be reused across backends, and contains helper
methods to search in the definitions
The implementation is not very efficient (but this could be improved).
@marzipankaiser
Copy link
Contributor

marzipankaiser commented Dec 14, 2022

Rebased [note: There were no new changes compared to master in the previous version?] and added a first draft of a CoreContext that tracks the effekt.context.Context and some core.Declarations.

@marzipankaiser
Copy link
Contributor

An alternative to the current design could also be to return, from e.g. findField, an object with methods

def isDefined: Boolean
def get: Field
def constructor: Constructor
def declaration: Declaration.Data

etc.
Pro:

  • This would allow us to search through the declarations at most once for each Id
    Con:
  • This is slightly more complicated.

@marzipankaiser
Copy link
Contributor

Additional note on CoreContext: If the Decls in lifted stay like they are (a different type), we would probably want something like this for lifted, too.

Decl is just a stripped-down version of Declaration anyway.
We can add it back once we actually do a type-preserving transformation
from core to lifted.
The Context will be tracked separately as needed.
We only depend on using ErrorReporter.
Later, try dropping the "using Context" in Phases on core (and just use
ErrorReporter), to enforce that we do not depend on the source symbols.
Also uses XRef-case classes to also cache the path to the X
(e.g., for a field, the Data and Constructor).
instead of inspecting the symbols
@marzipankaiser marzipankaiser changed the title Refactor/use declarations in js Common Context objects for phases on core and lifted Dec 16, 2022
@marzipankaiser
Copy link
Contributor

If we want anything after core.Transformer not to inspect symbols, we will have to also encode builtins into the declarations/externs.

@marzipankaiser
Copy link
Contributor

marzipankaiser commented Dec 19, 2022

If we want anything after core.Transformer not to inspect symbols, we will have to also encode builtins into the declarations/externs.

I think this will be a bigger (-ish) and somewhat separate change, and thus make a separate PR for it.

@marzipankaiser marzipankaiser marked this pull request as ready for review December 19, 2022 11:33
This reverts part of 2f3354c.
There, this was deleted in error.
@b-studios
Copy link
Collaborator Author

Otherwise it LGTM

@b-studios b-studios merged commit efe551e into master Dec 19, 2022
@b-studios b-studios deleted the refactor/use-declarations-in-JS branch December 19, 2022 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants