From 44e7a7b6c2b37c0632cc030c0c90df4d92c50088 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Arne=20D=C3=B6ring?= <arne.doering@gmx.net>
Date: Thu, 8 Aug 2019 16:57:06 +0200
Subject: [PATCH] Lock semchecked ast for macros (#11883) [bugfix]

* reject to modify type checked AST
* add flag to back out
* Introduce legacy feature set.
---
 compiler/commands.nim            | 20 +++++++++++++-------
 compiler/options.nim             |  9 ++++++++-
 compiler/vm.nim                  | 25 +++++++++++++++----------
 doc/advopt.txt                   |  3 +++
 tests/macros/tlocktypednode1.nim | 12 ++++++++++++
 tests/macros/tlocktypednode2.nim | 12 ++++++++++++
 tests/macros/tlocktypednode3.nim | 15 +++++++++++++++
 tests/macros/tmacro7.nim         | 11 +++++------
 8 files changed, 83 insertions(+), 24 deletions(-)
 create mode 100644 tests/macros/tlocktypednode1.nim
 create mode 100644 tests/macros/tlocktypednode2.nim
 create mode 100644 tests/macros/tlocktypednode3.nim

diff --git a/compiler/commands.nim b/compiler/commands.nim
index 87ded61db8..3874ea38fb 100644
--- a/compiler/commands.nim
+++ b/compiler/commands.nim
@@ -50,15 +50,16 @@ const
       "Compiled at $4\n" &
       "Copyright (c) 2006-" & copyrightYear & " by Andreas Rumpf\n"
 
+proc genFeatureDesc[T: enum](t: typedesc[T]): string {.compileTime.} =
+  var x = ""
+  for f in low(T)..high(T):
+    if x.len > 0: x.add "|"
+    x.add $f
+  x
+
 const
   Usage = slurp"../doc/basicopt.txt".replace(" //", " ")
-  FeatureDesc = block:
-    var x = ""
-    for f in low(Feature)..high(Feature):
-      if x.len > 0: x.add "|"
-      x.add $f
-    x
-  AdvancedUsage = slurp"../doc/advopt.txt".replace(" //", " ") % FeatureDesc
+  AdvancedUsage = slurp"../doc/advopt.txt".replace(" //", " ") % [genFeatureDesc(Feature), genFeatureDesc(LegacyFeature)]
 
 proc getCommandLineDesc(conf: ConfigRef): string =
   result = (HelpMessage % [VersionAsString, platform.OS[conf.target.hostOS].name,
@@ -744,6 +745,11 @@ proc processSwitch*(switch, arg: string, pass: TCmdLinePass, info: TLineInfo;
         conf.features.incl parseEnum[Feature](arg)
       except ValueError:
         localError(conf, info, "unknown experimental feature")
+  of "legacy":
+    try:
+      conf.legacyFeatures.incl parseEnum[LegacyFeature](arg)
+    except ValueError:
+      localError(conf, info, "unknown obsolete feature")
   of "nocppexceptions":
     expectNoArg(conf, switch, arg, pass, info)
     incl(conf.globalOptions, optNoCppExceptions)
diff --git a/compiler/options.nim b/compiler/options.nim
index 6303224a2d..75eec47560 100644
--- a/compiler/options.nim
+++ b/compiler/options.nim
@@ -135,6 +135,12 @@ type
       ## which itself requires `nimble install libffi`, see #10150
       ## Note: this feature can't be localized with {.push.}
 
+  LegacyFeature* = enum
+    allowSemcheckedAstModification,
+      ## Allows to modify a NimNode where the type has already been
+      ## flaged with nfSem. If you actually do this, it will cause
+      ## bugs.
+
   SymbolFilesOption* = enum
     disabledSf, writeOnlySf, readOnlySf, v2Sf
 
@@ -196,6 +202,7 @@ type
     cppDefines*: HashSet[string] # (*)
     headerFile*: string
     features*: set[Feature]
+    legacyFeatures*: set[LegacyFeature]
     arguments*: string ## the arguments to be passed to the program that
                        ## should be run
     ideCmd*: IdeCmd
@@ -315,7 +322,7 @@ proc newConfigRef*(): ConfigRef =
     m: initMsgConfig(),
     evalExpr: "",
     cppDefines: initHashSet[string](),
-    headerFile: "", features: {}, foreignPackageNotes: {hintProcessing, warnUnknownMagic,
+    headerFile: "", features: {}, legacyFeatures: {}, foreignPackageNotes: {hintProcessing, warnUnknownMagic,
     hintQuitCalled, hintExecuting},
     notes: NotesVerbosity[1], mainPackageNotes: NotesVerbosity[1],
     configVars: newStringTable(modeStyleInsensitive),
diff --git a/compiler/vm.nim b/compiler/vm.nim
index 26b7ab5a9a..75f6cc1c34 100644
--- a/compiler/vm.nim
+++ b/compiler/vm.nim
@@ -1392,27 +1392,32 @@ proc rawExecute(c: PCtx, start: int, tos: PStackFrame): TFullReg =
       decodeBC(rkNode)
       let idx = regs[rb].intVal.int
       var dest = regs[ra].node
-      if dest.kind notin {nkEmpty..nkNilLit} and idx <% dest.len:
-        dest.sons[idx] = regs[rc].node
-      else:
+      if nfSem in dest.flags and allowSemcheckedAstModification notin c.config.legacyFeatures:
+        stackTrace(c, tos, pc, "typechecked nodes may not be modified")
+      elif dest.kind in {nkEmpty..nkNilLit} or idx >=% dest.len:
         stackTrace(c, tos, pc, formatErrorIndexBound(idx, dest.len-1))
+      else:
+        dest.sons[idx] = regs[rc].node
     of opcNAdd:
       decodeBC(rkNode)
       var u = regs[rb].node
-      if u.kind notin {nkEmpty..nkNilLit}:
-        u.add(regs[rc].node)
-      else:
+      if nfSem in u.flags and allowSemcheckedAstModification notin c.config.legacyFeatures:
+        stackTrace(c, tos, pc, "typechecked nodes may not be modified")
+      elif u.kind in {nkEmpty..nkNilLit}:
         stackTrace(c, tos, pc, "cannot add to node kind: " & $u.kind)
+      else:
+        u.add(regs[rc].node)
       regs[ra].node = u
     of opcNAddMultiple:
       decodeBC(rkNode)
       let x = regs[rc].node
       var u = regs[rb].node
-      if u.kind notin {nkEmpty..nkNilLit}:
-        # XXX can be optimized:
-        for i in 0..<x.len: u.add(x.sons[i])
-      else:
+      if nfSem in u.flags and allowSemcheckedAstModification notin c.config.legacyFeatures:
+        stackTrace(c, tos, pc, "typechecked nodes may not be modified")
+      elif u.kind in {nkEmpty..nkNilLit}:
         stackTrace(c, tos, pc, "cannot add to node kind: " & $u.kind)
+      else:
+        for i in 0 ..< x.len: u.add(x.sons[i])
       regs[ra].node = u
     of opcNKind:
       decodeB(rkInt)
diff --git a/doc/advopt.txt b/doc/advopt.txt
index 00459333d9..f5359c0c07 100644
--- a/doc/advopt.txt
+++ b/doc/advopt.txt
@@ -120,6 +120,9 @@ Advanced options:
   --errorMax:N              stop compilation after N errors; 0 means unlimited
   --experimental:$1
                             enable experimental language feature
+  --legacy:$2
+                            enable obsolete/legacy language feature
+                            legacy code.
   --newruntime              use an alternative runtime that uses destructors
                             and that uses a shared heap via -d:useMalloc
   --profiler:on|off         enable profiling; requires `import nimprof`, and
diff --git a/tests/macros/tlocktypednode1.nim b/tests/macros/tlocktypednode1.nim
new file mode 100644
index 0000000000..f760c7cf12
--- /dev/null
+++ b/tests/macros/tlocktypednode1.nim
@@ -0,0 +1,12 @@
+discard """
+errormsg: "typechecked nodes may not be modified"
+"""
+
+import macros
+
+macro doSomething(arg: typed): untyped =
+  echo arg.treeREpr
+  result = arg
+  result.add newCall(bindSym"echo", newLit(1))
+
+doSomething((echo(1); echo(2)))
diff --git a/tests/macros/tlocktypednode2.nim b/tests/macros/tlocktypednode2.nim
new file mode 100644
index 0000000000..e921772b04
--- /dev/null
+++ b/tests/macros/tlocktypednode2.nim
@@ -0,0 +1,12 @@
+discard """
+errormsg: "typechecked nodes may not be modified"
+"""
+
+import macros
+
+macro doSomething(arg: typed): untyped =
+  echo arg.treeREpr
+  result = arg
+  result[0] = newCall(bindSym"echo", newLit(1))
+
+doSomething((echo(1); echo(2)))
diff --git a/tests/macros/tlocktypednode3.nim b/tests/macros/tlocktypednode3.nim
new file mode 100644
index 0000000000..0eb9d4467e
--- /dev/null
+++ b/tests/macros/tlocktypednode3.nim
@@ -0,0 +1,15 @@
+discard """
+errormsg: "typechecked nodes may not be modified"
+"""
+
+import macros
+
+macro doSomething(arg: typed): untyped =
+  echo arg.treeREpr
+  result = arg
+  result.add(
+    newCall(bindSym"echo", newLit(3)),
+    newCall(bindSym"echo", newLit(1))
+  )
+
+doSomething((echo(1); echo(2)))
diff --git a/tests/macros/tmacro7.nim b/tests/macros/tmacro7.nim
index 3d450c2916..602191506c 100644
--- a/tests/macros/tmacro7.nim
+++ b/tests/macros/tmacro7.nim
@@ -1,19 +1,20 @@
 discard """
-  output: '''calling!stuff
+output: '''
+calling!stuff
 calling!stuff
 '''
-  joinable: false
+disabled: true
 """
 
+# this test modifies an already semchecked ast (bad things happen)
+# this test relies on the bug #4547
 # issue #7792
 
 import macros
 
-
 proc callProc(str: string) =
   echo "calling!" & str
 
-
 macro testMacro(code: typed): untyped =
   let stmtList = newNimNode(nnkStmtList)
 
@@ -27,11 +28,9 @@ macro testMacro(code: typed): untyped =
 
   result = newEmptyNode()
 
-
 proc main() {.testMacro.} =
   echo "test"
   echo "test2"
 
-
 when isMainModule:
   main()