fixes a critical GC safety inference bug (#10615)

* fixes a critical GC safety inference bug
* make nimsuggest compile again
* make Nimble compile again

7 files changed, 65 insertions, 34 deletions

diff --git a/compiler/docgen.nim b/compiler/docgen.nim
index e6847cad3..7e7666de4 100644
--- a/compiler/docgen.nim
+++ b/compiler/docgen.nim
@@ -67,7 +67,7 @@ proc attachToType(d: PDoc; p: PSym): PSym =
 
 template declareClosures =
   proc compilerMsgHandler(filename: string, line, col: int,
-                          msgKind: rst.MsgKind, arg: string) {.procvar.} =
+                          msgKind: rst.MsgKind, arg: string) {.procvar, gcsafe.} =
     # translate msg kind:
     var k: TMsgKind
     case msgKind
@@ -81,9 +81,10 @@ template declareClosures =
     of mwUnknownSubstitution: k = warnUnknownSubstitutionX
     of mwUnsupportedLanguage: k = warnLanguageXNotSupported
     of mwUnsupportedField: k = warnFieldXNotSupported
-    globalError(conf, newLineInfo(conf, AbsoluteFile filename, line, col), k, arg)
+    {.gcsafe.}:
+      globalError(conf, newLineInfo(conf, AbsoluteFile filename, line, col), k, arg)
 
-  proc docgenFindFile(s: string): string {.procvar.} =
+  proc docgenFindFile(s: string): string {.procvar, gcsafe.} =
     result = options.findFile(conf, s).string
     if result.len == 0:
       result = getCurrentDir() / s
diff --git a/compiler/msgs.nim b/compiler/msgs.nim
index 78f253bdc..ca0425182 100644
--- a/compiler/msgs.nim
+++ b/compiler/msgs.nim
@@ -324,14 +324,15 @@ proc log*(s: string) {.procvar.} =
     f.writeLine(s)
     close(f)
 
-proc quit(conf: ConfigRef; msg: TMsgKind) =
+proc quit(conf: ConfigRef; msg: TMsgKind) {.gcsafe.} =
   if defined(debug) or msg == errInternal or hintStackTrace in conf.notes:
-    if stackTraceAvailable() and isNil(conf.writelnHook):
-      writeStackTrace()
-    else:
-      styledMsgWriteln(fgRed, "No stack traceback available\n" &
-          "To create a stacktrace, rerun compilation with ./koch temp " &
-          conf.command & " <file>")
+    {.gcsafe.}:
+      if stackTraceAvailable() and isNil(conf.writelnHook):
+        writeStackTrace()
+      else:
+        styledMsgWriteln(fgRed, "No stack traceback available\n" &
+            "To create a stacktrace, rerun compilation with ./koch temp " &
+            conf.command & " <file>")
   quit 1
 
 proc handleError(conf: ConfigRef; msg: TMsgKind, eh: TErrorHandling, s: string) =
diff --git a/compiler/options.nim b/compiler/options.nim
index 4bb17a03f..49ec6e87c 100644
--- a/compiler/options.nim
+++ b/compiler/options.nim
@@ -249,9 +249,9 @@ type
     suggestVersion*: int
     suggestMaxResults*: int
     lastLineInfo*: TLineInfo
-    writelnHook*: proc (output: string) {.closure.}
+    writelnHook*: proc (output: string) {.closure.} # cannot make this gcsafe yet because of Nimble
     structuredErrorHook*: proc (config: ConfigRef; info: TLineInfo; msg: string;
-                                severity: Severity) {.closure.}
+                                severity: Severity) {.closure, gcsafe.}
     cppCustomNamespace*: string
 
 proc hcrOn*(conf: ConfigRef): bool = return optHotCodeReloading in conf.globalOptions
diff --git a/compiler/ropes.nim b/compiler/ropes.nim
index 0d6d7d78f..2071ab46a 100644
--- a/compiler/ropes.nim
+++ b/compiler/ropes.nim
@@ -82,12 +82,13 @@ proc newRope(data: string = ""): Rope =
   result.L = -len(data)
   result.data = data
 
-var
-  cache: array[0..2048*2 - 1, Rope] # XXX Global here!
+when not compileOption("threads"):
+  var
+    cache: array[0..2048*2 - 1, Rope]
 
-proc resetRopeCache* =
-  for i in low(cache)..high(cache):
-    cache[i] = nil
+  proc resetRopeCache* =
+    for i in low(cache)..high(cache):
+      cache[i] = nil
 
 proc ropeInvariant(r: Rope): bool =
   if r == nil:
@@ -107,13 +108,16 @@ var gCacheMisses* = 0
 var gCacheIntTries* = 0
 
 proc insertInCache(s: string): Rope =
