fix #8519 T.distinctBase to reverse T = distinct A #8531

timotheecour · 2018-08-03T20:38:26Z

mratsim · 2018-08-04T09:25:31Z

Usage

I'd like something to undistinct variables as well, current usage would require:

var x: distinct seq[seq[int]] # replace by a complex type
(x.type.undistinct)(x)

for conversion

Simple Tests

There should be a test case with generics and static type.

I checked that generics work:

import typetraits
type
  Foo[T] = distinct seq[T]
var a: Foo[int]
doAssert undistinct(type a) is seq[int]

Stress test

Unfortunately as-is undistinct doesn't pass my "type system stress test" which combines macro, static and generics.

Super complex recursive big int type taken from Stint.

import macros, typetraits

macro uintImpl*(bits: static[int]): untyped =
  # Release version, StUint[64] = uint64.
  assert (bits and (bits-1)) == 0, $bits & " is not a power of 2"
  assert bits >= 8, "The number of bits in a should be greater or equal to 8"

  if bits >= 128:
    let inner = getAST(uintImpl(bits div 2))
    result = newTree(nnkBracketExpr, ident("UintImpl"), inner)
  elif bits == 64:
    result = ident("uint64")
  elif bits == 32:
    result = ident("uint32")
  elif bits == 16:
    result = ident("uint16")
  elif bits == 8:
    result = ident("uint8")
  else:
    error "Fatal: unreachable"

macro undistinct*(T: typedesc, recursive: static[bool] = false): untyped =
  ## reverses ``type T = distinct A``

  let typeNode = getTypeImpl(T)
  expectKind(typeNode, nnkBracketExpr)
  if $typeNode[0] != "typeDesc":
    error "expected typeDesc, got " & $typeNode[0]
  var typeSym = typeNode[1]
  if not recursive:
    let impl = getTypeImpl(typeSym)
    if $impl.typeKind != "ntyDistinct":
      error "type is not distinct"
    getTypeInst(impl[0])
  else:
    while true:
      let impl = getTypeImpl(typeSym)
      if $impl.typeKind != "ntyDistinct":
        typeSym = impl
        break
      typeSym=getTypeInst(impl[0])
    typeSym

type
  # ### Private ### #
  BaseUint* = UintImpl or SomeUnsignedInt

  UintImpl*[Baseuint] = object
    when system.cpuEndian == littleEndian:
      lo*, hi*: BaseUint
    else:
      hi*, lo*: BaseUint

  Uint[bits: static[int]] = distinct uintImpl(bits)



var a: Uint[128]


echo undistinct(type a)
# Error: cannot instantiate UintImpl
# got: <uint64>
# but expected: <Baseuint>

This is another case of https://github.com/nim-lang/Nim/issues/7719 / nim-lang/RFCs#44

It is very easy to get Cannot instantiate or type mismatch because of forgetting to convert the NimSym given by getTypeInst to NimIdent if they are not used raw.

undistinct returns NimSym due to getTypeInst.

Instead making sure to generate fresh idents work.

import macros, typetraits

macro uintImpl*(bits: static[int]): untyped =
  # Release version, StUint[64] = uint64.
  assert (bits and (bits-1)) == 0, $bits & " is not a power of 2"
  assert bits >= 8, "The number of bits in a should be greater or equal to 8"

  if bits >= 128:
    let inner = getAST(uintImpl(bits div 2))
    result = newTree(nnkBracketExpr, ident("UintImpl"), inner)
  elif bits == 64:
    result = ident("uint64")
  elif bits == 32:
    result = ident("uint32")
  elif bits == 16:
    result = ident("uint16")
  elif bits == 8:
    result = ident("uint8")
  else:
    error "Fatal: unreachable"

proc replaceNodes*(ast: NimNode): NimNode =
  # Replace NimIdent and NimSym by a fresh ident node
  proc inspect(node: NimNode): NimNode =
    case node.kind:
    of {nnkIdent, nnkSym}:
      return ident($node)
    of nnkEmpty:
      return node
    of nnkLiterals:
      return node
    else:
      var rTree = node.kind.newTree()
      for child in node:
        rTree.add inspect(child)
      return rTree
  result = inspect(ast)

