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

implementation for newLit on distinct types (fixes #13266) #13274

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@
- Added `minIndex` and `maxIndex` to the `sequtils` module
- Added `os.isRelativeTo` to tell whether a path is relative to another
- Added `resetOutputFormatters` to `unittest`

- Added a new generic overload of `newLit` for distinct types in `macros`

## Library changes

Expand Down
15 changes: 8 additions & 7 deletions lib/core/macros.nim
Original file line number Diff line number Diff line change
Expand Up @@ -725,13 +725,6 @@ proc newLit*(s: string): NimNode {.compileTime.} =
result = newNimNode(nnkStrLit)
result.strVal = s

when false:
# the float type is not really a distinct type as described in https://github.com/nim-lang/Nim/issues/5875
proc newLit*(f: float): NimNode {.compileTime.} =
## Produces a new float literal node.
result = newNimNode(nnkFloatLit)
result.floatVal = f

proc newLit*(f: float32): NimNode {.compileTime.} =
## Produces a new float literal node.
result = newNimNode(nnkFloat32Lit)
Expand Down Expand Up @@ -798,6 +791,14 @@ proc newLit*(arg: tuple): NimNode {.compileTime.} =
for a,b in arg.fieldPairs:
result.add nnkExprColonExpr.newTree(newIdentNode(a), newLit(b))

macro undistinct[T: distinct](arg: T): untyped =
Copy link
Contributor

@Clyybber Clyybber Feb 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use distinctBase instead?

Copy link
Contributor Author

@krux02 krux02 Feb 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well distinctBase requires a new import/dependency. I don't like that. But more importantly distinctBase works on typedesc. typedesc is a source of problems, bugs and complications. If I want my code to work reliably I avoid it. That is possible here without adding much boilerplate code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

distinctBase doesn't require an import dependency now that it's a magic.

proc distinctBase(T: typedesc): typedesc {.magic: "TypeTrait".}

Having 2 different ways to do implement the safe feature is not good (if it's broken, please show an example where it breaks)

Copy link
Contributor Author

@krux02 krux02 Mar 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timotheecour Just an example: #12582
That is a very typical bug that you hit just because you are using typedesc. If you want to avoid accidentally hitting compiler bugs like that, the only advice that works is, don't use typedesc.

But here are more typedesc related problems. Of cours the reported problems are only showing the tip of the iceberg. I encountered several more problems of typedesc that I did not report.

Copy link
Member

@timotheecour timotheecour Apr 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not aware of a bug involving distinctBase specifically, if you find one, please file an issue.

## convert and distinct value to its base type.
let baseTyp = getTypeImpl(arg)[0]
result = newCall(baseTyp, arg)

proc newLit*[T : distinct](arg: T): NimNode {.compileTime, since: (1,1).} =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since: (1,3)

result = newCall(bindSym"T", newLit(undistinct(arg)))

proc nestList*(op: NimNode; pack: NimNode): NimNode {.compileTime.} =
## Nests the list `pack` into a tree of call expressions:
## ``[a, b, c]`` is transformed into ``op(a, op(c, d))``.
Expand Down
19 changes: 19 additions & 0 deletions tests/macros/tnewlit.nim
Original file line number Diff line number Diff line change
Expand Up @@ -192,3 +192,22 @@ block:
let x = test_newLit_object_ref_alias()
doAssert $(x[]) == "(x: 10)"


type
Rune = distinct int32
Foo = object
a: Rune

proc `$`(arg: Rune): string = $(int32(arg))

macro test_newLit_distinct(): untyped =
newLit(Rune(123))

macro test_newLit_distinct_in_object(): untyped =
newLit(Foo(a: Rune(456)))

block:
let x1 = test_newLit_distinct()
let x2 = test_newLit_distinct_in_object()
doAssert $x1 == "123"
doAssert $x2 == "(a: 456)"