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

Autoscoped constructors #9190

Merged
merged 29 commits into from
Mar 4, 2024
Merged

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Feb 26, 2024

Pull Request Description

Fixes #8645 by recognizing ~ prefix to constructor names.

Checklist

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

  • The documentation has been updated in bc5c6fd
  • All code follows the
    Scala,
    Java,
    style guides.
  • All code has been tested:
    • Unit tests have been written
    • Enso unit tests with examples
    • Test for error conditions
    • Write new benchmarks

@enso-bot enso-bot bot mentioned this pull request Feb 27, 2024
Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for addressing my concerns and the thorough tests

Copy link
Member

@Akirathan Akirathan left a comment

Choose a reason for hiding this comment

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

LGTM. Seems that you are also correctly passing around State. Once you get to writing Enso tests, please, don't forget to test something like:

type My_Type
    False

    materialize t:My_Type =
        State.get Number . should_equal 42
        t.to_text

main =
    State.run Number 42 <|
        My_Type.materialize ~False

@JaroslavTulach
Copy link
Member Author

8266b52 introduces new benchGenerateListAutoscoping. Use as:

sbt:runtime-benchmarks> benchOnly benchGenerateList
[info] AtomBenchmarks.benchGenerateList             avgt    5     4.026 ±   0.252  ms/op
[info] AtomBenchmarks.benchGenerateListAutoscoping  avgt    5  2581.386 ± 191.845  ms/op
[info] AtomBenchmarks.benchGenerateListQualified    avgt    5     4.294 ±   0.542  ms/op

right now the benchGenerateListAutoscoping is 500x times slower then other benchmarks not using autoscoping of constructors, but we'll make it faster.

@JaroslavTulach JaroslavTulach force-pushed the wip/jtulach/AutoscopedConstructors_8645 branch from 7fa1cc0 to ea194cb Compare February 29, 2024 07:06
@JaroslavTulach JaroslavTulach force-pushed the wip/jtulach/AutoscopedConstructors_8645 branch from ea194cb to 59fa776 Compare February 29, 2024 07:43
@JaroslavTulach
Copy link
Member Author

However, I think we should strive to give good error messages on failure. Could you share what is the error message if I try to use a ~Foo autoscoped constructor and the constructor is not found?

After 6987672 the message is Error: Type error: expected expression to be Back, but got ~Value

@JaroslavTulach
Copy link
Member Author

LGTM. Seems that you are also correctly passing around State. Once you get to writing Enso tests, please, don't forget to test something like:

type My_Type
    False

    materialize t:My_Type =
        State.get Number . should_equal 42
        t.to_text

main =
    State.run Number 42 <|
        My_Type.materialize ~False

Thanks for bringing the topic of State up, @Akirathan. I believe it actually wasn't correct and your test wasn't testing State during materialization of the UnresolvedConstructor. Since a026086 the behavior shall be correct (if I understand what proper State behavior is supposed to be properly).

@radeusgd
Copy link
Member

radeusgd commented Mar 2, 2024

However, I think we should strive to give good error messages on failure. Could you share what is the error message if I try to use a ~Foo autoscoped constructor and the constructor is not found?

After 6987672 the message is Error: Type error: expected expression to be Back, but got ~Value

Personally I think it would be best to have a more specific message, e.g. Could not resolve ~Value as a constructor on type Back.. But I guess the current one is okay-ish and we can revise later when we see how this behaves in practice.


nothing = not isJust
- **Autoscoped Constructors:** Referencing constructors via their type name may
Copy link
Member Author

Choose a reason for hiding this comment

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

Documenting autoscoped constructors is hard when most of the surrounding documentation is outdated...

@JaroslavTulach
Copy link
Member Author

have a more specific message, e.g. Could not resolve ~Value as a constructor on type Back..

Done in 6e32a3a

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Mar 4, 2024

Done in 6e32a3a

Now we have a missing TruffleBoundary problem... let's concatenate the message via ropes in 60bfc76

@JaroslavTulach JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label Mar 4, 2024
@mergify mergify bot merged commit 5676618 into develop Mar 4, 2024
26 of 27 checks passed
@mergify mergify bot deleted the wip/jtulach/AutoscopedConstructors_8645 branch March 4, 2024 11:41
@JaroslavTulach
Copy link
Member Author

We are in. Dmitry reported

as a followup issue to make static dropdowns work in the IDE.

@JaroslavTulach
Copy link
Member Author

Syntax is being changed by

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.

Autoscoping of constructors
3 participants