Implement custom pragmas #6987

cooldome · 2017-12-29T08:57:05Z

One more implementation of superb feature that we can't live without ;)
Previously attempted by yglukhov: #6070 and #6138

I hope this time we will get it done. I have addressed review comments in #6138

yglukhov · 2017-12-29T09:35:35Z

I'm really happy there's not only me who wants to implement it :D. However, there was a problem I hit in my impl. The problem is the custom pragmas are stripped away from object field definitions on the semantic phase, so you can't get those pragmas later with getType*. That's why I've suspended my impl. In order to get this right we have to leave those pragmas in the types, and fix the code that expects stripped pragmas in a few places.

cooldome · 2017-12-29T12:01:57Z

Hi yglukhov, could you please describe the problem in more details (example?). Yes, pragmas are not in types but they are in nodes. They can be retrieved with getTypeInst at least, so I don't see a problem here.

cooldome · 2017-12-29T12:04:36Z

yglukhov, I have extra test compared to your implementation for inner fields. getCustomPragmaVal(s.field.c, serializationKey)

Is this what you mean?

yglukhov · 2017-12-29T17:04:03Z

@cooldome

in more details (example?)

import macros
template serializationKey(k: string) {.pragma.}

type Foo = object
    b1 {.serializationKey: "hi".}: int64

macro test(a: typed): untyped =
    echo treeRepr(getTypeImpl(a)[1].getTypeImpl())

test(Foo)

Outputs:

ObjectTy
  Empty
  Empty
  RecList
    IdentDefs
      Sym "b1"
      Sym "int64"
      Empty

I can't see a way to get the pragmas from within the macro. Or am i missing anything?

cooldome · 2017-12-29T19:45:40Z

@yglukhov
Hi Yuriy,
In the following way it works:

import macros
template serializationKey(k: string) {.pragma.}

type
  SubFoo = object
    c1 {.serializationKey: "myc1".}: int64
  Foo = object
    f1: SubFoo
    b1 {.serializationKey: "myb1".}: int64

macro test(a: typed): untyped =
  let asym = a.symbol.getImpl
  echo asym.treeRepr
  echo "---"
  let f1field = asym[2][2].findChild(it[0].kind == nnkIdent and $it[0] == "f1")
  echo f1field[1].symbol.getImpl.treeRepr

test(Foo)

outputs:

TypeDef
  Sym "Foo"
  Empty
  ObjectTy
    Empty
    Empty
    RecList
      IdentDefs
        Ident ident"f1"
        Sym "SubFoo"
        Empty
      IdentDefs
        PragmaExpr
          Ident ident"b1"
          Pragma
            ExprColonExpr
              Sym "serializationKey"
              StrLit myb1
        Sym "int64"
        Empty
---
TypeDef
  Sym "SubFoo"
  Empty
  ObjectTy
    Empty
    Empty
    RecList
      IdentDefs
        PragmaExpr
          Ident ident"c1"
          Pragma
            ExprColonExpr
              Sym "serializationKey"
              StrLit myc1
        Sym "int64"
        Empty

You can access type def pragmas using n.symbol.getImpl() and also recurse into the fields implementations if required. It does the job, however I understand the furstration as likely all of existing serialization code is using getType() and getTypeImpl() at the moment.

I have considered patching mapTypeToAstX in the compiler to produce pragmas for getTypeImpl(), but I don't think it is a good idea as it will be breaking change and every user of getTypeImpl() will be effected.

yglukhov · 2017-12-29T20:32:27Z

You can access type def pragmas...

That is an option I could bear with.

it will be breaking change and every user of getTypeImpl() will be effected.

Now regarding the getTypeImpl breakage, I suppose it will break only for those types with custom pragmas, which currently do not exist, so I would not call that a breakage at all. Besides, Nim is getting close to 1.0 so it is the perfect time to correct things now. Breakage after 1.0 will be a lot harder to advocate.

Araq · 2017-12-29T23:06:08Z

compiler/ast.nim

    mNHint, mNWarning, mNError,
    mInstantiationInfo, mGetTypeInfo, mNGenSym,
    mNimvm, mIntDefine, mStrDefine, mRunnableExamples,
+    mCustomPragma,


oops, removed

Araq · 2017-12-29T23:07:29Z

compiler/ast.nim

    sfOverriden,      # proc is overriden
-    sfGenSym          # symbol is 'gensym'ed; do not add to symbol table
+    sfGenSym,         # symbol is 'gensym'ed; do not add to symbol table,
+    sfCustomPragma    # symbol is custom pragma template


I would introduce an alias to an existing sfFlag here to keep the number of flags below 32.

Changed to the alias of existing sfFlag. As alternative I can make a new SymKind, I know it is a bit of work, but if you find it to be better solution I can do that.

No new SymKind. Why? Because the symbol kinds should reflect the concrete syntax, the syntax is template foo {.pragma.} and so it should be an skTemplate with an sfPragma flag.

cooldome · 2017-12-30T17:29:28Z

Hi Araq, I need this feature for my project in the next two weeks.
Tell me what is required to make this feature production ready for the upcoming release, I'll do my best to meet the aggressive timeline.

Help is very much appreciated.

Araq · 2017-12-31T08:31:30Z

compiler/pragmas.nim

+  elif n.kind == nkExprColonExpr and n[1].kind == nkBracket:
+    # pragma: [arg1, arg2] -> pragma(arg1, arg2)
+    result.add n[0]
+    result.sons &= n[1].sons


I don't like direct 'sons' accesses but have no better solution right now.

Now I have: Write a helper proc in ast.nim that does the required transformations.

Araq · 2017-12-31T08:36:11Z

The feature's implementation looks fine but it needs documentation. Please update the manual.

cooldome · 2018-01-02T09:55:39Z

Thanks Araq, I have updated the manual and added ast helpers for sons manipulation

GULPF · 2018-01-02T11:08:00Z

doc/manual/pragmas.txt

+-------------------
+It is possible to define custom typed pragmas. Custom pragmas do not effect 
+code generation directly, but their presence can be detected by macros.
+Custom pragmas are defined using ``pragma`` keyword:


Nitpick, but pragma isn't a keyword? Should be: "/.../ defined using the pragma pragma".

Great to see this feature implemented!

Make sense, updated the doc

Araq · 2018-01-03T11:29:14Z

doc/manual/pragmas.txt

+In this example custom pragmas are used to describe how Nim objects are 
+mapped to the schema of the relational database. Custom pragmas can have 
+zero, one or more arguments. In order to pass multiple arguments enclose
+them into brackets `[]` as for the rest of the pragmas. If you need to pass


This needs to be done with () instead, other pragmas already use tuple syntax and it makes more sense than the array syntax.

I have done it in the latest commit, however it required a minor parser change to make pass by name work. I was getting nkAsgn nodes instead of nkExprEqExpr. I hope your are ok with it

Araq · 2018-01-03T11:29:39Z

doc/manual/pragmas.txt

+mapped to the schema of the relational database. Custom pragmas can have 
+zero, one or more arguments. In order to pass multiple arguments enclose
+them into brackets `[]` as for the rest of the pragmas. If you need to pass
+a single argument which is a const array, double brackets will be required.


See? That's not good.

I like () even more, I thought I have to comply with existing pragmas syntax. Good we have changed it to ()

Araq · 2018-01-03T11:32:01Z

compiler/pragmas.nim

+
+  if n.kind == nkIdent:
+    result = result[0]
+  elif n.kind == nkExprColonExpr and n[1].kind == nkBracket:


n[1].kind == nkPar

Araq · 2018-01-03T13:08:20Z

doc/manual/pragmas.txt

+
+  type
+    User {.dbTable: ("users", tblspace).} = object
+      id {.dbKey: (primary_key = true).}: int


Ugh, no. Tuple constructors use ':'!

I kind of disagree. Yes, tuple constructors are using ':', but it is not a tuple. It is more of call expression and I am passing the argument by name.
IMO, tuple constructors don't fit as they can't have a mix of unnamed and named arguments, while proc/template calls can.

Let me know if got everything wrong.

It's clearly a tuple that is passed to the template here. That said, I think pragmas should just allow ordinary call syntax without the colons, importc"foo" that would solve this issue. I always wanted to allow this.

All done now, it well worth a double shot of wiskey at FOSDEM

Araq · 2018-01-07T13:05:44Z

compiler/pragmas.nim

      var y = a.sons[1]
-      if x.kind == nkExprColonExpr: x = x.sons[1]
-      if y.kind == nkExprColonExpr: y = y.sons[1]
+      if x.kind in nkPragmaCallKinds: x = x.sons[1]


Very good, but nkCall is not guaranteed to have 2 or more children, so you need a x.len check here too where there was none required before.

Thanks for quick review, added len checks in the latest commit. Let's see how tests will go.

Araq · 2018-01-07T13:08:10Z

Superb work. I love it.

cooldome · 2018-01-07T13:39:39Z

