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

Reimplement enso_project as a proper builtin #6352

Merged
merged 39 commits into from
May 2, 2023

Conversation

Akirathan
Copy link
Member

@Akirathan Akirathan commented Apr 19, 2023

Fixes #5050

Pull Request Description

Remove the magical code generation of enso_project method from codegen phase and reimplement it as a proper builtin method.

The old behavior of enso_project was special, and violated the language semantics (regarding the self argument):

  • It was implicitly declared in every module, so it could be called without a self argument.
  • It can be called with explicit module as self argument, e.g. Base.enso_project, or Visualizations.enso_project.

Let's avoid implicit methods on modules and let's be explicit. Let's reimplement the enso_project as a builtin method. To comply with the language semantics, we will have to change the signature a bit:

  • enso_project is a static method in the Standard.Base.Meta.Enso_Project module.
  • It takes an optional project argument (instead of taking it as an explicit self argument).

Having the enso_project defined as a (shadowed) builtin method, we will automatically have suggestions created for it.

Important Notes

  • Truffle nodes are no longer generated in codegen phase for the enso_project method. It is a standard builtin now.
    • The minimal import to use enso_project is now from Standard.Base.Meta.Enso_Project import enso_project.
  • Tested implicitly by org.enso.compiler.ExecCompilerTest#testInvalidEnsoProjectRef.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follow the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@Akirathan Akirathan linked an issue Apr 19, 2023 that may be closed by this pull request
@Akirathan Akirathan self-assigned this Apr 21, 2023
@Akirathan Akirathan added the CI: No changelog needed Do not require a changelog entry for this PR. label Apr 21, 2023
@Akirathan Akirathan force-pushed the wip/akirathan/enso-proj-suggestion-5050 branch 2 times, most recently from e9ab0f4 to 3ae329e Compare April 24, 2023 10:06
@Akirathan Akirathan force-pushed the wip/akirathan/enso-proj-suggestion-5050 branch from 3ae329e to 0fa6cb0 Compare April 24, 2023 10:10
@Akirathan Akirathan changed the title Add enso_project to suggestion database Reimplement enso_project as a proper builtin Apr 24, 2023
@Akirathan Akirathan removed the CI: No changelog needed Do not require a changelog entry for this PR. label Apr 24, 2023
@Akirathan Akirathan marked this pull request as ready for review April 24, 2023 13:08
@4e6
Copy link
Contributor

4e6 commented Apr 24, 2023

Please also add tests checking that

  • the enso_project method works properly in interpreter mode. See the RuntimeServerTest as an example. Notice that it uses micro-distribution instead of standard distribution library, and you may need to extend it with enso_project method as well
  • the engine generates proper enso_project suggestion in RuntimeStdlibTest

CHANGELOG.md Outdated Show resolved Hide resolved
@Akirathan Akirathan added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Apr 24, 2023
@Akirathan Akirathan added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Apr 27, 2023
@Akirathan Akirathan force-pushed the wip/akirathan/enso-proj-suggestion-5050 branch from fbe7491 to 92669ee Compare April 28, 2023 08:30
Copy link
Member

@jdunkerley jdunkerley left a comment

Choose a reason for hiding this comment

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

As the principal API still works am happy with this.

@Akirathan Akirathan added CI: Ready to merge This PR is eligible for automatic merge and removed CI: Clean build required CI runners will be cleaned before and after this PR is built. labels Apr 28, 2023
@@ -42,6 +43,7 @@ export project.Errors
export project.IO
export project.Math
export project.Meta
from project.Meta.Enso_Project export enso_project
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably add enso_project to micro-distribution

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in ef5deaf

Copy link
Member Author

Choose a reason for hiding this comment

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

Even though the module layout is not the same as in the standard distribution, but that should not be a problem, right?

# Conflicts:
#	lib/scala/interpreter-dsl/src/main/java/org/enso/interpreter/dsl/MethodProcessor.java
@Akirathan Akirathan force-pushed the wip/akirathan/enso-proj-suggestion-5050 branch from 7f700c9 to ef5deaf Compare April 28, 2023 16:37
@Akirathan Akirathan added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Apr 29, 2023
@Akirathan
Copy link
Member Author

Akirathan commented May 2, 2023

Cannot reproduce

org.graalvm.polyglot.PolyglotException: java.lang.AssertionError: assertion failed: exported symbol `enso_project` needs to be registered first in the module

(https://github.com/enso-org/enso/actions/runs/4839795157/jobs/8625338504#step:10:765) locally. Ensure "CI: Clean build required" label is set and restarting the jobs.

GitHub
Hybrid visual and textual functional programming. Contribute to enso-org/enso development by creating an account on GitHub.

@Akirathan
Copy link
Member Author

Forcing clean build with clean action. Seems like CI: Clean build require does not work as expected?

@Akirathan Akirathan removed the CI: Clean build required CI runners will be cleaned before and after this PR is built. label May 2, 2023
@mergify mergify bot merged commit 3a42d0c into develop May 2, 2023
@mergify mergify bot deleted the wip/akirathan/enso-proj-suggestion-5050 branch May 2, 2023 16:41
Procrat added a commit that referenced this pull request May 3, 2023
* develop: (34 commits)
  Continued Execution Context work and some little fixes (#6506)
  IDE's logging to a file (#6478)
  Fix application config (#6513)
  Cloud/desktop mode switcher (#6448)
  Fix doubled named arguments bug (#6422)
  Reimplement `enso_project` as a proper builtin (#6352)
  Fix layer ordering between dropdown and breadcrumbs backgrounds.  (#6483)
  Multiflavor layers (#6477)
  DataflowAnalysis preserves dependencies order (#6493)
  Implement `create_database_table` for Database Table (#6467)
  Limit Dead Letter logging (#6482)
  More reliable shutdown of the EnsoContext to save resources (#6468)
  Make execution mode `live` default for CLI (#6496)
  Finishing Vector Editor (#6470)
  Proper handling of multiple list views. (#6461)
  Fix disappearing cached shapes (#6458)
  Add Execution Context control to Text.write (#6459)
  Change defaults for `Connection.tables` and ensure that `Connection.query` recognizes all available tables (#6443)
  Introducing @BuiltinMethod.needsFrame and InlineableNode (#6442)
  Hide profile button behind a feature flag (#6430)
  ...
Procrat added a commit that referenced this pull request May 4, 2023
* develop:
  Fix cut-off in text visualisations (#6421)
  Infer correct synthetic name for nested modules (#6525)
  Delete unused websocket dependency (#6535)
  Run typecheck and eslint on `./run lint` (#6314)
  Force pending saves if client closes abruptly (#6514)
  Continued Execution Context work and some little fixes (#6506)
  IDE's logging to a file (#6478)
  Fix application config (#6513)
  Cloud/desktop mode switcher (#6448)
  Fix doubled named arguments bug (#6422)
  Reimplement `enso_project` as a proper builtin (#6352)
  Fix layer ordering between dropdown and breadcrumbs backgrounds.  (#6483)
  Multiflavor layers (#6477)
  DataflowAnalysis preserves dependencies order (#6493)
  Implement `create_database_table` for Database Table (#6467)
  Limit Dead Letter logging (#6482)
  More reliable shutdown of the EnsoContext to save resources (#6468)
  Make execution mode `live` default for CLI (#6496)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

enso_project does not appear in Component Browser suggestions
9 participants