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

Meta.meta returns a broken value for Types #5805

Closed
radeusgd opened this issue Mar 3, 2023 · 4 comments · Fixed by #5869
Closed

Meta.meta returns a broken value for Types #5805

radeusgd opened this issue Mar 3, 2023 · 4 comments · Fixed by #5869
Assignees
Labels
Milestone

Comments

@radeusgd
Copy link
Member

radeusgd commented Mar 3, 2023

See the following example:

from Standard.Base import all

type My_Type
    My_Ctor x

main =
    v1 = My_Type.My_Ctor 42
    IO.println (Meta.is_atom v1)
    m1 = Meta.meta v1
    IO.println m1
    IO.println m1.constructor.fields

    v2 = My_Type
    IO.println (Meta.is_atom v2)
    m2 = Meta.meta v2
    case m2 of
        Meta.Atom.Value _ ->
            IO.println m2
            IO.println m2.constructor.fields

Expected behaviour

  • A) it should print:
True
(Atom.Value (My_Ctor 42))
[x]
True
(Atom.Value My_Type)
[]
  • B) it should print sth like:
True
(Atom.Value (My_Ctor 42))
[x]
False
(Type.Value My_Type)

The variant B is my personal preference - I'm not sure if My_Type is and should be an Atom? I think we should introduce a separate Meta.Type to hold the types. This would make our life easier also in regards to #5792, allowing us to check if the second is_a argument is a Type and reject invalid arguments easily.

Current behaviour

True
(Atom.Value (My_Ctor 42))
[x]
True
(Atom.Value My_Type)
Execution finished with an error: Unsupported argument for arg1 expected a AtomConstructor
        at <enso> Meta.get_constructor_fields(Internal)
        at <enso> Constructor.fields<arg-1>(C:\NBO\enso\built-distribution\enso-engine-0.0.0-dev-windows-amd64\enso-0.0.0-dev\lib\Standard\Base\0.0.0-dev\src\Meta.enso:64:37-60)
        at <enso> Constructor.fields(C:\NBO\enso\built-distribution\enso-engine-0.0.0-dev-windows-amd64\enso-0.0.0-dev\lib\Standard\Base\0.0.0-dev\src\Meta.enso:64:9-61)
        at <enso> case_branch<arg-1>(metatype.enso:19:24-44)
        at <enso> case_branch(metatype.enso:19:13-44)
        at <enso> metatype.main(metatype.enso:16-19)

The fields method crashes. This is definitely not desirable.

I'd suggest to fix this by going (B) and not making types Atoms if that is OK. But a fix that will go the (A) route sounds acceptable too. We just shouldn't crash.

@JaroslavTulach
Copy link
Member

Type is treated as atom in IsAtomNode - written by @kustosz.

@JaroslavTulach JaroslavTulach moved this from 📤 Backlog to 👁️ Code review in Issues Board Mar 9, 2023
@JaroslavTulach
Copy link
Member

Variant A seems as the simplest fix: please take a look at #5869.

@radeusgd
Copy link
Member Author

image

I've also found out the same issue happens for Modules which are also not really atoms but are treated as such.

Does the fix also apply there? Can we add a test case for it?

@mergify mergify bot closed this as completed in #5869 Mar 13, 2023
mergify bot pushed a commit that referenced this issue Mar 13, 2023
Fixes #5805 by returning `[]` as list of fields of `Type`.

# Important Notes
`Type` is recognized as `Meta.is_atom` since #3671. However `Type` isn't an `Atom` internally. We have to provide special handling for it where needed.
@github-project-automation github-project-automation bot moved this from 👁️ Code review to 🟢 Accepted in Issues Board Mar 13, 2023
@enso-bot
Copy link

enso-bot bot commented Mar 14, 2023

Jaroslav Tulach reports a new STANDUP for yesterday (2023-03-13):

Progress: - Meta.get_constructor_fields works for Type: #5869
- CI: benchmarks last PR: #4081
- Reported issue: #5898
- Watching videos of https://medium.com/@enso_org/solving-preppin-data-challenges-with-enso-e527e204462a
- Standup & discussion
- Meeting with Dmitry
- analyzing 48MB of .ir caches: #5567 (comment)
- meeting Greg
- libs standup & brainstroming
- discussion about Optional: https://discord.com/channels/@me/959434339182067712/1084850758555545670
- defer serialization of IR: #5911
- prototype of getting 48MB down to 42MB
- demo meetings It should be finished by 2023-03-13.

Next Day: Continue investigating 48MB .ir format.

Discord
Discord is the easiest way to communicate over voice, video, and text. Chat, hang out, and stay close with your friends and communities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants