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

Pass typedesc as NimNode to macros #148

Closed
krux02 opened this issue May 8, 2019 · 15 comments

Comments

Projects
None yet
4 participants
@krux02
Copy link
Collaborator

commented May 8, 2019

I continuously try to fight surprise special cases in the Nim
compiler, as they usually lead to fewer bugs in the compilar as well as fewer bugs in user code. One of the special cases that continues to bother me is the
treatment of a typedesc as an argument for a macro. In Nim all arguments
for a macro are passed as a NimNode except for the the argument of
type typedesc, which behaves differently.

for example, changing macro foo(arg: int) to macro foo(arg: typed) does not have any affect on the type of the symbol arg within the macro. This is true for every type that is not tagged explicitly with static in the argument list. But if you pass in a typedesc to a macro, this rule is violated, and all of a sudden the type of the arguments changes from NimNode to some typedescriptor.

import macros

macro foo(a: int, b: string, c: float): untyped =
  echo typeof(a) # NimNode
  echo typeof(b) # NimNode
  echo typeof(c) # NimNode

foo(1,"string", 123.456)

# use

macro bar(a,b,c: typedesc): untyped =
  echo typeof(a) # None, should probably be typedesc[int]
  echo typeof(b) # None, should probably be typedesc[string]
  echo typeof(c) # None, should probably be typedesc[float]

bar(int, string, float)

To make sure that this breaking change does not cause too much harm, I would like to collect all use cases where a macro might take a typedesc argument and see how changing it to a NimNode within the macro body would be affected by this change.

The first use case for a typedesc type in the list of arguments is to disambiguate overloads:

macro baz(a: typedesc[int], b: untyped): untyped =
  echo "baz int"

macro baz(a: typedesc[string], b: untyped): untyped =
  echo "baz string"

macro baz(a: typedesc[float], b: untyped): untyped =
  echo "baz float"

baz(int, a+b)    # output: baz int
baz(string, a+b) # output: baz string
baz(float, a+b)  # output: baz float

The argument a isn't used in the macro at all. It is used for symbol disambiguation only. Therefore it doesn't matter if the symbol a is of type typedesc or
NimNode it would not have any affect. This code would be unaffected.

The second use case is a macro that has several types as an argument, and then it does some logic to select one of them. This result type can then be used in a type expression:

macro selectType(context: typed, a,b,c: typedesc): typedesc =
  # some random logic here
  case context.repr.len mod 3:
  of 0:
    result = a
  of 1:
    result = b
  else:
    result = c

var myValue = 1
var myOtherValue : selectType(myValue, int, float, string)
echo myOtherValue

Since both, argument and result would be changed at the same time
from typedesc to NimNode, the code would remain to work
without any change.

The only direct API that the typedesc provides is as far as I can tell to print the name. So let's see how that code would be affected:

macro baz(arg: typedesc): untyped =
  echo arg

baz(seq[int]) # output: None

Well I guess that never worked in the first place, so we don't need to
worry about breaking something here. But usually in Nim it pays off
to dig deeper and to apply random calls of getImpl, getTypeImpl or
getTypeInst to the problem until the expected result appears. In this
case getTypeInst does the job.

macro baz2(arg: typedesc): untyped =
  echo arg.getTypeInst.repr

baz2(seq[int]) # output: typeDesc[seq[int]]

So with getTypeInst, it works. But getTypeInst is designed to work
on NimNode primarily. So when arg changes from typedesc to
NimNode the exact same code remains to work with the exact same
result. (Notice the change from typedesc to typed int he argument to make arg a NimNode:

macro baz3(arg: typed): untyped =
  echo arg.getTypeInst.repr

baz3(seq[int]) # output: typeDesc[seq[int]]

So as far as I can tell, there isn't any code that would be broken in a horrible way.

So now I would like to take a look at code that would be required to be changed and see how it could be changed it a way that it works in both versions of the compiler old and new.

Assigning a type expression to the result of a macro wont't compile anymore:

# this won't be legal anymore, works only in old
macro baz4(): typedesc =
  result = int 

# this would work only in new
macro baz4(): typedesc =
  result = bindSym"int"

# this would work in both old and new
macro baz4(): untyped =
  result = bindSym"int"

Passing a typedesc to a macro is only a NimNode in the new version of the compiler. So a version that works on both old and new and it needs the type of the argument to be consistently either a NimNode or a typedesc has to pick one of the following options. But it should be mentioned that this only applies to cases wherer the difference between NimNode and typedesc truly matters, as in the examples above there were examples given on how the difference does not matter.

# consistently a typedesc in both now and old
macro baz5(arg: static[typedesc]): untyped =
  arg

# consistently a NimNode in both new and old
macro baz5(arg: typed): untyped =
  arg
@krux02

This comment has been minimized.

Copy link
Collaborator Author

commented May 8, 2019

@zah Since you introduced the special treatment of typedesc in macros, I would like to know what you think about this change? I guess you had some use cases in mind when you designed this behavior, would they be broken?

@Araq

This comment has been minimized.

Copy link
Member

commented May 8, 2019

In Nim all arguments for a macro are passed as a NimNode except for the the argument of type typedesc, which behaves differently.

static T is also special.

@krux02

This comment has been minimized.

Copy link
Collaborator Author

commented May 8, 2019

static T is also special.

Well I mention static T later in this RFC. But that part is not addressed in the RFC.

@dumjyl

This comment has been minimized.

Copy link

commented May 8, 2019

+1 Ran into this a couple days ago. It was confusing behavior for me. If nothing else comes of this rfc, docs should mention somewhere how typedesc get treated in macros (at least I couldn't find it). Now time to go update my code to use typedesc.getTypeInst instead of untyped.

@Araq

This comment has been minimized.

Copy link
Member

commented May 8, 2019

But that part is not addressed in the RFC.

Ok, but that's my only gripe with this RFC, it doesn't go far enough. The special casing for static T should also go away. :-)

@zah

This comment has been minimized.

Copy link
Member

commented May 8, 2019

A bit of history for how we arrived at the current state:

typedesc parameters in macros were introduced a very long time ago, when Nim had a different VM implementation called eval.nim. This older VM had some explicit support for storing type symbols inside the internal VM values and the design had the following goals:

  1. It should be possible to create containers and records holding typedesc values (e.g. seq[typedesc], tuple[foo: int, bar: typedesc], etc), to store them in variables, to pass them to functions and so on.

    The current line of thinking is that the same benefits can be achieved by representing the types as NimNodes with only minor inconveniences - for example, in the old design it was possible to have code like this:

  const myTypeList = [int, float, string]
  var foo: myTypeList[1]
  1. It should be possible to use any typetrait magic both outside of macros over any type symbol (e.g. sizeof(int)) and inside macros over typedesc parameters (e.g. sizeof(x)). I'm using sizeof as an example here, even though it's not implemented as a type trait at the moment.

    The current line of thinking is that the typetraits module should be deprecated in favor of a library built on top of getTypeImpl. Please note that some work is required to reach the original vision of having these helpers usable both outside and inside macros. Let's imagine a hypothetical type trait called myArity. It can be defined like this:

import
  macros

macro myArity(t: typedesc): untyped =
  let x = t.getTypeImpl[1].getImpl
  return newLit(x.len)

type
  MyProcType = proc (x, y, z: int)

static:
  echo MyProcType.myArity

macro printArity(procType: typedesc): untyped =
  let arity = procType.myArity
  echo arity

printArity MyProcType

The expected compile-time output here is 3 3, but at the moment Nim will fail to produce that. It will expand the myArity macro too early inside the printArity macro. We can try to counter this effect by using getAst(myArity(procType)), but then the retuned expression won't be a regular int value. We can try to further improve the myArity macro by changing the return type to an int, but this will lead us to more problems.

I've had various plans for how to solve this. The nicest solution I can think of is to map the typedesc parameters to a distinct NimNode type and then to introduce some logic in the compiler that will rewrite any expressions within macros that feature unresolved parameters into the equivalent of a delayed getAst call.

Please note that a reasonable way to define a "type trait" may be through a simple template like this:

template ElementType(T: typedesc): typedesc =
  type(default(T)[0])

macro printElementType(T: typedesc) =
  echo T.ElementType

printElementType seq[int]

It would be nice if these work as well.

  1. It should be possible to build or select a type inside a macro and then return it.

You can get creative here - Nim should support defining record types with varying set of fields, attaching type-bound operators to the returned types, doing some of the work in helper procs and so on.


For static T params, after much consideration, I support the following breaking change that was requested by Araq:

  1. static Foo in macros is typed as a NimNode (with some caveats, see point 3).
  2. The AST tree matches precisely what was written at the call-site
  3. There is a new magic called staticValue (similar to getType). It returns the well-typed static value the input expression evaluates to. This will take into account the static T type of the parameter and will be resolved to the same type.

This is a breaking change that is arguably less convenient than the current behaviour, but it has the nice property that you can inspect the precise expression that was passed to the macro. This may be important for some DSL and I think it's a property that should be enforced for the typedesc parameters as well.

@krux02

This comment has been minimized.

Copy link
Collaborator Author

commented May 9, 2019

Thank you for clearing things up, it is nice to know about some of the original design goal. I think there are a few problems with the original design goal. I had the same problem, I wanted to access traits of types, but I wanted my own traits, not something that is available from a typetraits module. For example I introduced my own type traits to map Nim types to GLSL types, and I used templates for it:

template glslType(arg: typedesc[Vec4f]): string = "vec4"
template glslType(arg: typedesc[Vec4i]): string = "ivec4"

const x = glslType(Vec4f) # "vec4"
const y = glslType(typeof(myExpr)) # "ivec4"

This pattern allows me attach arbitrary traits to types. But it is not available from within a macro. In macros I usually don't pass in the types as typedesc arguments, I get them by calling getTypeInst. The type inst expression I can then use to generate expressions that evaluate to the above calls:

# the explicit call to convert the type to a typedesc is necessary, but it is probably a bug that it isn't implicit.
macro foobar(arg: typed): string =
  result = newCall(bindSym"glslType", newCall(bindSym"typedesc",getTypeInst(arg)))

This does not allow me to access the type trait value at execution time of the macro, but it does allow me to generate calls that access the type traits. When I need the macro value at macro execution time, I just go one level deeper and generate another macro call from the macro:

macro foobar_inner(arg: typed): untyped =
  echo "foobar_inner: ", arg.lispRepr

macro foobar(arg: typed): untyped =
  result = newCall(bindSym"foobar_inner", newCall(bindSym"glslType", newCall(bindSym"typedesc",getTypeInst(arg))))

var v1: Vec4f
var v2: Vec4i

foobar(v1) # foobar_inner: (StrLit "vec4")
foobar(v2) # foobar_inner: (StrLit "ivec4")

Second, part is. Typetraits API that is in the standard library, for example size or alignment, can easiliy have a second API within macros so that those values can be accessed from NimNodes. Which is already true for NimNode.

@zah

This comment has been minimized.

Copy link
Member

commented May 9, 2019

The inner-macro solution still uses delayed expansion, which makes it a bit more troublesome to work with. The eager expansion provided by getAst is a nicer paradigm. Bonus points if we also make it work inside compileTime helper procs.

Otherwise, I agree that the "user-defined traits" through simple templates ought to be supported. My own message covered that. Did you understand the solution I'm proposing for automatically detecting and lifting the expressions where getAst should be inserted?

@Araq

This comment has been minimized.

Copy link
Member

commented May 9, 2019

The expected compile-time output here is 3 3, but at the moment Nim will fail to produce that. It will expand the myArity macro too early inside the printArity macro.

But if it were NimNode internally in printArity it would call myArity properly and produce the 3, automatically, no special casing required.

@zah

This comment has been minimized.

Copy link
Member

commented May 9, 2019

@Araq, I think you are mistaken. If you don't use getAst at the moment, the myArity macro will be expanded very early while the body of the printArityMacro is being processed - even before the VM instructions are generated. Since the expansion depends on the actual value being passed to the macro, there is no way you can produce correct code at that time. You need getAst to delay the expansion until the typedesc parameter has a known concrete value.

@Araq

This comment has been minimized.

Copy link
Member

commented May 9, 2019

Ah, yeah you need to use a .compileTime helper proc.

@zah

This comment has been minimized.

Copy link
Member

commented May 9, 2019

Yes, you can have your code DRY at the moment by constructing an intricate web of macros, compileTime procs and code using getAst explicitly, but all of this can be integrated much better.

@Araq

This comment has been minimized.

Copy link
Member

commented May 9, 2019

But now the problem shifted to "how to call macros from within macros more elegantly" which is a more general problem to solve and not typedesc-specific. Definitely a win IMO.

@krux02

This comment has been minimized.

Copy link
Collaborator Author

commented May 9, 2019

@zah, yes I do understand where you would use the getAst:

macro foobar2(arg: typed): untyped =
  let typeArg = newCall(bindSym"typedesc",getTypeInst(arg))
  let tmp =  getAst(glslType(typeArg))
  echo tmp.treeRepr

var v1: Vec4f
var v2: Vec4i

foobar2(v1) # StrLit "vec4"
foobar2(v2) # StrLit "ivec4"

currently this does not compile with the following error:

Error: ambiguous symbol in 'getAst' context: glslType(typeArg)

I know the inner-marco solution is more troublesome to work with, not just a bit. Especially with more than just one attribute call, you have to construct an AST that exists purely to be typechecked, after it is passed to the inner macro, it is deconstructed again. But it is a solution that works today.

@krux02 krux02 changed the title pass typedesc as NimNode to macros Pass typedesc as NimNode to macros May 21, 2019

@krux02

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 5, 2019

this has been merged.

@krux02 krux02 closed this Jun 5, 2019

sthenic added a commit to sthenic/NimYAML that referenced this issue Jun 11, 2019

Fixes for Nim v0.20.0
The breaking changes affecting this library are:

* Macro arguments of type typedesc are now passed to the macro as
  NimNode like every other type except static. Use typed for a behavior
  that is identical in new and old Nim. See the RFC Pass typedesc as
  NimNode to macros for more details.
  (nim-lang/RFCs#148)

* case object branch transitions via system.reset are deprecated.
  Compile your code with -d:nimOldCaseObjects for a transition period.

  The solution here is to compile with '-d:nimOldCaseObjects' for the
  time being. I didn't understand how to modify the code to adhere to
  the new way of initializing/changing variant objects.

* The compiler now enforces range checking which exposed a bug in the
  fieldCount proc. For variant objects where one of the branches is
  given on one line, e.g.

    ...
    of akDog: barkometer: int
    ...

  the field wasn't counted and the total field count was not accurate.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.