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

Default fields for objects #12378

Closed
wants to merge 1 commit into from
Closed

Conversation

Clyybber
Copy link
Contributor

@Clyybber Clyybber commented Oct 7, 2019

Implements nim-lang/RFCs#126
Only const values are allowed.

@krux02
Copy link
Contributor

krux02 commented Oct 20, 2019

Can you restrict the default values even further to also not allow allocated values like "seq" and "string"? Until now I've written a lot of code (for example the json.to macro rewrite), that lives under the assumption that var tmp: SomeType cannot have any side effects. Allocation is one of those side effects that I like to avoid and always be aware of. Here is an example that would make be very worried:

type
  MyTypeKind = enum
    mtA, mtB, mtC
  MyType = object
    something: int = -1
    case kind: MyTypeKind = mtC
    of mtA:
      aval: int = 123
    of mtB:
      bval: float = 456.789
    of mtC:
      cval: seq[int] = @[1,2,3]

# generated code for macro json.to[MyType]:
proc initFromJson(dst: var MyType, jsonNode: JsonNode; jsonPath: string) =
  initFromJson(dst.something, getOrDefault(jsonNode, "something"), jsonPath & ".something")
  var kindTmp: MyTypeKind
  initFromJson(kindTmp, jsonNode["kind"], jsonPath & ".kind")
  dst.kind = kindTmp # with some hack to make this an unsafe assignment that compiles
  case dst.kind
  of mtA:
    initFromJson(dst.aval, getOrDefault(jsonNode, "aval"), jsonPath & ".aval")
  of mtB:
    initFromJson(dst.bval, getOrDefault(jsonNode, "bval"), jsonPath & ".bval")
  of mtC:
    initFromJson(dst.cval, getOrDefault(jsonNode, "cval"), jsonPath & ".cval")

If you allow var mt: MyType to have allocations, then initFromJson will have an allocation for the cval field, even if it is later overridden with something else. But much worse, The hack that I did to assign to the kind field is no longer a safe hack. The destructor for the cval field will never be triggered.

@Clyybber
Copy link
Contributor Author

@krux02 That could be easily done. Question is should we? AFAICT this hack shouldn't be allowed in the first place. I don't oppose your POV but I'm also not convinced we should restrict this feature like that.

@krux02
Copy link
Contributor

krux02 commented Oct 21, 2019

@Clyybber with my PR I was forced to introduce that hack, because currently there is no way in Nim to set the kind field of an object that doesn't also resets everything else in the object. In deserialization I need to set the object fields element by element.

@Clyybber Clyybber force-pushed the netsphere branch 2 times, most recently from fbb67f7 to 49b88c1 Compare October 28, 2019 16:09
@krux02
Copy link
Contributor

krux02 commented Oct 29, 2019

I see that you are still active on this. What I am most interested in would be the test cases to see this feature in use. What I would like to see is are tests that cover good error messages for the parts that should not be supported, such as default seq and default string values.

@timotheecour
Copy link
Member

  • for consistency with rest of nim, if x: int = 12 as field is supported, then x = 12 should also be supported; this wasn't clear from this PR nor Support default values for object properties RFCs#126
  • does default(T) use initialization fields like T()?
  • we'd need a way to get both zero-initialized T (ie the current default(T)) as well as field-initialized T, maybe: defaultZero(T) and default(T)

@krux02
Copy link
Contributor

krux02 commented Dec 19, 2019

  • does default(T) use initialization fields like T()?

I would say default(T) should be the same as T(), similar how foo(bar) and bar.foo() are the same.

  • we'd need a way to get both zero-initialized T (ie the current default(T)) as well as field-initialized T, maybe: defaultZero(T) and default(T)

So the reason why we need default values for members are types that are not valid when initialized with zero. And now because values are initialized with something else than zero, we need to have a new function that initializes with zero anyway. In other words you want a defaultZero to construct invalid values. Am I right.

@timotheecour
Copy link
Member

So the reason why we need default values for members are types that are not valid when initialized with zero. And now because values are initialized with something else than zero, we need to have a new function that initializes with zero anyway. In other words you want a defaultZero to construct invalid values. Am I right.

not valid is application specific, the 0-value could still be a valid instance of the class but not the most sensible default. Field initialization simply makes existing patterns more DRY, avoiding the need for initFoo() in many cases.

Furthermore, 0 is special as it allows optimizations by fitting the data in .bss section

defaultZero(T) can be implemented after this PR though.

@Clyybber
Copy link
Contributor Author

Yes default(T) should be the same as T() for objects.

I think a defaultZero(T) is a bad idea. If you want to zero it out, potentially putting it in an invalid state you should use zeroMem

@zah
Copy link
Member

zah commented Apr 18, 2020

I've reviewed this PR as part of my work on #13808 and there are quite a few things that I would change:

  1. The code should be aware of generic types. The default expression may depend on the generic parameters, so it must be subjected to the generics pre-pass first and then potentially evaluated late in semtypinst.

  2. It wouldn't be too hard to allow the default value of one field to reference the value of another field. This is how we do default values in procs for example. I think a proper implementation can have this from the start.

@stale
Copy link

stale bot commented Apr 18, 2021

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.

@stale stale bot added the stale Staled PR/issues; remove the label after fixing them label Apr 18, 2021
@Araq
Copy link
Member

Araq commented Apr 19, 2021

We still want this feature. Right?

@stale stale bot removed the stale Staled PR/issues; remove the label after fixing them label Apr 19, 2021
@stale
Copy link

stale bot commented Apr 19, 2022

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.

@stale stale bot added the stale Staled PR/issues; remove the label after fixing them label Apr 19, 2022
@ZoomRmc
Copy link
Contributor

ZoomRmc commented Apr 19, 2022

Good bot.

@stale stale bot removed the stale Staled PR/issues; remove the label after fixing them label Apr 19, 2022
@dom96
Copy link
Contributor

dom96 commented Apr 20, 2022

Indeed, it's reminding us that we've got a PR for this and hopefully this will prod someone that cares to rebase it :)

@Araq
Copy link
Member

Araq commented Apr 26, 2022

Please take over, @xflywind

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

Successfully merging this pull request may close these issues.

8 participants