-  inc gCacheTries
-  var h = hash(s) and high(cache)
-  result = cache[h]
-  if isNil(result) or result.data != s:
-    inc gCacheMisses
+  when declared(cache):
+    inc gCacheTries
+    var h = hash(s) and high(cache)
+    result = cache[h]
+    if isNil(result) or result.data != s:
+      inc gCacheMisses
+      result = newRope(s)
+      cache[h] = result
+  else:
     result = newRope(s)
-    cache[h] = result
 
 proc rope*(s: string): Rope =
   ## Converts a string to a rope.
diff --git a/compiler/sempass2.nim b/compiler/sempass2.nim
index 923558a8d..9d46c0b0c 100644
--- a/compiler/sempass2.nim
+++ b/compiler/sempass2.nim
@@ -721,6 +721,17 @@ proc cstringCheck(tracked: PEffects; n: PNode) =
     message(tracked.config, n.info, warnUnsafeCode, renderTree(n))
 
 proc track(tracked: PEffects, n: PNode) =
+  template gcsafeAndSideeffectCheck() =
+    if notGcSafe(op) and not importedFromC(a):
+      # and it's not a recursive call:
+      if not (a.kind == nkSym and a.sym == tracked.owner):
+        if warnGcUnsafe in tracked.config.notes: warnAboutGcUnsafe(n, tracked.config)
+        markGcUnsafe(tracked, a)
+    if tfNoSideEffect notin op.flags and not importedFromC(a):
+      # and it's not a recursive call:
+      if not (a.kind == nkSym and a.sym == tracked.owner):
+        markSideEffect(tracked, a)
+
   case n.kind
   of nkSym:
     useVar(tracked, n)
@@ -764,18 +775,11 @@ proc track(tracked: PEffects, n: PNode) =
           propagateEffects(tracked, n, a.sym)
         elif isIndirectCall(a, tracked.owner):
           assumeTheWorst(tracked, n, op)
+          gcsafeAndSideeffectCheck()
       else:
         mergeEffects(tracked, effectList.sons[exceptionEffects], n)
         mergeTags(tracked, effectList.sons[tagEffects], n)
-        if notGcSafe(op) and not importedFromC(a):
-          # and it's not a recursive call:
-          if not (a.kind == nkSym and a.sym == tracked.owner):
-            if warnGcUnsafe in tracked.config.notes: warnAboutGcUnsafe(n, tracked.config)
-            markGcUnsafe(tracked, a)
-        if tfNoSideEffect notin op.flags and not importedFromC(a):
-          # and it's not a recursive call:
-          if not (a.kind == nkSym and a.sym == tracked.owner):
-            markSideEffect(tracked, a)
+        gcsafeAndSideeffectCheck()
     if a.kind != nkSym or a.sym.magic != mNBindSym:
       for i in 1 ..< len(n): trackOperand(tracked, n.sons[i], paramType(op, i), a)
     if a.kind == nkSym and a.sym.magic in {mNew, mNewFinalize, mNewSeq}:
diff --git a/lib/packages/docutils/rst.nim b/lib/packages/docutils/rst.nim
index 0b077b1f1..6e4e3f5dd 100644
--- a/lib/packages/docutils/rst.nim
+++ b/lib/packages/docutils/rst.nim
@@ -45,8 +45,8 @@ type
     mwUnsupportedField
 
   MsgHandler* = proc (filename: string, line, col: int, msgKind: MsgKind,
-                       arg: string) {.closure.} ## what to do in case of an error
-  FindFileHandler* = proc (filename: string): string {.closure.}
+                       arg: string) {.closure, gcsafe.} ## what to do in case of an error
+  FindFileHandler* = proc (filename: string): string {.closure, gcsafe.}
 
 const
   messages: array[MsgKind, string] = [
diff --git a/tests/effects/tgcsafe3.nim b/tests/effects/tgcsafe3.nim
new file mode 100644
index 000000000..5137efe4c
--- /dev/null
+++ b/tests/effects/tgcsafe3.nim
@@ -0,0 +1,21 @@
+discard """
+  errormsg: "'myproc' is not GC-safe as it accesses 'global_proc' which is a global using GC'ed memory"
+  line: 12
+  cmd: "nim $target --hints:on --threads:on $options $file"
+"""
+
+var useGcMem = "string here"
+
+var global_proc: proc(a: string) {.nimcall.} = proc (a: string) =
+  echo useGcMem
+
+proc myproc(i: int) {.gcsafe.} =
+  when false:
+    if global_proc != nil:
+      echo "a"
+    if isNil(global_proc):
+      return
+
+  global_proc("ho")
+
+myproc(0)