macro undistinct*(T: typedesc, recursive: static[bool] = false): untyped =
  ## reverses ``type T = distinct A``
  runnableExamples:
    import typetraits
    type T = distinct int
    doAssert undistinct(T) is int
    doAssert: not compiles(undistinct(int))
    type T2 = distinct T
    doAssert undistinct(T2, recursive = false) is T
    doAssert undistinct(int, recursive = true) is int
    doAssert undistinct(T2, recursive = true) is int

  let typeNode = getTypeImpl(T)
  expectKind(typeNode, nnkBracketExpr)
  if $typeNode[0] != "typeDesc":
    error "expected typeDesc, got " & $typeNode[0]
  var typeSym = typeNode[1]
  if not recursive:
    let impl = getTypeImpl(typeSym)
    if $impl.typeKind != "ntyDistinct":
      error "type is not distinct"
    getTypeInst(impl[0]).replaceNodes # <---- fresh ident generator
  else:
    while true:
      let impl = getTypeImpl(typeSym)
      if $impl.typeKind != "ntyDistinct":
        typeSym = impl
        break
      typeSym=getTypeInst(impl[0])
    typeSym.replaceNodes # <---- fresh ident generator

type
  # ### Private ### #
  BaseUint* = UintImpl or SomeUnsignedInt

  UintImpl*[Baseuint] = object
    when system.cpuEndian == littleEndian:
      lo*, hi*: BaseUint
    else:
      hi*, lo*: BaseUint

  Uint[bits: static[int]] = distinct uintImpl(bits)

var a: Uint[128]

echo undistinct(type a) # UintImpl[system.uint64]

Araq · 2018-08-04T14:27:06Z

undistinct should be called baseType, that is the term the manual uses. Alternatively it can be called skipDistinct.

timotheecour · 2018-08-04T22:22:43Z

baseType is too confusing, already has different meanings (eg would make it hard to search). skipDistinct sounds weird but at least has distinct in it. I changed to distinctBase which is better IMO, combining the requirements of having distinct, and having base of baseType

timotheecour · 2018-08-04T22:34:03Z

@mratsim
[note in meantime, i changed undistinct to another name]

I'd like something to undistinct variables as well, current usage would require:
(x.type.undistinct)(x)

I agree, let's leave it for another PR though:

var x: distinct seq[seq[int]] # replace by a complex type
let y=x.undistinct
# would translate to:
let y=cast[x.type.undistinct](x)
# seems more efficient than `let y=(x.type.undistinct)(x) ?`

timotheecour · 2018-08-04T23:00:29Z

@mratsim ok it was easy and useful enough so I added the distinctBase overload for variables:

type T = distinct int
var a: T = T(1)
let b = a.distinctBase

