From 741fed716edb163c95475db79b5a1ec95de11fa6 Mon Sep 17 00:00:00 2001
From: Jake Leahy <jake@leahy.dev>
Date: Tue, 21 Mar 2023 04:48:13 +1100
Subject: [PATCH] Use `analyseIfAddressTaken` logic for checking if address is
 taken in converter (#21533)

* Add a test case

There are way more test cases (See all branches of analyseIfAddressTaken but this covers at least a second branch

* Port analyseIfAddressTaken from semexprs to sigmatch

This was done since we cannot import sem or semexprs (circular import) but we need the rest of the logic. In needs to be done here since the converter isn't semmed afterwards and so we can't just leave the process til later use the version from semexprs

* Less hacky solution which has the checking be done in analyseIfAddressTakenInCall

This was done instead of the recommendation on removing it since sfAddrTaken is used in places other than the backend

* Remove weird whitespace

* Still check nkHiddenAddr if we are checking a converter
---
 compiler/semexprs.nim      | 24 +++++++++++++++---------
 compiler/sigmatch.nim      |  3 +--
 tests/converter/t21531.nim | 10 ++++++++++
 3 files changed, 26 insertions(+), 11 deletions(-)
 create mode 100644 tests/converter/t21531.nim

diff --git a/compiler/semexprs.nim b/compiler/semexprs.nim
index cfa34fcdc2..f74a72692b 100644
--- a/compiler/semexprs.nim
+++ b/compiler/semexprs.nim
@@ -787,7 +787,7 @@ proc analyseIfAddressTaken(c: PContext, n: PNode, isOutParam: bool): PNode =
   else:
     result = newHiddenAddrTaken(c, n, isOutParam)
 
-proc analyseIfAddressTakenInCall(c: PContext, n: PNode) =
+proc analyseIfAddressTakenInCall(c: PContext, n: PNode, isConverter = false) =
   checkMinSonsLen(n, 1, c.config)
   const
     FakeVarParams = {mNew, mNewFinalize, mInc, ast.mDec, mIncl, mExcl,
@@ -795,10 +795,15 @@ proc analyseIfAddressTakenInCall(c: PContext, n: PNode) =
       mAppendSeqElem, mNewSeq, mReset, mShallowCopy, mDeepCopy, mMove,
       mWasMoved}
 
+  template checkIfConverterCalled(c: PContext, n: PNode) =
+    ## Checks if there is a converter call which wouldn't be checked otherwise
+    # Call can sometimes be wrapped in a deref
+    let node = if n.kind == nkHiddenDeref: n[0] else: n
+    if node.kind == nkHiddenCallConv:
+      analyseIfAddressTakenInCall(c, node, true)
   # get the real type of the callee
   # it may be a proc var with a generic alias type, so we skip over them
   var t = n[0].typ.skipTypes({tyGenericInst, tyAlias, tySink})
-
   if n[0].kind == nkSym and n[0].sym.magic in FakeVarParams:
     # BUGFIX: check for L-Value still needs to be done for the arguments!
     # note sometimes this is eval'ed twice so we check for nkHiddenAddr here:
@@ -813,6 +818,8 @@ proc analyseIfAddressTakenInCall(c: PContext, n: PNode) =
               discard "allow access within a cast(unsafeAssign) section"
             else:
               localError(c.config, it.info, errVarForOutParamNeededX % $it)
+        # Make sure to still check arguments for converters
+        c.checkIfConverterCalled(n[i])
     # bug #5113: disallow newSeq(result) where result is a 'var T':
     if n[0].sym.magic in {mNew, mNewFinalize, mNewSeq}:
       var arg = n[1] #.skipAddr
@@ -824,15 +831,14 @@ proc analyseIfAddressTakenInCall(c: PContext, n: PNode) =
     return
   for i in 1..<n.len:
     let n = if n.kind == nkHiddenDeref: n[0] else: n
-    if n[i].kind == nkHiddenCallConv:
-      # we need to recurse explicitly here as converters can create nested
-      # calls and then they wouldn't be analysed otherwise
-      analyseIfAddressTakenInCall(c, n[i])
+    c.checkIfConverterCalled(n[i])
     if i < t.len and
         skipTypes(t[i], abstractInst-{tyTypeDesc}).kind in {tyVar}:
-      if n[i].kind != nkHiddenAddr:
-        n[i] = analyseIfAddressTaken(c, n[i], isOutParam(skipTypes(t[i], abstractInst-{tyTypeDesc})))
-
+      # Converters wrap var parameters in nkHiddenAddr but they haven't been analysed yet.
+      # So we need to make sure we are checking them still when in a converter call
+      if n[i].kind != nkHiddenAddr or isConverter:
+        n[i] = analyseIfAddressTaken(c, n[i].skipAddr(), isOutParam(skipTypes(t[i], abstractInst-{tyTypeDesc})))
+        
 include semmagic
 
 proc evalAtCompileTime(c: PContext, n: PNode): PNode =
diff --git a/compiler/sigmatch.nim b/compiler/sigmatch.nim
index 87a4fdf1c7..a1bd5f268e 100644
--- a/compiler/sigmatch.nim
+++ b/compiler/sigmatch.nim
@@ -1979,8 +1979,7 @@ proc userConvMatch(c: PContext, m: var TCandidate, f, a: PType,
       if srca == isSubtype:
         param = implicitConv(nkHiddenSubConv, src, copyTree(arg), m, c)
       elif src.kind in {tyVar}:
-        # Analyse the converter return type
-        arg.sym.flags.incl sfAddrTaken
+        # Analyse the converter return type.
         param = newNodeIT(nkHiddenAddr, arg.info, s.typ[1])
         param.add copyTree(arg)
       else:
diff --git a/tests/converter/t21531.nim b/tests/converter/t21531.nim
new file mode 100644
index 0000000000..b0198684d7
--- /dev/null
+++ b/tests/converter/t21531.nim
@@ -0,0 +1,10 @@
+import std/typetraits
+
+type Foo* = distinct string
+
+converter toBase*(headers: var Foo): var string =
+  headers.distinctBase
+
+proc bar*(headers: var Foo) =
+  for x in headers: discard
+