From 2af602a5c8184f8c29e208f9bf18fb9b8a60669c Mon Sep 17 00:00:00 2001 From: ringabout <43030857+ringabout@users.noreply.github.com> Date: Sat, 26 Oct 2024 04:35:26 +0800 Subject: [PATCH] deprecate `NewFinalize` with the ref T finalizer (#24354) pre-existing issues: ```nim block: type FooObj = object data: int Foo = ref ref FooObj proc delete(self: Foo) = echo self.data var s: Foo new(s, delete) ``` it crashed with arc/orc in 1.6.x and 2.x.x ```nim block: type Foo = ref int proc delete(self: Foo) = echo self[] var s: Foo new(s, delete) ``` The simple fix is to add a type restriction for the type `T` for arc/orc versions ```nim proc new*[T: object](a: var ref T, finalizer: proc (x: T) {.nimcall.}) ``` --- compiler/semmagic.nim | 48 ++++++++++++++++++++++-------------------- compiler/semstmts.nim | 4 ++-- lib/system.nim | 46 ++++++++++++++++++++++++++++------------ tests/arc/tarcmisc.nim | 27 ++++++++++++++++++++++++ 4 files changed, 86 insertions(+), 39 deletions(-) diff --git a/compiler/semmagic.nim b/compiler/semmagic.nim index 3c4b30f1d3..f3dff366eb 100644 --- a/compiler/semmagic.nim +++ b/compiler/semmagic.nim @@ -536,31 +536,33 @@ proc semNewFinalize(c: PContext; n: PNode): PNode = else: if fin.instantiatedFrom != nil and fin.instantiatedFrom != fin.owner: #undo move setOwner(fin, fin.instantiatedFrom) - let wrapperSym = newSym(skProc, getIdent(c.graph.cache, fin.name.s & "FinalizerWrapper"), c.idgen, fin.owner, fin.info) - let selfSymNode = newSymNode(copySym(fin.ast[paramsPos][1][0].sym, c.idgen)) - selfSymNode.typ() = fin.typ.firstParamType - wrapperSym.flags.incl sfUsed - let wrapper = c.semExpr(c, newProcNode(nkProcDef, fin.info, body = newTree(nkCall, newSymNode(fin), selfSymNode), - params = nkFormalParams.newTree(c.graph.emptyNode, - newTree(nkIdentDefs, selfSymNode, newNodeIT(nkType, - fin.ast[paramsPos][1][1].info, fin.typ.firstParamType), c.graph.emptyNode) - ), - name = newSymNode(wrapperSym), pattern = fin.ast[patternPos], - genericParams = fin.ast[genericParamsPos], pragmas = fin.ast[pragmasPos], exceptions = fin.ast[miscPos]), {}) + if fin.typ[1].skipTypes(abstractInst).kind != tyRef: + bindTypeHook(c, fin, n, attachedDestructor) + else: + let wrapperSym = newSym(skProc, getIdent(c.graph.cache, fin.name.s & "FinalizerWrapper"), c.idgen, fin.owner, fin.info) + let selfSymNode = newSymNode(copySym(fin.ast[paramsPos][1][0].sym, c.idgen)) + selfSymNode.typ() = fin.typ.firstParamType + wrapperSym.flags.incl sfUsed - var transFormedSym = turnFinalizerIntoDestructor(c, wrapperSym, wrapper.info) - setOwner(transFormedSym, fin) - if c.config.backend == backendCpp or sfCompileToCpp in c.module.flags: - let origParamType = transFormedSym.ast[bodyPos][1].typ - let selfSymbolType = makePtrType(c, origParamType.skipTypes(abstractPtrs)) - let selfPtr = newNodeI(nkHiddenAddr, transFormedSym.ast[bodyPos][1].info) - selfPtr.add transFormedSym.ast[bodyPos][1] - selfPtr.typ() = selfSymbolType - transFormedSym.ast[bodyPos][1] = c.semExpr(c, selfPtr) - # TODO: suppress var destructor warnings; if newFinalizer is not - # TODO: deprecated, try to implement plain T destructor - bindTypeHook(c, transFormedSym, n, attachedDestructor, suppressVarDestructorWarning = true) + let wrapper = c.semExpr(c, newProcNode(nkProcDef, fin.info, body = newTree(nkCall, newSymNode(fin), selfSymNode), + params = nkFormalParams.newTree(c.graph.emptyNode, + newTree(nkIdentDefs, selfSymNode, newNodeIT(nkType, + fin.ast[paramsPos][1][1].info, fin.typ.firstParamType), c.graph.emptyNode) + ), + name = newSymNode(wrapperSym), pattern = fin.ast[patternPos], + genericParams = fin.ast[genericParamsPos], pragmas = fin.ast[pragmasPos], exceptions = fin.ast[miscPos]), {}) + + var transFormedSym = turnFinalizerIntoDestructor(c, wrapperSym, wrapper.info) + setOwner(transFormedSym, fin) + if c.config.backend == backendCpp or sfCompileToCpp in c.module.flags: + let origParamType = transFormedSym.ast[bodyPos][1].typ + let selfSymbolType = makePtrType(c, origParamType.skipTypes(abstractPtrs)) + let selfPtr = newNodeI(nkHiddenAddr, transFormedSym.ast[bodyPos][1].info) + selfPtr.add transFormedSym.ast[bodyPos][1] + selfPtr.typ() = selfSymbolType + transFormedSym.ast[bodyPos][1] = c.semExpr(c, selfPtr) + bindTypeHook(c, transFormedSym, n, attachedDestructor) result = addDefaultFieldForNew(c, n) proc semPrivateAccess(c: PContext, n: PNode): PNode = diff --git a/compiler/semstmts.nim b/compiler/semstmts.nim index 8728359e9f..60778d25d6 100644 --- a/compiler/semstmts.nim +++ b/compiler/semstmts.nim @@ -2092,7 +2092,7 @@ proc bindDupHook(c: PContext; s: PSym; n: PNode; op: TTypeAttachedOp) = incl(s.flags, sfUsed) incl(s.flags, sfOverridden) -proc bindTypeHook(c: PContext; s: PSym; n: PNode; op: TTypeAttachedOp; suppressVarDestructorWarning = false) = +proc bindTypeHook(c: PContext; s: PSym; n: PNode; op: TTypeAttachedOp) = let t = s.typ var noError = false let cond = case op @@ -2116,7 +2116,7 @@ proc bindTypeHook(c: PContext; s: PSym; n: PNode; op: TTypeAttachedOp; suppressV elif obj.kind == tyGenericInvocation: obj = obj.genericHead else: break if obj.kind in {tyObject, tyDistinct, tySequence, tyString}: - if (not suppressVarDestructorWarning) and op == attachedDestructor and t.firstParamType.kind == tyVar and + if op == attachedDestructor and t.firstParamType.kind == tyVar and c.config.selectedGC in {gcArc, gcAtomicArc, gcOrc}: message(c.config, n.info, warnDeprecated, "A custom '=destroy' hook which takes a 'var T' parameter is deprecated; it should take a 'T' parameter") obj = canonType(c, obj) diff --git a/lib/system.nim b/lib/system.nim index 2f9cdc5f91..523e73e095 100644 --- a/lib/system.nim +++ b/lib/system.nim @@ -125,18 +125,38 @@ proc unsafeAddr*[T](x: T): ptr T {.magic: "Addr", noSideEffect.} = const ThisIsSystem = true -proc new*[T](a: var ref T, finalizer: proc (x: ref T) {.nimcall.}) {. - magic: "NewFinalize", noSideEffect.} - ## Creates a new object of type `T` and returns a safe (traced) - ## reference to it in `a`. - ## - ## When the garbage collector frees the object, `finalizer` is called. - ## The `finalizer` may not keep a reference to the - ## object pointed to by `x`. The `finalizer` cannot prevent the GC from - ## freeing the object. - ## - ## **Note**: The `finalizer` refers to the type `T`, not to the object! - ## This means that for each object of type `T` the finalizer will be called! +const arcLikeMem = defined(gcArc) or defined(gcAtomicArc) or defined(gcOrc) + +when defined(nimAllowNonVarDestructor) and arcLikeMem: + proc new*[T](a: var ref T, finalizer: proc (x: T) {.nimcall.}) {. + magic: "NewFinalize", noSideEffect.} + ## Creates a new object of type `T` and returns a safe (traced) + ## reference to it in `a`. + ## + ## When the garbage collector frees the object, `finalizer` is called. + ## The `finalizer` may not keep a reference to the + ## object pointed to by `x`. The `finalizer` cannot prevent the GC from + ## freeing the object. + ## + ## **Note**: The `finalizer` refers to the type `T`, not to the object! + ## This means that for each object of type `T` the finalizer will be called! + + proc new*[T](a: var ref T, finalizer: proc (x: ref T) {.nimcall.}) {. + magic: "NewFinalize", noSideEffect, deprecated: "pass a finalizer of the 'proc (x: T) {.nimcall.}' type".} + +else: + proc new*[T](a: var ref T, finalizer: proc (x: ref T) {.nimcall.}) {. + magic: "NewFinalize", noSideEffect.} + ## Creates a new object of type `T` and returns a safe (traced) + ## reference to it in `a`. + ## + ## When the garbage collector frees the object, `finalizer` is called. + ## The `finalizer` may not keep a reference to the + ## object pointed to by `x`. The `finalizer` cannot prevent the GC from + ## freeing the object. + ## + ## **Note**: The `finalizer` refers to the type `T`, not to the object! + ## This means that for each object of type `T` the finalizer will be called! proc `=wasMoved`*[T](obj: var T) {.magic: "WasMoved", noSideEffect.} = ## Generic `wasMoved`:idx: implementation that can be overridden. @@ -362,8 +382,6 @@ proc arrGet[I: Ordinal;T](a: T; i: I): T {. proc arrPut[I: Ordinal;T,S](a: T; i: I; x: S) {.noSideEffect, magic: "ArrPut".} -const arcLikeMem = defined(gcArc) or defined(gcAtomicArc) or defined(gcOrc) - when defined(nimAllowNonVarDestructor) and arcLikeMem and defined(nimPreviewNonVarDestructor): proc `=destroy`*[T](x: T) {.inline, magic: "Destroy".} = diff --git a/tests/arc/tarcmisc.nim b/tests/arc/tarcmisc.nim index b4476ef4f8..1a4ffeddb0 100644 --- a/tests/arc/tarcmisc.nim +++ b/tests/arc/tarcmisc.nim @@ -834,3 +834,30 @@ block: # bug #24141 doAssert abc == "fbc" main() + +block: + type + FooObj = object + data: int + Foo = ref FooObj + + + proc delete(self: FooObj) = + discard + + var s = Foo() + new(s, delete) + +block: + type + FooObj = object + data: int + i1, i2, i3, i4: float + Foo = ref FooObj + + + proc delete(self: FooObj) = + discard + + var s = Foo() + new(s, delete)