Agree, len checks were required. Added them in the latest submit

data-man · 2018-01-07T18:53:15Z

Many repetitions:

if n.kind notin nkPragmaCallKinds or n.len != 2:

Maybe to write a separate proc for this check?

cooldome · 2018-01-07T18:57:15Z

I don't see a problem in repetition, it generally follows compiler code convention.
Ultimately, it is Araq's call. If he wants these checks as a separate function I can do it in 20 minutes

cooldome · 2018-01-08T09:11:51Z

ready for the next review

Araq · 2018-01-08T09:17:56Z

doc/manual/pragmas.txt

+
+More examples with custom pragmas:
+- Better serialization/deserialization control:
+.. code-block:: nim


Newline before this 'code-block'.

Araq · 2018-01-08T09:18:11Z

doc/manual/pragmas.txt

+    c {.serializationKey: "_c".}: string
+
+- Adopting type for gui inspector in a game engine:
+.. code-block:: nim


Newline before this 'code-block'.

Araq · 2018-01-08T09:18:56Z

doc/manual/pragmas.txt

+you cannot do yourself by walking AST object representation.
+
+More examples with custom pragmas:
+- Better serialization/deserialization control:


This should be a heading.

Araq · 2018-01-08T09:19:07Z

doc/manual/pragmas.txt

+    b {.defaultDeserialize: 5.}: int
+    c {.serializationKey: "_c".}: string
+
+- Adopting type for gui inspector in a game engine:


This should be a heading.

Araq · 2018-01-08T09:19:28Z

doc/manual/pragmas.txt

+  template dbIgnore {.pragma.}
+
+Consider stylized example of possible Object Relation Mapping (ORM) implementation:
+.. code-block:: nim


Newline before this 'code-block'.

Araq · 2018-01-08T09:20:09Z

doc/manual/pragmas.txt

 used.
+
+
+User custom pragmas 


The name of this feature shall be "Custom annotations"

Araq · 2018-01-08T09:20:45Z

Travis is red because of the illformed RST documentation.

dom96 · 2018-01-08T23:05:01Z

doc/manual/pragmas.txt

+------------------
+It is possible to define custom typed pragmas. Custom pragmas do not effect 
+code generation directly, but their presence can be detected by macros.
+Custom pragmas are defined using pragma ``pragma``:


When I read this first it seemed as if this "pragma" can only be named pragma, please explain that any arbitrary identifier is possible.

dom96 · 2018-01-08T23:07:04Z

doc/manual/pragmas.txt

+    type MyComponent = object
+      position {.editable, animatable.}: Vector3
+      alpha {.editRange: [0.0..1.0], animatable.}: float32
+


An example implementation for one of these pragmas would be nice

I am not really game or gui developer so I can't implement it in realistic way, I have taken this example from yglukhov first proposal as I thought the more examples the better. I can leave it as it is or remove it.

dom96 · 2018-01-08T23:08:06Z

lib/core/macros.nim

+  ##
+  ## .. code-block:: nim
+  ##   template myAttr() = discard
+  ##   type MyObj = object


convention is that type should be on its own line.

dom96 · 2018-01-08T23:09:26Z

lib/core/macros.nim

+  ##   type MyObj = object
+  ##     myField {.serializationKey: "mf".}: int
+  ##   var o: MyObj
+  ##   assert(o.myField.customPragmaNode(serializationKey) == "mf")


This example uses a private proc.

dom96 · 2018-01-08T23:10:29Z

lib/core/macros.nim

+      return newLit(true)
+  return newLit(false)
+
+macro getCustomPragmaVal*(n: typed, cp: typed{nkSym}): untyped =


I think a better API would be getCustomPragma and possibly an additional getValue procedure.

Hrm, so you don't think it's useful to be able to retrieve the CustomPragma itself?

I don't think getCustomPragma can be a macro, what it is going to return? Invalid NimNode.
However, getCustomPragma can be a compile time proc that is used inside other user macros to do something with pragma node. Actually, I have such proc already it is called customPragmaNode, but it is not exported.

Should I rename it and make it public?

Should I rename it and make it public?

We can do that later, I'm merging this now, I don't want to keep reviewing it.

dom96 · 2018-01-08T23:10:53Z

lib/core/macros.nim

+  ##
+  ## .. code-block:: nim
+  ##   template serializationKey(key: string) = discard
+  ##   type MyObj = object


same as above.

dom96 · 2018-01-08T23:12:38Z

