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

State accepts only Type as key - explain in docs #7572

Closed
Akirathan opened this issue Aug 11, 2023 · 3 comments · Fixed by #7585
Closed

State accepts only Type as key - explain in docs #7572

Akirathan opened this issue Aug 11, 2023 · 3 comments · Fixed by #7585
Assignees
Labels
--bug Type: bug -compiler -libs Libraries: New libraries to be implemented p-low Low priority

Comments

@Akirathan
Copy link
Member

The following code fails on Uninitialized_State panic:

main =
  State.run "my_state"  42 <|
    State.get "my_state"

This is because in both cases "my_state" is a different instance of Text and we do not have equals method implemented on Text. See the javadoc of DynamicObjectLibrary:

Property keys are compared using object identity (`==`) first and then `Object.equals(Object)`. It is
therefore recommended to use the same string/key instances for each access of the property in order
to avoid pulling in `equals`.

Putting types as keys works because types are singletons. So the following works:

type My_Type
main =
  State.run My_Type 42 <|
    State.get My_Type

Ideally, we should compare keys with EqualsNode, at least on the slow path. Or update the documentation of State methods.

@Akirathan Akirathan added p-low Low priority --bug Type: bug -compiler -libs Libraries: New libraries to be implemented labels Aug 11, 2023
@JaroslavTulach JaroslavTulach changed the title Methods manipulating with State accept only Type as key State accepts only Type as key - explain in docs Aug 13, 2023
@jdunkerley
Copy link
Member

Can we make it so that the methods throw and error if the key is not a type object.

@jdunkerley jdunkerley moved this from ❓New to 📤 Backlog in Issues Board Aug 15, 2023
@enso-bot
Copy link

enso-bot bot commented Aug 16, 2023

Jaroslav Tulach reports a new STANDUP for yesterday (2023-08-15):

Progress: - State & Type PR: #7585

Next Day: Towards Python Interop & TCK

@JaroslavTulach JaroslavTulach moved this from 📤 Backlog to 🔧 Implementation in Issues Board Aug 16, 2023
@JaroslavTulach JaroslavTulach moved this from 🔧 Implementation to 👁️ Code review in Issues Board Aug 16, 2023
@github-project-automation github-project-automation bot moved this from 👁️ Code review to 🟢 Accepted in Issues Board Aug 16, 2023
@enso-bot
Copy link

enso-bot bot commented Aug 17, 2023

Jaroslav Tulach reports a new STANDUP for yesterday (2023-08-16):

Progress: - review Column.round: #7521 (review)

Next Day: Towards Python Interop & TCK

GitHub
Pull Request Description Fixes #7572 by requiring keys to be Type instances. Checklist Please ensure that the following checklist has been satisfied before submitting the PR:

All code follows the...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
--bug Type: bug -compiler -libs Libraries: New libraries to be implemented p-low Low priority
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants