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

Newruntime: assignment to discriminant field in case objects not supported #11205

Closed
cooldome opened this issue May 8, 2019 · 5 comments

Comments

Projects
None yet
4 participants
@cooldome
Copy link
Member

commented May 8, 2019

Test case:

type
  MyEnum = enum
    A, B, C
  MyCaseObject = object
    case kind: MyEnum
    of A: iseq: seq[int]
    of B: fseq: seq[float]
    of C: str: string


var x = MyCaseObject()
x.iseq.add 1
x.kind = B
x.fseq.add -3.0

nim c --newruntime rt2.nim fails with

   Error: system module needs: FieldDiscriminantCheck`

The issue is more fundamental then it seems. Descriminator field assignments are quite widespread even in stdlib hence can't be forbidden. Such assignment needs destructor call or even more accurately subdestructor call. Subdestructor will destroy only branches of the union part of the object.

Looks like liftdestructor needs to generate extra destructor for the case field in the object. These destructors are field sym bound and not type bound which makes it difficult. Inject destructor needs to call them on descriminator assignments.

The good news once it is done, existing issues with object variants will be resolved.

@Araq: Possibly you have better ideas?

@Araq

This comment has been minimized.

Copy link
Member

commented May 9, 2019

I would disallow assignments that trigger an object branch switch. :-) I consider it a messy legacy that is not even completely memorysafe.

@zevv

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

The manual already states "The new value must not lead to a change of the active object branch. For an object branch switch system.reset has to be used".

@cooldome

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

The problem is that example above comes from stdlib. Check for example https://github.com/nim-lang/Nim/blob/devel/lib/pure/json.nim: descriminator assignment happens 20+ times.

Manual doesn't mention actively used loophole: descriminant value of zero is treated differently.

@Araq,
Do you plan close the loophole and change all the libraries that is currently using it? It is quite a few.
All serialisation libraries will need to be rewritten for example.

@krux02

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

In my opinion that code example is supposed to crash, even on the old runtime. The only question is how it should crash.

@cooldome Yes the descriminator is assigned many times, but the difference is, it isn't assigned after it's members have been assigned. I don't even know what the code above is supposed to do. Should it reinterpet iseq as fseq or should it destroy iseq after the discriminant has been set.

Btw I see a clear conflict here with discriminant objects and objects that are not initialized with 0 data.

@Araq

This comment has been minimized.

Copy link
Member

commented May 10, 2019

Do you plan close the loophole and change all the libraries that is currently using it? It is quite a few.
All serialisation libraries will need to be rewritten for example.

Yeah, the serialisation problem is a big issue which I have no good solution for.

That said, FieldDiscriminantCheck needs to be implemented for --newruntime but shouldn't rely on the old RTTI which doesn't exist for the new runtime anymore.

@Araq Araq closed this in 8bb1a6b May 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.