deprecate macros.expectLen, expectKind, expectMinLen in favor of something generic or doAssert

timotheecour commented 4 years ago

I don't think the proliferation of expectX (eg: expectLen, expectKind, expectMinLen, expectIdent (https://github.com/nim-lang/Nim/pull/12778)) makes sense in a language like nim that has macros and can do this generically.

this approach doesn't scale, there'll always be other conditions to check, it bloats macros.nim
expectLen(n, 2) doesn't even show the value n.len in the error msg in case of failure (granted, this is easy to fix)
none of these expectX procs allow passing an optional msg (eg with values)
expectKind(n, nnkSym) hides the fact that n.kind is accessed, eg where searching for accessors of a field kind
there's no reason to differentiate between VM (doAssert) and CT (expectLen etc), doAssert can handle both
it's "always on" whereas with assert/doAssert you get to customize whether it's always on or not

proposal 1

replace expectX by a simple doAssert; it shows all the context, and you can choose whether to use assert or doAssert

doAssert n.len == 2 # shows exactly same information as `expectLen(n, 2)`, but clearer
doAssert n.len == 2, $n.len # shows more information than `expectLen(n, 2)`
doAssert n.len == 2, $(n.len, n.kind) # shows whatever information deemed relelvant

noted, expectKind (unlike expectLen) does show the input kind, but we can do:

expectKind(n, n.kind)
doAssert(n.kind == nnkSym, $n.kind)

proposal 2