compiler/pragmas.nim

          localError(it.info, "'experimental' pragma only valid as toplevel statement")
      of wThis:
-        if it.kind == nkExprColonExpr:
+        if it.kind in nkPragmaCallKinds and it.len == 2:


I agree with @data-man here. You're using this expression many times.

But what's possibly error prone is that you're using the negation of this too (just above on line 988, and probably in other places too). It would be far clearer if this expression was in some nice proc and you negated it when necessary.

That's a bit unfair since the original code had the same issues. I planned to clean this one up after the merge. But ok, since other stuff needs to be reworked too, no big issue.

I don't think it was as severe previously: only the kind was checked, no?

I disagree. Introduction of a new function for every pair of checks will turn compiler's code into hell, because it will contain 30000 one line helper procs that everybody will have to learn before reading compilers code.

It is pointless: it will not save a single line of code, because if statements can't be removed as error messages are different for different pragma.

I'm going to let @Araq decide. If this was my code though I wouldn't accept it. This is a common check.

Araq · 2018-01-09T14:25:02Z

Unrelated test failure. Merging.

cooldome added 4 commits December 29, 2017 11:50

Implement custom pragmas

2dee111

remove not used line

54e0ba2

Add missing file

870249d

Add comma

4d92d7f

yglukhov mentioned this pull request Dec 29, 2017

attr feature #6070

Closed

Araq reviewed Dec 29, 2017

View reviewed changes

Araq added the Next Release label Dec 29, 2017

cooldome added 2 commits December 30, 2017 10:09

Feedback incorporated

ff3b1d7

Add support for multiple arguments

c35aec1

Araq reviewed Dec 31, 2017

View reviewed changes

cooldome added 4 commits January 1, 2018 15:38

Add node sons helpers to ast.nim

7a5c8ea

Progress on pragmas manual

0af5984

Manual progress

8c4f6ba

Documentation for custom pragmas is ready

5d0466f

GULPF reviewed Jan 2, 2018

View reviewed changes

Pragma docs: pragma is not keyword

ffb7e10

Araq reviewed Jan 3, 2018

View reviewed changes

Custom pragma arguments are enclosed in parenthesis

cece9f2

Araq reviewed Jan 3, 2018

View reviewed changes

cooldome added 4 commits January 6, 2018 23:52

Support pragma args as call syntax

a079a43

Remove not used copySonsOf

ef5eb0a

Remove comma

23e3724

remove addSonsOf, fix tests

2ec26ef

Araq reviewed Jan 7, 2018

View reviewed changes

add nkCall len checks in pragmas

68af801

cooldome and others added 5 commits January 7, 2018 13:47

Add a note to the changelog.md

7b4ab94

Manual merge of changelog

2c9ffdb

manual merge 2

bcc7297

Merge branch 'devel' into custom_pragma

1b30560

Bug fix for pragma line

8169e3e

Araq reviewed Jan 8, 2018

View reviewed changes

doc/manual/pragmas.txt Outdated

used.

User custom pragmas

Copy link

Member

Araq Jan 8, 2018

Choose a reason for hiding this comment

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

The name of this feature shall be "Custom annotations"

Fix the doc

ea8c712

dom96 requested changes Jan 8, 2018

View reviewed changes

cooldome added 2 commits January 9, 2018 10:25

Improved examples

1e7bd65

fix the doc

1ef8b89

Araq merged commit 2c9e56a into nim-lang:devel Jan 9, 2018

Uh oh!

Implement custom pragmas #6987

Implement custom pragmas #6987

Uh oh!

Conversation

cooldome commented Dec 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yglukhov commented Dec 29, 2017

Uh oh!

cooldome commented Dec 29, 2017

Uh oh!

cooldome commented Dec 29, 2017

Uh oh!

yglukhov commented Dec 29, 2017

Uh oh!

cooldome commented Dec 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yglukhov commented Dec 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cooldome commented Dec 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Araq commented Dec 31, 2017

Uh oh!

cooldome commented Jan 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cooldome Jan 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Araq Jan 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Araq commented Jan 7, 2018

Uh oh!

cooldome commented Jan 7, 2018

Uh oh!

data-man commented Jan 7, 2018

Uh oh!

cooldome commented Dec 29, 2017 •

edited

Loading

cooldome commented Dec 29, 2017 •

edited

Loading

yglukhov commented Dec 29, 2017 •

edited

Loading

cooldome Jan 3, 2018 •

edited

Loading

Araq Jan 3, 2018 •

edited

Loading

cooldome commented Jan 7, 2018 •

edited

Loading