Nim icon indicating copy to clipboard operation
Nim copied to clipboard

fixes #23755; array static inference during overload resolution

Open Graveflo opened this issue 1 year ago • 5 comments

#23755

Graveflo avatar Jun 26 '24 04:06 Graveflo

We should add both test cases you mentioned in https://github.com/nim-lang/Nim/issues/23755#issuecomment-2187859094. If it's intentional because you will address the first one later, carry on.

mratsim avatar Jun 26 '24 20:06 mratsim

We should add both test cases you mentioned in https://github.com/nim-lang/Nim/issues/23755#issuecomment-2187859094. If it's intentional because you will address the first one later, carry on.

Yea I don't have a lot of time to work on this. Im just just leaving it here drafted as I make progress. Plus, the CI gets effed up every time I try to run it locally and my my repos so I use that here sometimes.

Graveflo avatar Jun 26 '24 21:06 Graveflo

CI failing, but idk, it looks like http connection issues. This is where I'm at anyway:

Both of these "fixes" are dirty workarounds. The code near getOrdValue might actually be a decent idea, but the code added in inferStaticsInRange is not. I still think this incompatibility needs to be flushed out earlier. It's far less then ideal to decide if input is valid by bouncing them off of those ord procs.

I can't help but feel like adding more asserts to specific procs would guard against them being abused and expose logic that is not quite right.

Also, the check for the longer standing bug is just a patch. It's probably fragile.

Graveflo avatar Jun 29 '24 13:06 Graveflo

Although this is kinda obvious, I should mention what is happening:

In typeRel's tyArray branch the range of the array is processed via inferStaticsInRange. In that proc, families of functions like lengthOrd, lastOrd, and eventually getOrdValue are called that work with Int128 types. There was a comment in getOrdValue warning that the current error handling scheme (returning high(Int128)) was probably a bad idea due to overflow concerns. inferStaticsInRange takes the Int128 and converts it with toInt64, which from an erroneous call to getOrdValue is not compatible, triggering the assertion error from the issue.

As I said before, I think there is something else wrong with this besides avoiding the overflow. Maybe the node structure is not what is expected.

Graveflo avatar Jun 29 '24 17:06 Graveflo

If this passes the CI, it seems like the workaround can be removed from inferStaticsInRange as well (at least for the givin test cases). I'll probably be asleep by the time it finishes though

Graveflo avatar Jun 30 '24 02:06 Graveflo

Even after the fixes from this PR, there is still an Error: fatal error: invalid kind for lastOrd(tyGenericParam) in an example that @tersec and I are trying to minimize and make self-contained. I've bisected the problem, and the root cause seems to be the same: https://github.com/nim-lang/Nim/pull/23137

Don't know if this stops the merge of this PR, or if it will be fixed in some future PR.

narimiran avatar Jul 01 '24 12:07 narimiran

I'm merging it already as the change is a definite improvement.

Araq avatar Jul 01 '24 12:07 Araq

Here is a self-contained example that worked before https://github.com/nim-lang/Nim/pull/23137 and it fails with Error: fatal error: invalid kind for lastOrd(tyGenericParam) after it (including this PR):

func `xor`[T: array](a, b: T): T =
  for i in 0..<result.len:
    result[i] = a[i] xor b[i]

template eachElement(x, y, res, op: untyped) =
  for i in 0..<res.len:
    res[i] = op(x[i], y[i])

func `xor`*[N: static int; T](x, y: array[N, T]): array[N, T] =
  eachElement(x, y, result, `xor`)

var a: array[5, int]
var b: array[5, int]

discard a xor b

narimiran avatar Jul 01 '24 12:07 narimiran

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

Hint: mm: orc; opt: speed; options: -d:release 179051 lines; 8.471s; 664.711MiB peakmem

github-actions[bot] avatar Jul 01 '24 12:07 github-actions[bot]

Here is a self-contained example that worked before #23137 and it fails with Error: fatal error: invalid kind for lastOrd(tyGenericParam) after it (including this PR):

func `xor`[T: array](a, b: T): T =
  for i in 0..<result.len:
    result[i] = a[i] xor b[i]

template eachElement(x, y, res, op: untyped) =
  for i in 0..<res.len:
    res[i] = op(x[i], y[i])

func `xor`*[N: static int; T](x, y: array[N, T]): array[N, T] =
  eachElement(x, y, result, `xor`)

var a: array[5, int]
var b: array[5, int]

discard a xor b

Is anyone looking into fixing the error from the above example? Should I open a separate issue for it?

EDIT: Ok, I created a new issue for it: https://github.com/nim-lang/Nim/issues/23823

narimiran avatar Jul 10 '24 11:07 narimiran