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

Refactor Stdlib #449

Merged
merged 54 commits into from
May 24, 2024
Merged

Refactor Stdlib #449

merged 54 commits into from
May 24, 2024

Conversation

b-studios
Copy link
Collaborator

This PR uses #427 to refactor the stdlib and share more parts of it.

Issues / annoyances (re)discovered on the way:

  • we really need to allow importing "from-the-back" (I remember @jiribenes reported this somewhere)
  • we need to split import into include and import and add a project mode!
  • there are still issues with Use feature flags for extern definitions #427 (@marzipankaiser). In arrays.effekt I meant to write
    extern def get[T](arr: Array[T], index: Int): Option[T] =
      js {  js::undefinedToOption(arr.unsafeGet(index)) }
    
    but it did not work.

@@ -159,8 +159,8 @@ our input:
```
def pipeline(input: String): String =
parse(input) { parseExpr() } match {
case Success(tree) => pretty(translate(tree))
case Failure(msg) => msg
case examples::casestudies::parser::Success(tree) => pretty(translate(tree))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here I'd like to write parser::Success(tree) to disambiguate.

@b-studios
Copy link
Collaborator Author

Working on the PR I understand @phischu's sentiment to keep the stdlib and prelude small. Almost every test will load the stdlib which is not the point of the test. So maybe we can change the prelude for tests?

marzipankaiser and others added 7 commits May 22, 2024 17:32
This will:
- Potentially incur double coercions for arguments
- Probably not work in some cases, as we do not track the return type

But this does fix the bugs we are currently running into.

Note: In the future, a more complete rewrite is needed, as it was
written with parametric polymorphism in mind and is now used for
subtyping. This means the assumption that casts *only* need to happen
at instantiation sites (i.e. call sites) is broken.
// Context.warning(pp"Extern definition ${id} contains extern string without feature flag. This will likely not work in other backends, "
// + pp"please annotate it with a feature flag (Supported by the current backend: ${Context.compiler.supportedFeatureFlags.mkString(", ")})")
// case _ => ()
// }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@marzipankaiser I have this commented out here; don't we have your check on master anymore? The diff suggests this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll go ahead and delete this to minify the diff; please complain if we still need this.

@b-studios b-studios marked this pull request as ready for review May 23, 2024 17:46
@b-studios
Copy link
Collaborator Author

This is finally ready for review. It would be great if someone could help me make this work on the website again since the paths changed from

immutable/list

to just

list

Also, the prelude is much larger (maybe we want to use a smaller prelude on the website?). Can someone check out how long loading and compiling things now takes on the website, given the version in this PR?

Maybe @dvdvgt (since you are familiar with the website)?

@dvdvgt
Copy link
Collaborator

dvdvgt commented May 23, 2024

This is finally ready for review. It would be great if someone could help me make this work on the website again since the paths changed from

immutable/list

to just

list

Also, the prelude is much larger (maybe we want to use a smaller prelude on the website?). Can someone check out how long loading and compiling things now takes on the website, given the version in this PR?

Maybe @dvdvgt (since you are familiar with the website)?

Yes, I'll look into it

@b-studios
Copy link
Collaborator Author

I'm gonna click merge; please don't build the website with this, yet, until @dvdvgt finished fixing things up.

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

3 participants