NOTE: this PR reinforces my opinion that generics should only be used for IFTI (see https://github.com/nim-lang/Nim/issues/7517)
it would've been ambiguous if I had use distinctBase[T](recursive: static[bool] = false) instead of macro distinctBase*(T: typedesc, recursive: static[bool] = false) because of the generic overload that operates on variables

timotheecour · 2018-08-05T03:16:28Z

@mratsim thanks a bunch for your stress test example and implementation fix! I've added your fix, cleaned it up a bit, and added a slightly simplified version of your stress test.

PTAL

GULPF · 2018-08-05T04:48:57Z

let y=cast[x.type.undistinct](x)
# seems more efficient than `let y=(x.type.undistinct)(x) ?`

Why would it be more efficient? Conversion from a distinct type shouldn't generate any code at all. cast is unsafe and should be avoided if possible.

timotheecour · 2018-08-05T07:53:29Z

EDIT ~~wasnt sure, will fix~~ DONE

Araq · 2018-08-05T09:54:06Z

lib/pure/sugar.nim

+  proc inspect(node: NimNode): NimNode =
+    case node.kind:
+    of nnkIdent, nnkSym:
+      return ident($node)


Use result instead.

Araq · 2018-08-05T09:54:11Z

lib/pure/sugar.nim

+    of nnkIdent, nnkSym:
+      return ident($node)
+    of nnkEmpty, nnkLiterals:
+      return node


Use result instead.

Araq · 2018-08-05T09:54:17Z

lib/pure/sugar.nim

+      var rTree = node.kind.newTree()
+      for child in node:
+        rTree.add inspect(child)
+      return rTree


Use result instead.

Araq · 2018-08-05T09:55:31Z

lib/pure/sugar.nim

+      return rTree
+  result = inspect(ast)
+
+macro distinctBase*(T: typedesc, recursive: static[bool] = false): untyped =


I think it should always be recursive. If I want to skip the 'distinct' indirections, I want to skip all of them.

/cc @mratsim @Araq

I think it should always be recursive. If I want to skip the 'distinct' indirections, I want to skip all of them.

same rationale as having both getTypeImpl and getImpl, plus other constructs where we want a direct vs a recursive behavior.

also, recursive = false is a saner default IMO:
distinctBase(T) will give clean compile error in case T is not a distinct type (ie, catches bugs)
distinctBase(T, recursive = true) will work (and recurse if applies), analog to the difference between "mkdir" and "mkdir -p"

well indeed, I don't see the point of "mkdir" vs "mkdir -p", just create the directory recursively and stop wasting my time. ;-)

I've changed it to be always recursive for the sake of moving on with this PR (but comments like "stop wasting my time" are not helpful as they don't address the points I mentioned: eg, some applications require the non-recursive getImpl and cannot work with its recursive variant getTypeImpl)

I'm sorry, but "stop wasting my time" referred to mkdir's design.

Araq · 2018-08-05T09:56:34Z

lib/pure/sugar.nim

+  else:
+    while true:
+      let impl = getTypeImpl(typeSym)
+      if $impl.typeKind != "ntyDistinct":


This test is if impl.typeKind != ntyDistinct.

Araq · 2018-08-05T09:56:53Z

lib/pure/sugar.nim

+  var typeSym = typeNode[1]
+  if not recursive:
+    let impl = getTypeImpl(typeSym)
+    if $impl.typeKind != "ntyDistinct":


This is if impl.typeKind != ntyDistinct.

Araq · 2018-08-05T09:57:43Z

lib/pure/sugar.nim

+      typeSym=getTypeInst(impl[0])
+  typeSym.replaceNodes
+
+proc distinctBase*[T](a: T, recursive: static[bool] = false): auto =


Why do we need this proc? Please remove it.

Hint why is this suspicious: You cannot even write down its return type explicitly.

To convert a variable without needing to do:

(type a.type.disctinctBase)(a), you would just use a.distinctBase instead.

The signature can be changed to:

template distinctBase*[T](a: T, recursive: static[bool] = false): distinctBase(T, recursive) = ## converts a distinct variable to it's original type runnableExamples: type T = distinct int var a: T = T(1) let b = a.distinctBase doAssert b is int doAssert b == 1 distinctBase(T, recursive)(a)

Here I think func {.inline.} or template are better than simple proc, it's basically a no-op in the C backend.

@Araq indeed, this feature was suggested by @mratsim and I agree with him that it's useful.

Changed to Here I think func {.inline.} (not template, according to rule of preferring procs/funcs whenever possible)

basically a no-op in the C backend

I would hope with high enough optimization level the {.inline.} would not make a difference though

You cannot even write down its return type explicitly.

that's because of #8545

so I kept auto

timotheecour · 2018-08-06T03:01:04Z

PTAL, all comments addressed

timotheecour · 2018-08-06T21:16:58Z

PTAL, all 2nd round comments addressed

Araq · 2018-08-07T07:32:16Z

lib/pure/sugar.nim