if assert/doAssert is not enough (say because it doesn't automatically show input value unless you specify it eg via doAssert(n.kind == nnkSym, $n.kind)): we can automatically show the relevant input arg deconstructed from AST (eg n.kind == nnkSym) in same way as done in unittest.expect; either we can reuse unittest.expect, or we can introduce a new "simplified" version of unittest.expect that just also shows the values of arguments in simple cases (eg infix), but doesn't add dependency on std/unittest, eg: doExpect(n.kind == nnkSym)

krux02 commented 4 years ago

Sorry, I use the expectX family of procs regularly. Every macro that I write uses them. expectX is easy to understand both from the implementation point of view as well as the usage side. It is free of bugs. It generates nice error messages pointing to the ast where it is wrong, somthing that doAssert doesn't do.

timotheecour commented 4 years ago

It generates nice error messages pointing to the ast where it is wrong, somthing that doAssert doesn't do.

see example:

expectLen shows result = newNimNode(nnkIntLit) which is super misleading / not helpful since it points to newLit 1 instead of result
it doesn't show the actual len nor can it show any additional runtime context because it doesnt' take a msg argument

nim c --hint:source:on main.nim

  import macros
  macro fun(n1, n2) =
    result = newStmtList()
    result.add quote do:
      [1,2]
    expectLen result, 3

  macro baz(): untyped = newCall("fun", newLit 1, newLit 2)
  baz()

with expectLen:

stack trace: (most recent call last)
/Users/timothee/git_clone/nim/timn/tests/nim/all/t10194.nim(99, 15) fun
/Users/timothee/git_clone/nim/Nim_devel/lib/core/macros.nim(621, 25) expectLen
/Users/timothee/git_clone/nim/timn/tests/nim/all/t10194.nim(102, 6) template/generic instantiation of `baz` from here
/Users/timothee/git_clone/nim/Nim_devel/lib/core/macros.nim(660, 22) template/generic instantiation of `fun` from here
/Users/timothee/git_clone/nim/Nim_devel/lib/core/macros.nim(671, 22) Error: macro expects a node with 3 children
    result = newNimNode(nnkIntLit)
                       ^

by contrast, doAssert shows the actual failing expression (result.len == 3), along with any extra runtime information passed along in msg, eg if you're lazy $result.len, or $(result.len, result.repr) or if you want formatted messages $(len: result.len, repr: result.repr); here's with simply $result.len:

stack trace: (most recent call last)
/Users/timothee/git_clone/nim/timn/tests/nim/all/t10194.nim(99, 11) fun
/Users/timothee/git_clone/nim/Nim_devel/lib/system/assertions.nim(27, 20) failedAssertImpl
/Users/timothee/git_clone/nim/Nim_devel/lib/system/assertions.nim(20, 11) raiseAssert
/Users/timothee/git_clone/nim/Nim_devel/lib/system/fatal.nim(55, 5) sysFatal
/Users/timothee/git_clone/nim/timn/tests/nim/all/t10194.nim(102, 6) template/generic instantiation of `baz` from here
/Users/timothee/git_clone/nim/Nim_devel/lib/core/macros.nim(660, 22) template/generic instantiation of `fun` from here
/Users/timothee/git_clone/nim/Nim_devel/lib/system/fatal.nim(55, 5) Error: unhandled exception: /Users/timothee/git_clone/nim/timn/tests/nim/all/t10194.nim(99, 14) `result.len == 3` 1 [AssertionError]
      raise e
      ^

the doAssert error msg isn't missing any relevant information (at least none that expectLen shows)

krux02 commented 4 years ago

bad example. expectX are procs you use to check for valid inputs on the argument ast, not on the ast that you just constructed.

here is a list of examples where the expect procs are used in the project I have currently in my working directory.

openmesh/src/openmesh.nim:91:      identDefs.expectKind nnkIdentDefs
openmesh/src/openmesh.nim:106:      identDefs.expectKind nnkIdentDefs
openmesh/src/openmesh.nim:153:  result.expectKind nnkTypeSection
openmesh/src/openmesh.nim:158:  typeDef.expectKind nnkTypeDef
openmesh/src/openmesh.nim:160:  typeDef[1].expectKind nnkEmpty
openmesh/src/openmesh.nim:161:  typeDef[2].expectKind nnkObjectTy
openmesh/src/openmesh.nim:162:  typeDef[2][0].expectKind nnkEmpty
openmesh/src/openmesh.nim:163:  typeDef[2][1].expectKind nnkEmpty
openmesh/src/openmesh.nim:164:  typeDef[2][2].expectKind nnkRecList
openmesh/src/openmesh.nim:169:    identDefs.expectKind nnkIdentDefs
openmesh/src/openmesh.nim:170:    identDefs[0].expectKind nnkIdent
openmesh/src/openmesh.nim:171:    identDefs[1].expectKind nnkIdent
openmesh/src/openmesh.nim:172:    identDefs[2].expectKind nnkEmpty
openmesh/src/openmesh.nim:186:  name.expectKind nnkIdent
openmesh/src/openmesh.nim:187:  argStmtList.expectKind nnkStmtList
openmesh/src/openmesh.nim:195:    argStmtList[0].expectKind nnkTypeSection
openmesh/src/openmesh.nim:198:  argTypeSection.expectKind nnkTypeSection
nim-chronos/chronos/asyncmacro2.nim:141:    expectKind(params[i], nnkIdentDefs)
opengl/src/opengl/private/errors.nim:6:  f.expectKind nnkStmtList
opengl/src/opengl/private/errors.nim:12:    child.expectKind nnkProcDef
ast-pattern-matching/tests/test1.nim:4:  arg.expectKind nnkStmtListExpr
ast-pattern-matching/tests/test1.nim:5:  arg[0].expectKind(nnkVarSection)
ast-pattern-matching/tests/test1.nim:6:  arg[1].expectKind({nnkAddr, nnkCall})
ast-pattern-matching/src/ast_pattern_matching.nim:109:  arg.expectKind nnkLiterals
ast-pattern-matching/src/ast_pattern_matching.nim:114:  arg.expectKind nnkLiterals
ast-pattern-matching/src/ast_pattern_matching.nim:119:  arg.expectKind nnkLiterals
ast-pattern-matching/src/ast_pattern_matching.nim:124:  arg.expectKind nnkLiterals
ast-pattern-matching/src/ast_pattern_matching.nim:127:  arg.expectKind nnkNilLit
ast-pattern-matching/src/ast_pattern_matching.nim:129:proc expectIdent(arg: NimNode; strVal: string): void {.compileTime.} =
ast-pattern-matching/src/ast_pattern_matching.nim:264:          pattern[1][0].expectIdent("strVal")
ast-pattern-matching/src/ast_pattern_matching.nim:266:          pattern[1][0].expectIdent("intVal")
ast-pattern-matching/src/ast_pattern_matching.nim:268:          pattern[1][0].expectIdent("floatVal")
ast-pattern-matching/src/ast_pattern_matching.nim:294:      matchedExpr.expectKind nnkIdent
ast-pattern-matching/src/ast_pattern_matching.nim:299:      pattern[1].expectKind nnkAccQuoted
ast-pattern-matching/src/ast_pattern_matching.nim:302:      matchedExpr.expectKind nnkIdent
ast-pattern-matching/src/ast_pattern_matching.nim:333:    args[i].expectKind nnkOfBranch
ast-pattern-matching/src/ast_pattern_matching.nim:337:      args[0].expectKind nnkIdent
ast-pattern-matching/src/ast_pattern_matching.nim:344:      args[^1].expectKind(nnkElse)
ast-pattern-matching/src/ast_pattern_matching.nim:363:    ofBranch.expectKind(nnkOfBranch)
ast-pattern-matching/src/ast_pattern_matching.nim:364:    ofBranch.expectLen(2)
ast-pattern-matching/src/ast_pattern_matching.nim:367:    code.expectKind nnkStmtList
ast-pattern-matching/src/ast_pattern_matching.nim:446:    ofBranch.expectKind(nnkOfBranch)
ast-pattern-matching/src/ast_pattern_matching.nim:447:    ofBranch.expectLen(2)
nimx/nimx/menu.nim:32:        i.expectKind(nnkPrefix)
nimx/nimx/class_registry.nim:10:    n.expectKind(nnkTypeDef)
nimx/nimx/class_registry.nim:42:        t[1].expectKind(nnkOfInherit)
nimx/nimx/private/helper_macros.nim:15:    cond.expectKind(nnkInfix)
nimx/nimx/private/helper_macros.nim:17:    cond[1].expectKind(nnkIdent)
nimx/nimx/layout.nim:117:#     expectKind(n, nnkDo)
tensordslnim/foldAst.nim:127:    node[0].expectKind nnkBracketExpr
tensordslnim/foldAst.nim:143:    objConstr.expectKind nnkObjConstr
tensordslnim/foldAst.nim:150:    valuesBracket.expectKind nnkBracket
tensordslnim/foldAst.nim:205:  assignments.expectKind nnkStmtList
tensordslnim/tensorDslInner.nim:94:    arg[2].expectKind nnkHiddenStdConv
tensordslnim/tensorDslInner.nim:95:    arg[2][1].expectKind nnkBracket
tensordslnim/tensorDslInner.nim:200:    asgn.expectKind nnkAsgn
tensordslnim/tensorDsl.nim:241:      asgn[1].expectKind nnkBracketExpr
tensordslnim/tensormath.nim:297:  arg.expectKind nnkSym
tensordslnim/tensormath.nim:489:  typ.expectKind nnkBracketExpr
tensordslnim/tensormath.nim:490:  typ[0].expectKind nnkSym
tensordslnim/tensormath.nim:493:  typ[1].expectKind nnkObjConstr
tensordslnim/tensormath.nim:494:  typ[1][1].expectKind nnkExprColonExpr
tensordslnim/tensormath.nim:557:    innerBody.expectKind nnkStmtList
tensordslnim/tensormath.nim:560:    innerBody.expectLen 0
tensordslnim/tensormathtblis.nim:4:  arg.expectKind nnkSym
tensordslnim/tensormathtblis.nim:6:  bracketExpr.expectKind nnkBracketExpr
tensordslnim/moremacros.nim:99:  innerStmtList.expectKind nnkStmtList
tensordslnim/moremacros.nim:100:  insertionPoint.expectKind nnkStmtList
pubviz/data.nim:5:  fieldPtrIdent.expectKind nnkIdent
pubviz/data.nim:7:  tpe.expectKind nnkObjectTy
pubviz/data.nim:10:    identDefs.expectKind nnkIdentDefs
pubviz/graphgen.nim:143:  graphTypeIdent.expectKind nnkIdent
pubviz/graphgen.nim:150:    section.expectKind({nnkIdent, nnkCall})
pubviz/graphgen.nim:153:      nodeTypesStmtList.expectKind nnkStmtList
pubviz/graphgen.nim:156:      connectivityStmtList.expectKind nnkStmtList
pubviz/graphgen.nim:200:    arg.expectKind nnkTypeSection
pubviz/graphgen.nim:201:    arg[0].expectKind nnkTypeDef
pubviz/graphgen.nim:202:    arg[0].expectLen 3
pubviz/graphgen.nim:203:    arg[0][2].expectKind nnkObjectTy
pubviz/graphgen.nim:204:    arg[0][2].expectLen 3
pubviz/graphgen.nim:205:    arg[0][2][2].expectKind nnkRecList
pubviz/graphgen.nim:209:    arg.expectKind nnkTypeSection
pubviz/graphgen.nim:210:    arg.expectLen 1
pubviz/graphgen.nim:211:    arg[0].expectKind nnkTypeDef
norm/src/norm/sqlite/sqlgen.nim:110:      expectKind(prag.value, {nnkIdent, nnkSym, nnkDotExpr})
norm/src/norm/sqlite/sqlgen.nim:166:  expectKind(field, nnkDotExpr)
norm/src/norm/sqlite/sqlgen.nim:179:  expectKind(field, nnkDotExpr)
norm/src/norm/postgres/sqlgen.nim:118:      expectKind(prag.value, {nnkIdent, nnkSym, nnkDotExpr})
norm/src/norm/postgres/sqlgen.nim:175:  expectKind(field, nnkDotExpr)
norm/src/norm/postgres/sqlgen.nim:198:  expectKind(field, nnkDotExpr)
norm/src/norm/objutils.nim:80:  expectKind(pragmaDefs, nnkPragma)
norm/src/norm/objutils.nim:91:  expectKind(def[0], {nnkIdent, nnkSym, nnkPostfix, nnkPragmaExpr})
norm/src/norm/objutils.nim:100:      expectKind(def[0][0], {nnkIdent, nnkSym, nnkPostfix})
norm/src/norm/objutils.nim:120:      expectKind(body[0], nnkTypeSection)
norm/src/norm/objutils.nim:141:  expectKind(typeDef[2], nnkObjectTy)
norm/src/norm/objutils.nim:175:      expectKind(body[0], nnkTypeSection)
norm/src/norm/objutils.nim:249:      expectKind(body[0], nnkTypeSection)
norm/src/norm/objutils.nim:274:      expectKind(body[0], nnkTypeSection)
norm/src/norm/postgres.nim:326:    expectKind(node, nnkTypeSection)
norm/src/norm/sqlite.nim:332:    expectKind(node, nnkTypeSection)
fancygl/experiment/deadcode.nim:39:  arg.expectKind nnkProcDef
fancygl/experiment/deadcode.nim:40:  arg[0].expectKind nnkIdent
fancygl/experiment/deadcode.nim:76:proc expectIdent*(arg: NimNode; identName: string): void =
fancygl/experiment/deadcode.nim:106:    expr.expectKind(nnkCallKinds + {nnkDotExpr})
fancygl/experiment/deadcode.nim:140:        identDefs.expectKind nnkIdentDefs
fancygl/experiment/deadcode.nim:182:  arg.expectKind nnkStmtList
fancygl/experiment/deadcode.nim:251:    asgn[1].expectKind nnkPragma
fancygl/experiment/deadcode.nim:252:    asgn[1].expectLen 2
fancygl/experiment/deadcode.nim:255:    dotExpr.expectKind nnkDotExpr
fancygl/experiment/deadcode.nim:257:    lhsSym.expectKind nnkSym
fancygl/experiment/deadcode.nim:265:    asgn.expectLen 1
fancygl/experiment/deadcode.nim:267:    identDefs.expectLen(3)
fancygl/experiment/deadcode.nim:341:      assignment[0].expectKind nnkDotExpr
fancygl/experiment/deadcode.nim:422:        rhs.expectLen(4)
fancygl/experiment/deadcode.nim:441:  arg.expectKind({nnkLetSection, nnkAsgn})
fancygl/experiment/deadcode.nim:448:  arg.expectKind({nnkLetSection, nnkAsgn})
fancygl/experiment/deadcode.nim:500:    asgn.expectKind nnkLetSection
fancygl/experiment/renderMacro.nim:82:  arg.expectKind nnkDo
fancygl/experiment/renderMacro.nim:100:    typeInst.expectKind nnkProcTy
fancygl/experiment/renderMacro.nim:101:    typeInst[0].expectKind nnkFormalParams
fancygl/experiment/renderMacro.nim:102:    typeInst[0][1].expectKind nnkIdentDefs
fancygl/experiment/renderMacro.nim:103:    typeInst[0][2].expectKind nnkIdentDefs
fancygl/experiment/renderMacro.nim:308:      result.expectKind nnkProcDef
fancygl/experiment/renderMacro.nim:309:      result[3].expectKind nnkFormalParams
fancygl/experiment/renderMacro.nim:568:      vertexType.expectKind(nnkSym)
fancygl/experiment/renderMacro.nim:573:      typeDef[2].expectKind nnkTupleTy
fancygl/experiment/renderMacro.nim:662:  arg.expectKind(nnkDo)
fancygl/experiment/renderMacro.nim:664:  formalParams.expectKind(nnkFormalParams)
fancygl/experiment/glslTranslate.nim:213:    argSym.expectKind nnkSym
fancygl/experiment/glslTranslate.nim:309:    arg[0].expectKind nnkSym
fancygl/experiment/glslTranslate.nim:315:      arg.expectLen(3)
fancygl/experiment/glslTranslate.nim:326:        arg.expectLen(2)
fancygl/fancygl/framebuffer.nim:73:  typename.expectKind nnkIdent
fancygl/fancygl/framebuffer.nim:96:    asgn.expectKind nnkAsgn
fancygl/fancygl/framebuffer.nim:102:        rhs.expectKind(nnkCall)
fancygl/fancygl/glwrapper.nim:771:  buffer.expectKind nnkSym
fancygl/fancygl/glwrapper.nim:783:  buffer.expectKind nnkSym
fancygl/fancygl/glwrapper.nim:793:  buffer.expectKind nnkSym
fancygl/fancygl/glwrapper.nim:1151:#   typeImpl.expectKind(nnkObjectTy)
fancygl/fancygl/glwrapper.nim:1179:#   typeImpl.expectKind(nnkObjectTy)
fancygl/fancygl/glwrapper.nim:1193:#     typeImpl.expectKind(nnkObjectTy)
fancygl/fancygl/glwrapper.nim:1230:    identDefs.expectKind nnkIdentDefs
fancygl/fancygl/macroutils.nim:71:proc expectIdent*(n: NimNode, name: string) {.compileTime.} =
fancygl/fancygl/macroutils.nim:75:  n.expectKind(nnkIdent)
fancygl/fancygl/normalizeType.nim:69:    result.expectKind nnkTypeDef
fancygl/fancygl/normalizeType.nim:80:    result.expectKind nnkTypeDef
fancygl/fancygl/normalizeType.nim:103:    sym.expectKind nnkSym
fancygl/fancygl/offsetof.nim:58:  field.expectKind(nnkIdent)
fancygl/fancygl/shadingDsl.nim:163:    call.expectKind nnkCall
fancygl/fancygl/shadingDsl.nim:220:        innerCall[1].expectKind nnkStrLit
fancygl/fancygl/shadingDsl.nim:265:        innerCall[1].expectKind nnkStrLit
fancygl/fancygl/shadingDsl.nim:574:    section.expectKind({nnkCall, nnkAsgn, nnkIdent})
fancygl/fancygl/shadingDsl.nim:582:      section.expectLen(2)
fancygl/fancygl/shadingDsl.nim:585:      ident.expectKind nnkIdent
fancygl/fancygl/shadingDsl.nim:630:      ident.expectKind nnkIdent
fancygl/fancygl/shadingDsl.nim:632:      stmtList.expectKind nnkStmtList
fancygl/fancygl/shadingDsl.nim:645:          capture.expectKind({nnkAsgn, nnkIdent})
fancygl/fancygl/shadingDsl.nim:650:            capture.expectLen 2
fancygl/fancygl/shadingDsl.nim:651:            capture[0].expectKind nnkIdent
fancygl/fancygl/shadingDsl.nim:669:          node.expectKind(nnkPragma)
fancygl/fancygl/shadingDsl.nim:670:          node.expectLen(1)
fancygl/fancygl/shadingDsl.nim:671:          node[0].expectKind(nnkExprColonExpr)
fancygl/fancygl/shadingDsl.nim:672:          node[0][0].expectIdent("divisor")
fancygl/fancygl/shadingDsl.nim:676:          capture.expectKind({nnkAsgn, nnkIdent, nnkPragmaExpr})
fancygl/fancygl/shadingDsl.nim:683:            capture.expectLen 2
fancygl/fancygl/shadingDsl.nim:684:            capture[0].expectKind nnkIdent
fancygl/fancygl/shadingDsl.nim:695:            capture[0].expectKind(nnkIdent)
fancygl/fancygl/shadingDsl.nim:712:              stmtList.expectKind nnkStmtList
fancygl/fancygl/shadingDsl.nim:735:          section.expectKind({
fancygl/fancygl/shadingDsl.nim:783:        stmtList.expectLen(1)
fancygl/fancygl/shadingDsl.nim:792:        stmtList.expectLen(1)
fancygl/fancygl/shadingDsl.nim:801:          stmtList[0].expectKind nnkDiscardStmt
fancygl/fancygl/shadingDsl.nim:803:          stmtList.expectLen(2)
fancygl/fancygl/shadingDsl.nim:804:          stmtList[0].expectKind({nnkTripleStrLit, nnkStrLit})
fancygl/fancygl/shadingDsl.nim:805:          stmtList[1].expectKind({nnkTripleStrLit, nnkStrLit})
fancygl/fancygl/shadingDsl.nim:817:          stmtList[0].expectKind nnkDiscardStmt
fancygl/fancygl/shadingDsl.nim:819:          stmtList.expectLen(2)
fancygl/fancygl/shadingDsl.nim:820:          stmtList[0].expectKind({nnkTripleStrLit, nnkStrLit})
fancygl/fancygl/shadingDsl.nim:821:          stmtList[1].expectKind({nnkTripleStrLit, nnkStrLit})
fancygl/fancygl/shadingDsl.nim:830:        stmtList.expectLen(1)
fancygl/fancygl/shadingDsl.nim:832:          stmtList[0].expectKind({ nnkTripleStrLit, nnkStrLit })
fancygl/fancygl/shadingDsl.nim:843:        stmtList.expectLen(1)
fancygl/fancygl/shadingDsl.nim:845:          stmtList[0].expectKind({ nnkTripleStrLit, nnkStrLit })
fancygl/fancygl/shadingDsl.nim:858:            statement.expectKind({nnkIdent,nnkStrLit,nnkTripleStrLit})
fancygl/fancygl/boring_stuff.nim:112:    identDefs.expectKind nnkIdentDefs
fancygl/fancygl/boring_stuff.nim:121:    identDefs.expectKind nnkIdentDefs
fancygl/fancygl/boring_stuff.nim:128:  arg.expectKind nnkCallKinds
fancygl/fancygl/boring_stuff.nim:555:  sym.expectKind({nnkSym,nnkBracketExpr})
variant/variant.nim:68:        # impl.expectKind({nnkTupleTy, nnkPar, nnkTupleConstr})
variant/variant.nim:243:    expectKind(body, nnkCaseStmt)
variant/variant.nim:257:            expectLen(c, 2)
variant/variant.nim:263:            expectKind(unpackSym, nnkIdent)
qtnim/nimport.nim:370:  arg.expectKind nnkStmtList
qtnim/nimport.nim:379:    procDef.expectKind nnkProcDef
qtnim/nimport.nim:386:    formalParams.expectKind nnkFormalParams
qtnim/nimport.nim:449:    identDefs.expectKind nnkIdentDefs
qtnim/nimport.nim:450:    identDefs.expectLen 3
patty/patty.nim:12:  n.expectKind(nnkIdent)
patty/patty.nim:15:proc expectKinds(n: NimNode, kinds: varargs[NimNodeKind]) =
patty/patty.nim:45:      id.expectKind(nnkIdent)
patty/patty.nim:81:  e.expectKinds(nnkIdent, nnkBracketExpr)
patty/patty.nim:95:  e.expectKinds(nnkIdent, nnkBracketExpr)
patty/patty.nim:99:  typeName.expectKind(nnkIdent)
patty/patty.nim:100:  body.expectKind(nnkStmtList)
patty/patty.nim:137:  e.expectKinds(nnkIdent, nnkBracketExpr)
patty/patty.nim:141:  typeName.expectKind(nnkIdent)
patty/patty.nim:182:  e.expectKinds(nnkIdent, nnkBracketExpr)
patty/patty.nim:186:  typeName.expectKind(nnkIdent)
patty/patty.nim:279:  id.expectKinds(nnkIdent, nnkSym)
patty/patty.nim:344:  statements.expectKind(nnkStmtList)
patty/patty.nim:365:  n.expectKind(nnkCall)
patty/patty.nim:404:    n.expectLen(2)
patty/patty.nim:449:  statements.expectKind(nnkStmtList)

disruptek commented 4 years ago

Since someone wanted opinions... IMHO, a generic ast-aware solution is a good idea, but you have to try harder to satisfy existing code.

Clyybber commented 4 years ago

it doesn't show the actual len nor can it show any additional runtime context because it doesnt' take a msg argument

It shows the actual len now.

expectLen shows result = newNimNode(nnkIntLit) which is super misleading / not helpful since it points to newLit 1 instead of result

Your example is specially crafted to fail this way.. expectX should be used on inputs.

this approach doesn't scale, there'll always be other conditions to check, it bloats macros.nim

Its about making often used stuff convinient. And providing coherent and nice error messages.

expectKind(n, nnkSym) hides the fact that n.kind is accessed, eg where searching for accessors of a field kind

This is not an issue. Its not like expectKind modifies kind..

it's "always on" whereas with assert/doAssert you get to customize whether it's always on or not

It should be always on. It would be really bad if my macro fails silently because I compiled with -d:danger.

none of these expectX procs allow passing an optional msg (eg with values)

Fine, you can easily "fix" that though.

there's no reason to differentiate between VM (doAssert) and CT (expectLen etc), doAssert can handle both

In general this RFC feels a bit like: Use tires instead of cars :p

akavel commented 4 years ago

As something of an alternative, I wrote some utility macros for use when writing macros ("...we must go deeper..."), which allow me to write code like this:

  if proto =~ Infix(Ident("->"), [], Ident(_)):
    rett = proto[2].handleJavaType
    proto = proto[1]
  # ...class & method name:
  if proto !~ Call(_):
    error "jproto expects a method declaration as an argument", proto
  if proto !~ Call(DotExpr(_), _):
    error "jproto expects dot-separated class & method name in the argument", proto
  if proto[0] !~ DotExpr(Ident(_), Ident(_)) and
    proto[0] !~ DotExpr(Ident(_), AccQuoted(_)):
    error "jproto expects exactly 2 dot-separated names in the argument", proto[0]

where:

[] means: "exactly 1 node of any kind"
_ means: "0 or more nodes of any kind"

They would definitively need some polishing etc. if they were to be e.g. considered for merging into stdlib, but even if not, I think they may work as an interesting example for discussion; I personally find them nice from readability point of view.

timotheecour commented 4 years ago

In general this RFC feels a bit like: Use tires instead of cars :p

or: use 1 car that's good enough to drive to multiple destinations instead of having to change car for every route.

In all seriousness, if doAssert isn't good enough (still not convinced on that but assuming), can't we at least have a single expect proc (or renamed to avoid clash with unittest.expect).

expectLen(n, 2) =>  expect n.len == 2
expectMinLen(n, 2) =>  expect n.len >= 2

and expect would then support everything that's supported by the expectX procs (in similar way as done in unittest by deconstructing the AST given to it).

expect would also take an optional msg param for when extra runtime info is needed, but that's a separate issue that can also be supported in each expectX proc

krux02 commented 4 years ago

@akavel looks like you have invented ast pattern matching as well.

Araq commented 4 years ago

While I see the value in improving the expectX procs/interface I think our attention is better spent on gc:arc, "on and dup" (chaining and outplace) (got an RFC in development), var/lets as alias, incremental recompilation, cmake integration .... well you get the idea :-)

krux02 commented 4 years ago

@timotheecour Please close this RFC, the improvement would be too minor and it eats up attention.

nim-lang / RFCs

deprecate macros.expectLen, expectKind, expectMinLen in favor of something generic or doAssert #185

proposal 1

proposal 2