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

False positive detection of conflicting branches of object variant #11862

Open
zevv opened this issue Jul 31, 2019 · 5 comments
Open

False positive detection of conflicting branches of object variant #11862

zevv opened this issue Jul 31, 2019 · 5 comments

Comments

@zevv
Copy link
Contributor

zevv commented Jul 31, 2019

This snippet:

import macros 

type
  Kind = enum kOne, kTwo

  Thing = object
    case kind: Kind
      of kOne:
        v1: int
      of kTwo:
        v2: int
               
macro magic(): untyped =
  var b = Thing(kind: kOne, v1: 3)
  echo b                          
  quote do:                       
    `b`    
           
const c = magic()

generates the following error:

/tmp/t.nim(16, 3) Error: The fields ''v1'' and ''v2'' cannot be initialized together, because they are from conflicting branches in the case object.
Nim Compiler Version 0.20.99 [Linux: amd64]
Compiled at 2019-07-31
Copyright (c) 2006-2019 by Andreas Rumpf

git hash: bb949a75db490c9bccae975855310ee2de0b4b28
active boot switches: -d:danger
@deech
Copy link
Contributor

deech commented Jul 31, 2019

This is expected since the body of a macro is evaluated by the VM which doesn't currently support variants.

@mratsim mratsim added Macros Object Variants VM see also `const` label labels Aug 1, 2019
@zevv
Copy link
Contributor Author

zevv commented Aug 1, 2019

Not sure if that is true; If you take out the quote-do block the type is instantiated and evaluated at compile time by the VM just fine:

(kind: kOne, v1: 3)

@ghost
Copy link

ghost commented Aug 1, 2019

The vm treats case objects in a flattened way (like normal objects) with added field checks. This seems fine for loads/store, but breaks for literal constructions from getAst. Thing(kind: kOne, v1: 3, v2: 0) Only fields of the active discriminators should show up in the construction.

As a workaround, you can postprocess the ast removing null initializations, but const caseObject is not supported:

proc patchUpObjConstrs*(n: NimNode) =
  proc isDefault(n: NimNode): bool {.nimcall.} =
    result = case n.kind:
    of nnkExprColonExpr: isDefault(n[1])
    of nnkIntLit, nnkUIntLit: n.intVal == 0
    of nnkFloatLit: n.floatVal == 0
    of nnkBracket: n.len == 0
    of nnkStrLit: n.strVal == ""
    of nnkNilLit: true
    of nnkObjConstr:
      patchUpObjConstrs(n)
      n.len == 1
    else:
      patchUpObjConstrs(n)
      false
  proc findDiscrims(n: NimNode): seq[NimNode] {.nimcall.} =
    case n.kind:
    of nnkObjectTy:
      result.add(findDiscrims(n[2]))
    of nnkRecList:
      for i in 0 ..< n.len:
        result.add(findDiscrims(n[i]))
    of nnkRecCase:
      result.add(n[0])
      for i in 1 ..< n.len:
        result.add(findDiscrims(n[i]))
    else: discard
  proc isDiscrim(discrims: seq[NimNode], init: NimNode): bool {.nimcall.} =
    for discrim in discrims:
      if init[0].eqIdent(discrim):
        return true
  if n.kind == nnkObjConstr:
    let discrims = findDiscrims(getType(n))
    for i in countdown(n.len-1, 1):
      if isDefault(n[i]) and not isDiscrim(discrims, n[i]):
        n.del(i)
  else:
    for i in 0 ..< n.len:
      patchUpObjConstrs(n[i])

@krux02
Copy link
Contributor

krux02 commented Aug 1, 2019

When you want to embed values in a quoted source code block, you always have to to lift the value into a NimNode via newLit. It is a bug that it sometimes works, and the compiler doesn't complain about it.

import macros

type
  Kind = enum kOne, kTwo

  Thing = object
    case kind: Kind
      of kOne:
        v1: int
      of kTwo:
        v2: int

macro magic(): untyped =
  var b = newLit(Thing(kind: kOne, v1: 3))
  echo b.repr
  quote do:
    `b`

const c = magic()

The simple reason is, the result of quote do has to be a NimNode, and the only allowed children of NimNode are other NimNodes.

Since so many people do use quote do incorrectly I have a PR here that provides a fixed version of quote do. One of it's fixes is that it ensures all injected symbols are of type NimNode.

#11823

But as aready mentioned, it does not fix the problem const caseObject is not supported.

@mratsim
Copy link
Collaborator

mratsim commented Aug 2, 2019

From @Araq in #8015 (comment)

Const case objects never were supposed to compile, there is logic in the compiler to detect and reject that but as we can see it's not triggered.

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

No branches or pull requests

4 participants