+  typeSym.replaceNodes
+
+# using `auto` return pending https://github.com/nim-lang/Nim/issues/8551
+func distinctBase*[T](a: T): auto {.inline.} =


Again, why doesn't the macro version suffice?

the discussion of that was here: #8531 (comment)
@mratsim had suggested it, and I added it because I agreed with him it made sense:
instead of (type(a).disctinctBase)(a), you would just use a.distinctBase instead

Please remove it, all this bloat everywhere...

sigh... done.

timotheecour · 2018-08-07T20:10:19Z

PTAL

Araq · 2018-08-09T08:29:52Z

lib/pure/sugar.nim

+    of nnkEmpty, nnkLiterals:
+      result = node
+    else:
+      var rTree = node.kind.newTree()


Use result here. That's why it exists.

done (thanks!)

Araq · 2018-08-09T08:31:28Z

lib/pure/sugar.nim

@@ -198,3 +198,42 @@ macro dump*(x: typed): untyped =
  let r = quote do:
    debugEcho `s`, " = ", `x`
  return r
+
+# TODO: consider exporting this in macros.nim
+proc replaceNodes(ast: NimNode): NimNode =


This should be called freshIdentNodes or similar.

…riginal type

and fixed distinctBase with replaceNodes; simplified a bit implementation

; * distinctBase now requires T to be distinct (even being recursive) * fixed a bug in (now-default) recursive distinctBase that would only trigger on complex stress test in tsugar.nim

timotheecour · 2018-08-10T04:25:13Z

@Araq PTAL, tests completed and all comments addressed

timotheecour · 2020-03-09T23:33:45Z

@mratsim since this is being revived here #13274 (comment) I was just testing and:

Unfortunately as-is undistinct doesn't pass my "type system stress test" which combines macro, static and generics.

