fixes #26133; Side effects not checked in =destroy

https://github.com/nim-lang/Nim/issues/25819

---------

Co-authored-by: Andreas Rumpf <araq4k@proton.me>

changelog.md

@@ -99,6 +99,7 @@ parameter and result types, not just their source-level shape. Use
   works without single-quoting.
 - `std/uri`: The `?` operator now appends query parameters to an existing query
   string instead of replacing it. Fixes [#19782](https://github.com/nim-lang/Nim/issues/19782).
+- `std/jsonutils`: `fromJson` now throws an exception when converting to `array`/`seq` if the JSON isn't an array instead of silently failing
 
 ## Language changes
 

compiler/aliasanalysis.nim

@@ -8,7 +8,7 @@ const
                  nkBracketExpr, nkDerefExpr, nkHiddenDeref,
                  nkAddr, nkHiddenAddr,
                  nkObjDownConv, nkObjUpConv}
-  PathKinds1* = {nkHiddenStdConv, nkHiddenSubConv}
+  PathKinds1* = {nkHiddenStdConv, nkHiddenSubConv, nkCast}
 
 proc skipConvDfa*(n: PNode): PNode =
   result = n
@@ -125,4 +125,3 @@ proc aliases*(obj, field: PNode): AliasKind =
       else:
         result = maybe
     else: assert false # unreachable
-

compiler/ast2nif.nim

@@ -800,7 +800,11 @@ proc writeSymNode(w: var Writer; dest: var IcBuilder; n: PNode; sym: PSym) =
       dest.addSymUse pool.syms.getOrIncl(w.toNifSymName(sym)), info
 
 proc writeNodeFlags(dest: var IcBuilder; flags: set[TNodeFlag]) {.inline.} =
-  writeFlags(dest, flags)
+  # Comment text is not stored in NIF; `nfHasComment` is process-local
+  # (see `comment` in ast.nim). Emitting it made IC non-deterministic:
+  # `copyTree` from a parsed generic kept the comment (`"sh"`) while
+  # `copyTree` from a cache-loaded generic did not (`"s"`).
+  writeFlags(dest, flags - {nfHasComment})
 
 template withNode(w: var Writer; dest: var IcBuilder; n: PNode; body: untyped) =
   dest.addParLe pool.tags.getOrIncl(toNifTag(n.kind)), trLineInfo(w, n.info)

compiler/cgen.nim

@@ -1854,10 +1854,16 @@ proc genVarPrototype(m: BModule, n: PNode) =
         typ = ptrType(typ)
       if lfDynamicLib in sym.loc.flags:
         typ = ptrType(typ)
