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.

Sign up for GitHub

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

Jump to bottom

Static, but instance, but static #3764

Merged
merged 7 commits into from
Oct 10, 2022
Merged

Static, but instance, but static #3764

merged 7 commits into from
Oct 10, 2022

Conversation

kustosz
Copy link
Contributor

@kustosz kustosz commented Oct 5, 2022

Pull Request Description

Adds the ability to write Foo.method (Mk_Foo 123) as a synonym of (Mk_Foo 123).method because Rust.

Important Notes

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the
    Scala,
    Java,
    and
    Rust
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide build and ./run ide watch.

JaroslavTulach
JaroslavTulach approved these changes Oct 6, 2022
View reviewed changes
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

Just eight changed files? So small PR!

hubertp
hubertp approved these changes Oct 7, 2022
View reviewed changes
Copy link
Collaborator

@hubertp hubertp left a comment •
edited
Loading

Choose a reason for hiding this comment

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

pls remove the print statement, I also wonder whether we can bring back that test?

engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/ImportsTest.scala Outdated
class ImportsTest extends PackageTest {
"Atoms and methods" should "be available for import" in {
evalTestProject("TestSimpleImports") shouldEqual 20
Try(evalTestProject("TestSimpleImports"))
println(consumeOut)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
println(consumeOut)

@kustosz kustosz added the CI: Ready to merge This PR is eligible for automatic merge label Oct 10, 2022
@mergify mergify bot merged commit b3dd778 into develop Oct 10, 2022
@mergify mergify bot deleted the wip/mk/static-but-instance-but-static branch October 10, 2022 19:28
hubertp added a commit that referenced this pull request Jan 24, 2023
@hubertp
Create static wrappers for builtin types
2eaff1e 
#3764 introduced static wrappers
for instance methods. Except it had a limitation to only be allowed for
types with at least a single constructor.
That excluded builtin types as well which, by default, don't have them.
This limitation is problematic for Array/Vector consolidation and makes
builtin types somehow second-citizens.

This change lifts the limitation for builtin types only. Note that we do
not want to share the implementatin of the generated builtin methods.
At the same time due to the additional argument we have to adjust the
starting index of the arguments.
This change avoids messing with the existing dispatch logic, to avoid
unnecessary complexity.

As a result it is now possible to call builtin types' instance methods,
statically:
```
  arr = Array.new_1 42
  Array.length arr
```
That would previously lead to missing method exception in runtime.
The only exception is `Nothing`. Primarily because it requires `Nothing`
to have a proper eigentype (`Nothing.type`) which would messed up a lot
of existing logic.
@hubertp hubertp mentioned this pull request Jan 24, 2023
Merged
4 tasks
hubertp added a commit that referenced this pull request Jan 26, 2023
@hubertp
Create static wrappers for builtin types
5ed8df8 
#3764 introduced static wrappers
for instance methods. Except it had a limitation to only be allowed for
types with at least a single constructor.
That excluded builtin types as well which, by default, don't have them.
This limitation is problematic for Array/Vector consolidation and makes
builtin types somehow second-citizens.

This change lifts the limitation for builtin types only. Note that we do
not want to share the implementatin of the generated builtin methods.
At the same time due to the additional argument we have to adjust the
starting index of the arguments.
This change avoids messing with the existing dispatch logic, to avoid
unnecessary complexity.

As a result it is now possible to call builtin types' instance methods,
statically:
```
  arr = Array.new_1 42
  Array.length arr
```
That would previously lead to missing method exception in runtime.
The only exception is `Nothing`. Primarily because it requires `Nothing`
to have a proper eigentype (`Nothing.type`) which would messed up a lot
of existing logic.
hubertp added a commit that referenced this pull request Jan 30, 2023
@hubertp
Create static wrappers for builtin types
8fe78f7 
#3764 introduced static wrappers
for instance methods. Except it had a limitation to only be allowed for
types with at least a single constructor.
That excluded builtin types as well which, by default, don't have them.
This limitation is problematic for Array/Vector consolidation and makes
builtin types somehow second-citizens.

This change lifts the limitation for builtin types only. Note that we do
not want to share the implementatin of the generated builtin methods.
At the same time due to the additional argument we have to adjust the
starting index of the arguments.
This change avoids messing with the existing dispatch logic, to avoid
unnecessary complexity.

As a result it is now possible to call builtin types' instance methods,
statically:
```
  arr = Array.new_1 42
  Array.length arr
```
That would previously lead to missing method exception in runtime.
The only exception is `Nothing`. Primarily because it requires `Nothing`
to have a proper eigentype (`Nothing.type`) which would messed up a lot
of existing logic.
mergify bot pushed a commit that referenced this pull request Jan 30, 2023
@hubertp
Create static wrappers for builtin types (#4077)
be91b1e 
#3764 introduced static wrappers for instance methods. Except it had a limitation to only be allowed for types with at least a single constructor.
That excluded builtin types as well which, by default, don't have them. This limitation is problematic for Array/Vector consolidation and makes builtin types somehow second-citizens.

This change lifts the limitation for builtin types only. Note that we do want to share the implementation of the generated builtin methods. At the same time due to the additional argument we have to adjust the starting index of the arguments.
This change avoids messing with the existing dispatch logic, to avoid unnecessary complexity.

As a result it is now possible to call builtin types' instance methods, statically:
```
arr = Array.new_1 42
Array.length arr
```
That would previously lead to missing method exception in runtime.

# Important Notes
The only exception is `Nothing`. Primarily because it requires `Nothing` to have a proper eigentype (`Nothing.type`) which would messed up a lot of existing logic for no obvious benefit (no more calling of `foo=Nothing` in parameters being one example).
@wdanilo wdanilo mentioned this pull request Feb 7, 2023
Closed
This was referenced Apr 11, 2023
Closed
Closed
Closed
@JaroslavTulach JaroslavTulach mentioned this pull request Feb 22, 2024
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hubertp hubertp hubertp approved these changes

@JaroslavTulach JaroslavTulach JaroslavTulach approved these changes

@4e6 4e6 Awaiting requested review from 4e6 4e6 is a code owner

Assignees
No one assigned
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kustosz @hubertp @JaroslavTulach