this seems to have disappeared bw 0.19.6 and 0.20.2, so that freshIdentNodes (that you originally suggested as replaceNodes in #8531 (comment) ) doesn't seem needed anymore after that; not only that but freshIdentNodes was actually causing #13031 (comment) IIUC.

so the magic replacement introduced in #13031 may not be needed anymore and could potentially be replaced by a macro (the original one I had minus freshIdentNodes)

timotheecour · 2021-07-30T22:23:16Z

we should add a flag to make it non-recursive as option

refs
https://discord.com/channels/371759389889003530/371759389889003532/870793498264236042

metagn · 2024-01-04T11:35:28Z

we should add a flag to make it non-recursive as option

Assuming this is talking about distinctBase, it does, removing followup label

This was referenced Aug 3, 2018

proposal (with implementation): undistinct(T) #8519

Closed

seq[int].T is int should compile (and be true) according to spec; also fails with type seq2[T] = T #8433

Open

timotheecour force-pushed the pr_undistinct branch from 61fe527 to 788f1f9 Compare August 4, 2018 22:21

timotheecour changed the title ~~fix #8519 T.undistinct to revese T = distinct A~~ fix #8519 T.distinctBase to revese T = distinct A Aug 4, 2018

timotheecour changed the title ~~fix #8519 T.distinctBase to revese T = distinct A~~ fix #8519 T.distinctBase to reverse T = distinct A Aug 4, 2018

timotheecour force-pushed the pr_undistinct branch from 788f1f9 to 1663a21 Compare August 4, 2018 22:24

timotheecour force-pushed the pr_undistinct branch from 17903bb to eb2e73a Compare August 5, 2018 00:38

timotheecour mentioned this pull request Aug 5, 2018

BUG: case inconsistently accepts set of argument #8537

Closed

Araq reviewed Aug 5, 2018

View reviewed changes

timotheecour force-pushed the pr_undistinct branch from b226046 to 05e2900 Compare August 6, 2018 02:11

This was referenced Aug 6, 2018

SIGSEGV when return type depends on a static[T] parameter #8545

Closed

BUG: can't refer to generic type in return type #8551

Open

timotheecour force-pushed the pr_undistinct branch from 05e2900 to 24a4a9e Compare August 6, 2018 21:05

Araq reviewed Aug 7, 2018

View reviewed changes

timotheecour force-pushed the pr_undistinct branch from 24a4a9e to 051c11f Compare August 7, 2018 20:09

Araq reviewed Aug 9, 2018

View reviewed changes

timotheecour added 2 commits August 9, 2018 19:25

fix nim-lang#8519 T.distinctBase to reverse T = distinct A

78875b2

add distinctBase for variables: converts a distinct variable to its o…

7f3dfd1

…riginal type

timotheecour added 7 commits August 9, 2018 19:25

add tests/stdlib/tsugar.nim ; add test case from @mratsim

2b2bb94

add complex stress test case from @mratsim, simplified to its essence;

0a1a180

and fixed distinctBase with replaceNodes; simplified a bit implementation

use T() instead of cast

f4bd55a

address reviewer comments

73b46e0

* removed option for non recursive distinctBase after request from @Araq

e96bf0a

; * distinctBase now requires T to be distinct (even being recursive) * fixed a bug in (now-default) recursive distinctBase that would only trigger on complex stress test in tsugar.nim

remove overload of distinctBase for variables (requested by @Araq)

ba708f8

address comments

cf864d9

timotheecour force-pushed the pr_undistinct branch from 051c11f to cf864d9 Compare August 10, 2018 02:28

Araq merged commit 43f634d into nim-lang:devel Aug 10, 2018

timotheecour deleted the pr_undistinct branch August 10, 2018 08:06

mratsim mentioned this pull request Oct 2, 2018

[typetraits] echo A[B] prints differently depending on unrelated code #8569

Closed

mratsim mentioned this pull request Mar 24, 2019

cannot instantiate T when generating AST from macro #10902

Open

mratsim mentioned this pull request Apr 23, 2019

untyped macro nested in template: getting sym instead of idents #11091

Open

mratsim mentioned this pull request Aug 4, 2019

Macro generated generic proc doesnt work #11877

Open

timotheecour mentioned this pull request Jan 6, 2020

distinctBase type trait for distinct types #13031

Merged

timotheecour mentioned this pull request Mar 25, 2020

distinctBase overload for values #13746

Merged

timotheecour added the TODO: followup needed remove tag once fixed or tracked elsewhere label Jul 30, 2021

mratsim mentioned this pull request Feb 23, 2022

Cannot instantiate Future got: <T> but expected: <T> after bumping nim-chronos status-im/nimbus-eth1#963

Closed

metagn removed the TODO: followup needed remove tag once fixed or tracked elsewhere label Jan 4, 2024

fix #8519 T.distinctBase to reverse T = distinct A #8531

fix #8519 T.distinctBase to reverse T = distinct A #8531

Conversation

timotheecour commented Aug 3, 2018

mratsim commented Aug 4, 2018 • edited

Usage

Simple Tests

Stress test

Araq commented Aug 4, 2018

timotheecour commented Aug 4, 2018 • edited

timotheecour commented Aug 4, 2018

timotheecour commented Aug 4, 2018 • edited

timotheecour commented Aug 5, 2018

GULPF commented Aug 5, 2018 • edited

timotheecour commented Aug 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mratsim Aug 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timotheecour commented Aug 6, 2018

timotheecour commented Aug 6, 2018

Choose a reason for hiding this comment

timotheecour Aug 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timotheecour commented Aug 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timotheecour commented Aug 10, 2018

timotheecour commented Mar 9, 2020

timotheecour commented Jul 30, 2021

metagn commented Jan 4, 2024

mratsim commented Aug 4, 2018 •

edited

timotheecour commented Aug 4, 2018 •

edited

timotheecour commented Aug 4, 2018 •

edited

GULPF commented Aug 5, 2018 •

edited

timotheecour commented Aug 5, 2018 •

edited

mratsim Aug 5, 2018 •

edited

timotheecour Aug 7, 2018 •

edited