Overload resultion with generic variables an inheritance

[Graveflo]

The test case diff is self explanatory

[Graveflo]

just to add some context here, the inheritance penalty is not looked at unless other more important factors are considered. (exact matches etc). For this reason a "penalty" of zero doesn't mean "no penalty" as in "better then a penalty of 1,2,..etc", it means "no relation". No relation has to be worse then any inheritance penalty because at least there is a relation if the penalty is non-zero.

[Graveflo]

actually, I think it would make more sense to rework this to make a penalty of 0 actually mean what it conceptually should and make the default -1 with negatives signifying "no inheritance relation".

[github-actions]

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from c1f91c2

Hint: mm: orc; opt: speed; options: -d:release
173262 lines; 8.625s; 652.074MiB peakmem

[metagn]

This broke something with varargs in a way I can't reproduce. It's the cause of this: nim-works/cps#333

Trying to compile cps/normalizedast without that change causes it, this should be the entire relevant code but it works by itself, so maybe it's related to symbol order:

# a.nim
import std/macros
type NormNode* = distinct NimNode
converter normNodeToNimNode(n: NormNode): NimNode =
  n.NimNode

# b.nim
import std/macros
from std/typetraits import distinctBase
from a import NormNode
proc upgradeToNormalizedNode*[T](n: T): NormNode =
  when T is NormNode:
    n
  elif T is NimNode or T is distinct and distinctBase(T) is NimNode:
    NormNode n
  else:
    {.error: "abc".}
type
  AnyNodeVarargs = varargs[NormNode, upgradeToNormalizedNode]
proc newTree*(kind: NimNodeKind, n: AnyNodeVarargs): NormNode =
  NormNode macros.newTree(kind, varargs[NimNode] n)
proc wrap*(kind: NimNodeKind, n: NormNode): NormNode =
  newTree(kind, n)

[Graveflo]

This broke something with varargs in a way I can't reproduce

Unless this is a comment just for documentation purposes I'm going to need more info then this. Not to be haphazard, but changes like this might change overload precedence, especially in edge cases. I'm not sure what binding rule is being violated here, and if that is actually a breakage in Nim. Looks like this potentially involves scope, distinct types, generics and varags which seems edge-casey. To flush out an issue we might need to really think about what the correct behavior should be. I believe there are pretty straight forward ways to determine if bindings are correct or not in general, but I can't really apply them here when it's not apparent what is going on.

[metagn]

Unless this is a comment just for documentation purposes I'm going to need more info then this.

Maybe I should have been clearer, trying to compile the file cps/normalizedast.nim by itself is enough to cause it. I don't have more information, if I did I would know what the issue is.

To make things easier, the issue still occurs if newTree in normalizedast just uses varargs[NormNode] instead of AnyNodeVarargs = varargs[NormNode, upgradeToNormalizedNode], as well as when removing the converter in rewrites never mind, it needs the converter because of #22416. So yes this is a Nim bug, it's picking an outright wrong overload. I'm just saying this issue only happens after this commit, I don't know anything more than that.

More info: a.convMatches == b.convMatches since varargs increases convMatches for both equal matches and converter matches. The problem is that the inheritancePenalty of the NimNode overload is 0 when the other is -1:

  ...
  result = a.convMatches - b.convMatches
  if result != 0: return
  result = cmpInheritancePenalty(a.inheritancePenalty, b.inheritancePenalty)
  if a.calleeSym != nil and a.calleeSym.name.s == "newTree":
    echo (result, a.inheritancePenalty, b.inheritancePenalty, a.calleeSym.typ, b.calleeSym.typ)
  if result != 0: return
  ...

Output of the echo is:

(-4611686018427387903, -1, 0, proc (kind: NimNodeKind, n: varargs[NormNode]): NormNode{.noSideEffect, gcsafe.}, proc (kind: NimNodeKind, children: varargs[NimNode]): NimNode{.noSideEffect, gcsafe.})

My guess is somehow the inheritance penalty is being delayed until after the converter call, which causes NimNode to match itself perfectly. If this is intentional, maybe we can change the varargs match to lower itself some other way, like with a negative c.varargsPenalty field.

[Graveflo]

Alright thanks for clarifying. I'll take a closer look later.