From 52a3acb0663a7d94c10f128a8ed3e1ff8e3b4be4 Mon Sep 17 00:00:00 2001 From: Reimer Behrends Date: Fri, 31 Oct 2014 07:55:56 +0100 Subject: [PATCH 1/4] Fix method recursion bug. Additional checks for method call transformations. --- compiler/transf.nim | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/compiler/transf.nim b/compiler/transf.nim index b9b06675ed..20fd5620f3 100644 --- a/compiler/transf.nim +++ b/compiler/transf.nim @@ -624,7 +624,11 @@ proc transformCall(c: PTransf, n: PNode): PTransNode = # bugfix: check after 'transformSons' if it's still a method call: # use the dispatcher for the call: if s.sons[0].kind == nkSym and s.sons[0].sym.kind == skMethod: - result = methodCall(s).PTransNode + let t = lastSon(s.sons[0].sym.ast) + if t.kind == nkSym and sfDispatcher in t.sym.flags: + result = methodCall(s).PTransNode + else: + result = s.PTransNode else: result = s.PTransNode From 1fc8bab6433982611b8dc3585e65dbd7be946a89 Mon Sep 17 00:00:00 2001 From: Reimer Behrends Date: Sat, 1 Nov 2014 04:07:42 +0100 Subject: [PATCH 2/4] Reset location when creating a method dispatcher When creating a method dispatcher, the location of the underlying method was copied. Under some circumstances, the name of the location (loc.r) was already initialized, in which case the method dispatcher shared a name with one of the methods, leading to a C compiler error. By setting loc.r to nil when copying the dispatcher information from the original method, we ensure that the dispatcher C function gets its proper name. --- compiler/cgmeth.nim | 1 + 1 file changed, 1 insertion(+) diff --git a/compiler/cgmeth.nim b/compiler/cgmeth.nim index 948541302c..9755bc6745 100644 --- a/compiler/cgmeth.nim +++ b/compiler/cgmeth.nim @@ -100,6 +100,7 @@ proc methodDef*(s: PSym, fromCache: bool) = if disp.typ.callConv == ccInline: disp.typ.callConv = ccDefault disp.ast = copyTree(s.ast) disp.ast.sons[bodyPos] = ast.emptyNode + disp.loc.r = nil if s.typ.sons[0] != nil: disp.ast.sons[resultPos].sym = copySym(s.ast.sons[resultPos].sym) attachDispatcher(s, newSymNode(disp)) From bce2d57d95b71f3e4a5d4585ea735870251d1caa Mon Sep 17 00:00:00 2001 From: Reimer Behrends Date: Sun, 2 Nov 2014 15:37:16 +0100 Subject: [PATCH 3/4] Added test case for recursive methods. --- tests/method/trecmeth.nim | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 tests/method/trecmeth.nim diff --git a/tests/method/trecmeth.nim b/tests/method/trecmeth.nim new file mode 100644 index 0000000000..32f620f15f --- /dev/null +++ b/tests/method/trecmeth.nim @@ -0,0 +1,22 @@ +# Note: We only compile this to verify that code generation +# for recursive methods works, no code is being executed + +type + Obj = ref object of TObject + +# Mutual recursion + +method alpha(x: Obj) +method beta(x: Obj) + +method alpha(x: Obj) = + beta(x) + +method beta(x: Obj) = + alpha(x) + +# Simple recursion + +method gamma(x: Obj) = + gamma(x) + From ce9a57fcfd21a1a3cb17087107c246f0492bec7f Mon Sep 17 00:00:00 2001 From: Reimer Behrends Date: Sun, 2 Nov 2014 23:35:41 +0100 Subject: [PATCH 4/4] Fix dispatcher creation for method prototypes. When method prototypes were involved (e.g. forward declarations for mutual recursion), calls were sometimes dispatched to the wrong method implementation. One of the reasons was that method dispatchers were then not always attached to method ASTs in the correct place. --- compiler/cgmeth.nim | 88 +++++++++++++++++++++++++++++----------- compiler/transf.nim | 7 ++-- tests/method/tmproto.nim | 25 ++++++++++++ 3 files changed, 92 insertions(+), 28 deletions(-) create mode 100644 tests/method/tmproto.nim diff --git a/compiler/cgmeth.nim b/compiler/cgmeth.nim index 9755bc6745..d881226d4d 100644 --- a/compiler/cgmeth.nim +++ b/compiler/cgmeth.nim @@ -11,7 +11,7 @@ import intsets, options, ast, astalgo, msgs, idents, renderer, types, magicsys, - sempass2 + sempass2, strutils proc genConv(n: PNode, d: PType, downcast: bool): PNode = var dest = skipTypes(d, abstractPtrs) @@ -44,7 +44,8 @@ proc methodCall*(n: PNode): PNode = result.sons[i] = genConv(result.sons[i], disp.typ.sons[i], true) # save for incremental compilation: -var gMethods: seq[TSymSeq] = @[] +var + gMethods: seq[tuple[methods: TSymSeq, dispatcher: PSym]] = @[] proc sameMethodBucket(a, b: PSym): bool = result = false @@ -80,32 +81,70 @@ proc attachDispatcher(s: PSym, dispatcher: PNode) = else: s.ast.add(dispatcher) +proc createDispatcher(s: PSym): PSym = + var disp = copySym(s) + incl(disp.flags, sfDispatcher) + excl(disp.flags, sfExported) + disp.typ = copyType(disp.typ, disp.typ.owner, false) + # we can't inline the dispatcher itself (for now): + if disp.typ.callConv == ccInline: disp.typ.callConv = ccDefault + disp.ast = copyTree(s.ast) + disp.ast.sons[bodyPos] = ast.emptyNode + disp.loc.r = nil + if s.typ.sons[0] != nil: + if disp.ast.sonsLen > resultPos: + disp.ast.sons[resultPos].sym = copySym(s.ast.sons[resultPos].sym) + else: + # We've encountered a method prototype without a filled-in + # resultPos slot. We put a placeholder in there that will + # be updated in fixupDispatcher(). + disp.ast.addSon(ast.emptyNode) + attachDispatcher(s, newSymNode(disp)) + # attach to itself to prevent bugs: + attachDispatcher(disp, newSymNode(disp)) + return disp + +proc fixupDispatcher(meth, disp: PSym) = + # We may have constructed the dispatcher from a method prototype + # and need to augment the incomplete dispatcher with information + # from later definitions, particularly the resultPos slot. Also, + # the lock level of the dispatcher needs to be updated/checked + # against that of the method. + if disp.ast.sonsLen > resultPos and meth.ast.sonsLen > resultPos and + disp.ast.sons[resultPos] == ast.emptyNode: + disp.ast.sons[resultPos] = copyTree(meth.ast.sons[resultPos]) + + # The following code works only with lock levels, so we disable + # it when they're not available. + when declared(TLockLevel): + proc `<`(a, b: TLockLevel): bool {.borrow.} + proc `==`(a, b: TLockLevel): bool {.borrow.} + if disp.typ.lockLevel == UnspecifiedLockLevel: + disp.typ.lockLevel = meth.typ.lockLevel + elif meth.typ.lockLevel != UnspecifiedLockLevel and + meth.typ.lockLevel != disp.typ.lockLevel: + message(meth.info, warnLockLevel, + "method has lock level $1, but another method has $2" % + [$meth.typ.lockLevel, $disp.typ.lockLevel]) + # XXX The following code silences a duplicate warning in + # checkMethodeffects() in sempass2.nim for now. + if disp.typ.lockLevel < meth.typ.lockLevel: + disp.typ.lockLevel = meth.typ.lockLevel + proc methodDef*(s: PSym, fromCache: bool) = var L = len(gMethods) for i in countup(0, L - 1): - let disp = gMethods[i][0] + var disp = gMethods[i].dispatcher if sameMethodBucket(disp, s): - add(gMethods[i], s) + add(gMethods[i].methods, s) attachDispatcher(s, lastSon(disp.ast)) + fixupDispatcher(s, disp) when useEffectSystem: checkMethodEffects(disp, s) return - add(gMethods, @[s]) # create a new dispatcher: - if not fromCache: - var disp = copySym(s) - incl(disp.flags, sfDispatcher) - excl(disp.flags, sfExported) - disp.typ = copyType(disp.typ, disp.typ.owner, false) - # we can't inline the dispatcher itself (for now): - if disp.typ.callConv == ccInline: disp.typ.callConv = ccDefault - disp.ast = copyTree(s.ast) - disp.ast.sons[bodyPos] = ast.emptyNode - disp.loc.r = nil - if s.typ.sons[0] != nil: - disp.ast.sons[resultPos].sym = copySym(s.ast.sons[resultPos].sym) - attachDispatcher(s, newSymNode(disp)) - # attach to itself to prevent bugs: - attachDispatcher(disp, newSymNode(disp)) + add(gMethods, (methods: @[s], dispatcher: createDispatcher(s))) + if fromCache: + internalError(s.info, "no method dispatcher found") proc relevantCol(methods: TSymSeq, col: int): bool = # returns true iff the position is relevant @@ -195,8 +234,9 @@ proc generateMethodDispatchers*(): PNode = result = newNode(nkStmtList) for bucket in countup(0, len(gMethods) - 1): var relevantCols = initIntSet() - for col in countup(1, sonsLen(gMethods[bucket][0].typ) - 1): - if relevantCol(gMethods[bucket], col): incl(relevantCols, col) - sortBucket(gMethods[bucket], relevantCols) - addSon(result, newSymNode(genDispatcher(gMethods[bucket], relevantCols))) + for col in countup(1, sonsLen(gMethods[bucket].methods[0].typ) - 1): + if relevantCol(gMethods[bucket].methods, col): incl(relevantCols, col) + sortBucket(gMethods[bucket].methods, relevantCols) + addSon(result, + newSymNode(genDispatcher(gMethods[bucket].methods, relevantCols))) diff --git a/compiler/transf.nim b/compiler/transf.nim index 20fd5620f3..1ec31d605c 100644 --- a/compiler/transf.nim +++ b/compiler/transf.nim @@ -625,10 +625,9 @@ proc transformCall(c: PTransf, n: PNode): PTransNode = # use the dispatcher for the call: if s.sons[0].kind == nkSym and s.sons[0].sym.kind == skMethod: let t = lastSon(s.sons[0].sym.ast) - if t.kind == nkSym and sfDispatcher in t.sym.flags: - result = methodCall(s).PTransNode - else: - result = s.PTransNode + if t.kind != nkSym or sfDispatcher notin t.sym.flags: + methodDef(s.sons[0].sym, false) + result = methodCall(s).PTransNode else: result = s.PTransNode diff --git a/tests/method/tmproto.nim b/tests/method/tmproto.nim new file mode 100644 index 0000000000..5d75cff1a0 --- /dev/null +++ b/tests/method/tmproto.nim @@ -0,0 +1,25 @@ +type + Obj1 = ref object {.inheritable.} + Obj2 = ref object of Obj1 + +method beta(x: Obj1): int + +proc delta(x: Obj2): int = + beta(x) + +method beta(x: Obj2): int + +proc alpha(x: Obj1): int = + beta(x) + +method beta(x: Obj1): int = 1 +method beta(x: Obj2): int = 2 + +proc gamma(x: Obj1): int = + beta(x) + +doAssert alpha(Obj1()) == 1 +doAssert gamma(Obj1()) == 1 +doAssert alpha(Obj2()) == 2 +doAssert gamma(Obj2()) == 2 +doAssert delta(Obj2()) == 2