Nim icon indicating copy to clipboard operation
Nim copied to clipboard

fixes variable initialization of `concept` types with ORC on Macos and Linux i386

Open ringabout opened this issue 1 year ago • 5 comments

ref https://github.com/nim-lang/Nim/pull/19972#issuecomment-1250199557

given a small test case

type
  stringTest = concept x
    x is string

let usedToFail: stringTest = "111"
echo usedToFail

After matching the concept type and the type of argument using paramTypesMatch, we can infer the type of usedToFail with string instead of keeping using tyConcept type. We don't need to use typeAllowed to check whether the concept is properly constructed because it is checked at the concept definition.

ringabout avatar Sep 18 '22 06:09 ringabout

Please outline why this affects ORC.

Araq avatar Sep 19 '22 13:09 Araq

type
  stringTest = concept x
    x is string

let usedToFail: stringTest = "111"
echo usedToFail

generates code like below

/Users//@[email protected]:209:81: error: initializer element is not a compile-time constant
 N_LIB_PRIVATE NIM_CONST NimStringV2 usedToFail__conceptsZtusertypeclasses_368 = TM__Zcmg9a6xw00tn7dapi0pbqQ_13;

which is not accepted by some C compilers => my first commit verifies that (https://github.com/nim-lang/Nim/pull/20380/commits/cd90ec7410df54e4d89b89efadaf719195db52e1) which only test the specified test with ORC but it failed. Here is the CI https://github.com/nim-lang/Nim/runs/8413141986

It seems to be specific to macos with C backend and Linux i386, probably related to the C compiler. But still I suppose it should be a concrete type instead of a tyConcept type at code gen phase.

ringabout avatar Sep 19 '22 13:09 ringabout

Found it

proc potentialValueInit(p: BProc; v: PSym; value: PNode): Rope =
  if lfDynamicLib in v.loc.flags or sfThread in v.flags or p.hcrOn:
    result = nil
  elif sfGlobal in v.flags and value != nil and isDeepConstExpr(value, p.module.compileToCpp) and
      p.withinLoop == 0 and not containsGarbageCollectedRef(v.typ):
    #echo "New code produced for ", v.name.s, " ", p.config $ value.info
    result = genBracedInit(p, value, isConst = false, v.typ)
  else:
    result = nil

in potentialValueInit, the second branch (isDeepConstExpr) evaluates to true with a tyConcept type. Then genBracedInit gets called,

    of tyString, tyCstring:
      if optSeqDestructors in p.config.globalOptions and n.kind != nkNilLit and ty == tyString:
        result = genStringLiteralV2Const(p.module, n, isConst)
      else:
        var d: TLoc
        initLocExpr(p, n, d)
        result = rdLoc(d)

The snippet above is picked up from the genBracedInit proc, the code differs between refc and arc/orc. So genStringLiteralV2Const will be called for ORC and generates N_LIB_PRIVATE NIM_CONST NimStringV2 usedToFail__conceptsZtusertypeclasses_368 = TM__Zcmg9a6xw00tn7dapi0pbqQ_13; which gives an error error: initializer element is not a compile-time constant.

ringabout avatar Sep 19 '22 13:09 ringabout

Shouldn't we fix isDeepConstExpr instead then?

Araq avatar Sep 20 '22 15:09 Araq

After semVarOrLet, let usedToFail: string = "111" is transformed into let usedToFail: string; usedToFail = "111" while let usedToFail: stringTest = "111" still keeps this form. Should It probably need to be transformed first? Because the value is a stringLiteral(should have been a nkEmpty) so isDeepConst evaluates it to true. There seems to be nothing wrong with isDeepConst.

ringabout avatar Sep 20 '22 15:09 ringabout