-      m.s[cfsVars].addVar(m, sym,
-        name = sym.loc.snippet,
-        typ = typ,
-        visibility = vis)
+      if sfCodegenDecl in sym.flags:
+        m.s[cfsVars].addDeclWithVisibility(vis):
+          m.s[cfsVars].addVar(m, sym,
+            name = sym.loc.snippet,
+            typ = typ)
+      else:
+        m.s[cfsVars].addVar(m, sym,
+          name = sym.loc.snippet,
+          typ = typ,
+          visibility = vis)
       if m.hcrOn:
         m.initProc.procSec(cpsLocals).add('\t')
         m.initProc.procSec(cpsLocals).addAssignment(sym.loc.snippet,

compiler/dfa.nim

@@ -454,7 +454,7 @@ proc gen(c: var Con; n: PNode) =
   of nkPragmaBlock: gen(c, n.lastSon)
   of nkDiscardStmt, nkObjDownConv, nkObjUpConv, nkStringToCString, nkCStringToString:
     gen(c, n[0])
-  of nkConv, nkExprColonExpr, nkExprEqExpr, nkCast, PathKinds1:
+  of nkConv, nkExprColonExpr, nkExprEqExpr, PathKinds1:
     gen(c, n[1])
   of nkVarSection, nkLetSection: genVarSection(c, n)
   of nkDefer: raiseAssert "dfa construction pass requires the elimination of 'defer'"

compiler/semexprs.nim

@@ -1931,7 +1931,7 @@ proc borrowCheck(c: PContext, n, le, ri: PNode) =
     PathKinds0 = {nkDotExpr, nkCheckedFieldExpr,
                   nkBracketExpr, nkAddr, nkHiddenAddr,
                   nkObjDownConv, nkObjUpConv}
-    PathKinds1 = {nkHiddenStdConv, nkHiddenSubConv}
+    PathKinds1 = {nkHiddenStdConv, nkHiddenSubConv, nkCast}
 
   proc getRoot(n: PNode; followDeref: bool): PNode =
     result = n

compiler/sempass2.nim

@@ -129,6 +129,8 @@ proc collectObjectTree(graph: ModuleGraph, n: PNode) =
             else:
               graph.objectTree[root].add (depthLevel, typ)
 
+proc markSideEffect(a: PEffects; reason: PNode | PSym; useLoc: TLineInfo)
+
 proc createTypeBoundOps(tracked: PEffects, typ: PType; info: TLineInfo; explicit = false) =
   if typ == nil or (sfGeneratedOp in tracked.owner.flags and not explicit):
     # don't create type bound ops for anything in a function with a `nodestroy` pragma
@@ -147,6 +149,10 @@ proc createTypeBoundOps(tracked: PEffects, typ: PType; info: TLineInfo; explicit
     if op != nil and sfNeverRaises notin op.flags:
       tracked.canRaiseDefect = true
       break
+  let destructor = getAttachedOp(tracked.graph, typ, attachedDestructor)
+  if destructor != nil and sfOverridden in destructor.flags and
+      tfNoSideEffect notin destructor.typ.flags:
+    markSideEffect(tracked, destructor, info)
   if tracked.config.selectedGC == gcRefc or
       optSeqDestructors in tracked.config.globalOptions or
       tfHasAsgn in typ.flags:

lib/packages/docutils/rst.nim

@@ -3319,31 +3319,31 @@ proc dirInclude(p: var RstParser): PRstNode =
     rstMessage(p, meCannotOpenFile, filename)
   else:
     # XXX: error handling; recursive file inclusion!
+    let inputString = readFile(path)
+    let startPosition =
+      block:
+        let searchFor = n.getFieldValue("start-after").strip()
+        if searchFor != "":
+          let pos = inputString.find(searchFor)
+          if pos != -1: pos + searchFor.len
+          else: 0
+        else:
+          0
+
+    let endPosition =
+      block:
+        let searchFor = n.getFieldValue("end-before").strip()
+        if searchFor != "":
+          let pos = inputString.find(searchFor, start = startPosition)
+          if pos != -1: pos - 1
+          else: 0
+        else:
+          inputString.len - 1
+
     if getFieldValue(n, "literal") != "":
       result = newRstNode(rnLiteralBlock)
-      result.add newLeaf(readFile(path))
+      result.add newLeaf(inputString[startPosition..endPosition])
     else:
-      let inputString = readFile(path)
-      let startPosition =
-        block:
-          let searchFor = n.getFieldValue("start-after").strip()
-          if searchFor != "":
-            let pos = inputString.find(searchFor)
-            if pos != -1: pos + searchFor.len
-            else: 0
-          else:
-            0
-
-      let endPosition =
-        block:
-          let searchFor = n.getFieldValue("end-before").strip()
-          if searchFor != "":
-            let pos = inputString.find(searchFor, start = startPosition)
-            if pos != -1: pos - 1
-            else: 0
-          else:
-            inputString.len - 1
-
       var q: RstParser
       initParser(q, p.s)
       let saveFileIdx = p.s.currFileIdx

lib/pure/asynchttpserver.nim

@@ -152,9 +152,11 @@ proc parseProtocol(protocol: string): tuple[orig: string, major, minor: int] =
     raise newException(ValueError, "Invalid request protocol. Got: " &
         protocol)
   result.orig = protocol
-  i.inc protocol.parseSaturatedNatural(result.major, i)
-  if i < protocol.len: inc i # Skip .
-  i.inc protocol.parseSaturatedNatural(result.minor, i)
+  var n = protocol.parseSaturatedNatural(result.major, i)
+  i.inc n
+  if i < protocol.len and protocol[i] == '.':
+    inc i
+    n = protocol.parseSaturatedNatural(result.minor, i)
 
 proc sendStatus(client: AsyncSocket, status: string): Future[void] =
   client.send("HTTP/1.1 " & status & "\c\L\c\L")

lib/std/jsonutils.nim

@@ -238,6 +238,7 @@ proc fromJson*[T](a: var T, b: JsonNode, opt = Joptions()) =
       a = T()
       fromJson(a[], b, opt)
   elif T is array:
+    checkJson b.kind == JArray
     checkJson a.len == b.len, "Json array size doesn't match for " & $T
     var i = 0
     for ai in mitems(a):
@@ -248,6 +249,7 @@ proc fromJson*[T](a: var T, b: JsonNode, opt = Joptions()) =
     for val in b.getElems:
       incl a, jsonTo(val, E)
   elif T is seq:
+    checkJson b.kind == JArray
     a.setLen b.len
     for i, val in b.getElems:
       fromJson(a[i], val, opt)

lib/system/alloc.nim

@@ -155,15 +155,17 @@ type
 
   MemRegion = object
     when usesRegionHandles:
+      # Keeping the handle here does change the layout, but until proven otherwise
+      # this layout is more readable and shouldn't regress performance.
       regionHandle: ptr RegionHandle
     when not defined(gcDestructors):
       minLargeObj, maxLargeObj: int
     freeSmallChunks: array[0..max(1, SmallChunkSize div MemAlign-1), PSmallChunk]
       # List of available chunks per size class. Only one is expected to be active per class.
-    when defined(gcDestructors):
+    when defined(gcDestructors) and not usesRegionHandles:
       sharedFreeLists: SharedFreeLists
-        # Used directly without threads. Threaded builds use RegionHandle but
-        # retain this 2 KiB spacer: removing it regresses 2-4 KiB allocations.
+        # Remote-free buckets live on the MemRegion when there is no
+        # RegionHandle. Threaded memory managers with handles keep them on the handle instead.
     flBitmap: uint32
     slBitmap: array[RealFli, uint32]
     matrix: array[RealFli, array[MaxSli, PBigChunk]]
@@ -963,13 +965,19 @@ proc bigChunkAlignOffset(alignment: int): int {.inline.} =
   else:
     result = align(sizeof(BigChunk) + sizeof(FreeCell), alignment) - sizeof(BigChunk) - sizeof(FreeCell)
 
-proc rawAlloc(a: var MemRegion, requestedSize: int, alignment: int = 0): pointer =
+template rawAllocAux(aligned: static bool) {.dirty.} =
   when defined(nimTypeNames):
     inc(a.allocCounter)
   sysAssert(allocInv(a), "rawAlloc: begin")
   sysAssert(roundup(65, 8) == 72, "rawAlloc: roundup broken")
-  var size = roundup(requestedSize, max(MemAlign, alignment))
-  let alignOff = smallChunkAlignOffset(alignment)
+  when aligned:
+    var size = roundup(requestedSize, max(MemAlign, alignment))
+    let alignOff = smallChunkAlignOffset(alignment)
+  else:
+    # Common `alloc` path: no custom alignment. Keep this a separate
+    # instantiation so clang does not emit `smallChunkAlignOffset(0)`.
+    var size = (requestedSize + (MemAlign - 1)) and not (MemAlign - 1)
+    const alignOff = 0
   sysAssert(size >= sizeof(FreeCell), "rawAlloc: requested size too small")
   sysAssert(size >= requestedSize, "insufficient allocated size!")
   #c_fprintf(stdout, "alloc; size: %ld; %ld\n", requestedSize, size)
@@ -986,11 +994,13 @@ proc rawAlloc(a: var MemRegion, requestedSize: int, alignment: int = 0): pointer
             if atomicLoadN(sharedHead, ATOMIC_RELAXED) != nil:
               tc.freeList = atomicExchangeN(sharedHead, nil, ATOMIC_ACQUIRE)
           else:
-            tc.freeList = a.sharedFreeLists[s]
-            a.sharedFreeLists[s] = nil
-          # If `tc.freeList` isn't nil, `tc` gains capacity. Calculate how
-          # much it gained and how many foreign cells are included.
-          compensateCounters(a, tc, size)
+            let sharedHead = addr a.sharedFreeLists[s]
+            tc.freeList = sharedHead[]
+            sharedHead[] = nil
+          # Empty peeks are the common local case; skip the walk and the
+          # `free += 0` / `occ -= 0` stores clang would otherwise keep.
+          if tc.freeList != nil:
+            compensateCounters(a, tc, size)
 
     # allocate a small block: for small chunks, we use only its next pointer
     let s = size div MemAlign
@@ -1071,7 +1081,7 @@ proc rawAlloc(a: var MemRegion, requestedSize: int, alignment: int = 0): pointer
     # For big chunks with custom alignment, allocate extra space.
     # Since chunks are page-aligned, the needed padding is a compile-time
     # deterministic value rather than a worst-case estimate.
-    let alignPad = bigChunkAlignOffset(alignment)
+    let alignPad = when aligned: bigChunkAlignOffset(alignment) else: 0
     size = requestedSize + bigChunkOverhead() + alignPad
     # allocate a large block
     var c = if size >= HugeChunkSize: getHugeChunk(a, size)
@@ -1096,6 +1106,12 @@ proc rawAlloc(a: var MemRegion, requestedSize: int, alignment: int = 0): pointer
   when defined(heaptrack):
     heaptrack_malloc(result, requestedSize)
 
+proc rawAlloc(a: var MemRegion, requestedSize: int): pointer =
+  rawAllocAux(false)
+
+proc rawAlloc(a: var MemRegion, requestedSize: int, alignment: int): pointer =
+  rawAllocAux(true)
+
 proc rawAlloc0(a: var MemRegion, requestedSize: int): pointer =
   result = rawAlloc(a, requestedSize)
   zeroMem(result, requestedSize)

tests/ccgbugs/mcodegendeclglobal.nim

@@ -0,0 +1,4 @@
+var codegenDeclGlobal* {.codegenDecl: "$# /* custom declaration */ $#".} = 123
+
+proc readCodegenDeclGlobal*(): int {.inline.} =
+  codegenDeclGlobal

tests/ccgbugs/tcodegendeclglobal.nim

@@ -0,0 +1,13 @@
+discard """
+  output: '''
+123
+123
+'''
+  ccodecheck: "'extern NI /* custom declaration */ codegenDeclGlobal'"
+  targets: "c cpp"
+"""
+
+import ./mcodegendeclglobal
+
+echo codegenDeclGlobal
+echo readCodegenDeclGlobal()

tests/destructor/t26123.nim

@@ -0,0 +1,28 @@
+discard """
+  matrix: "--mm:orc"
+  output: "destroy b"
+"""
+
+# bug #26123
+
+type
+  A = ptr AObj
+
+  AObj = object
+    b: B
+
+  B = distinct ptr BObj
+
+  BObj = object
+    a: A
+
+proc `=destroy`(r: var B) =
+  echo "destroy b"
+
+proc main() =
+  var a = create(AObj)
+  var b = B(create(BObj))
+  a.b = b
+  cast[ptr BObj](b).a = a
+
+main()

tests/effects/t26133.nim

@@ -0,0 +1,13 @@
+discard """
+  cmd: "nim check --hints:off $file"
+  errormsg: "'del' can have side effects"
+  file: "system.nim"
+"""
+
+type MyObject = object
+
+proc `=destroy`(v: var MyObject) =
+  echo "hello"
+
+func remove(v: var seq[MyObject]) =
+  v.del(0)

tests/stdlib/tjsonutils.nim

@@ -451,6 +451,12 @@ template fn() =
       let json = inner.toJson(ToJsonOptions(enumMode: joptEnumSymbol))
       doAssert $json == """{"x":"hello","y":"A"}"""
 
+    block arrayTypeCheck:
+      let json = """{"key": "value"}""".parseJson()
+      var output: seq[int]
+      doAssertRaises(ValueError):
+        output.fromJson(json)
+
     block: # bug #21638
       type Something = object
 

tests/stdlib/trst.nim

@@ -1630,6 +1630,38 @@ And this should **NOT** be visible in `docs.html`
     doAssert "<em>Visible</em>" == rstToHtml(input, {roSandboxDisabled}, defaultConfig())
     removeFile("other.rst")
 
+
+  test "`:literal:` flag":
+    "code.nim".writeFile("""
+discard
+""")
+
+    let input = """
+.. include:: code.nim
+             :literal:
+"""
+    check "<pre>discard\n</pre>" == rstToHtml(input, {roSandboxDisabled}, defaultConfig())
+    removeFile("code.nim")
+
+
+  test "Include everything between in `:literal:` mode":
+    "code.nim".writeFile("""
+proc notIncluded = discard
+#CodeStart
+proc included = discard
+#CodeEnd
+proc notIncluded = discard
+""")
+
+    let input = """
+.. include:: code.nim
+             :literal:
+             :start-after: #CodeStart
+             :end-before: #CodeEnd
+"""
+    check "<pre>\nproc included = discard\n</pre>" == rstToHtml(input, {roSandboxDisabled}, defaultConfig())
+    removeFile("code.nim")
+
 suite "RST escaping":
   test "backspaces":
     check("""\ this""".toAst == dedent"""