fixes #23755; array static inference during overload resolution

[Graveflo]

#23755

[mratsim]

We should add both test cases you mentioned in #23755 (comment). If it's intentional because you will address the first one later, carry on.

[Graveflo]

We should add both test cases you mentioned in #23755 (comment). 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]

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]

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.

[metagn]

fRange.kind == tyArray doesn't make sense, I think the lines above are wrong:

if prev == nil:
  put(c, fRange, a.indexType)
  fRange = a

fRange = a should be fRange = a.indexType, then we can just check if fRange.kind == tyGenericParam.

[Graveflo]

I'm of the opinion that the whole tyArray branch of typeRel needs to be revisited, not just this part. I didn't try to mess with what was already there. Sorry this PR is so patchy. It would take me a while to understand how the statics are packed into the node structure.

[Graveflo]

And to be clear, I did look at that briefly and fRange = a makes zero sense to me at all, so if you have an idea about that you are further along then me.

[Graveflo]

wait, something just clicked for me. fRange is being set to a because if it has no binding it's assumed that it can accept a, so that is the substitution. What if the generic parameter has constraints though? I think that might be another error. Just because the range is a generic type variable doesn't mean it accepts any type. That's an unconstrained generic type variable.

[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

[metagn]

I think an alternative to this would be to change the code below:

if fRange.rangeHasUnresolvedStatic:
  return inferStaticsInRange(c, fRange, a)

to:

if fRange.rangeHasUnresolvedStatic:
  if not aRange.rangeHasUnresolvedStatic:
    return inferStaticsInRange(c, fRange, a)
  else:
    return isNone

The issue with the ord value failing is that f and a both have unresolved statics (which is supposed to happen, checkGeneric in cmpCandidates compares generic overloads to each other), and the inferStaticsInRange tries to evaluate unresolved statics in a to assign to the unresolved statics in f and crashes because the unresolved statics don't have values. In this case calling inferStaticsInRange at all doesn't make sense, we can deal with it early on by checking if a has unresolved statics.

This change compiles the test but idk if it'll break anything in CI in general.

[Graveflo]

As per my comment above the test cases that bring us to this point are avoided by the extra call to typeRel from the "try this" push. If that is a good sanity check then maybe it's worth trying it out anyway. Maybe run the CI with the

proc view(a: Limbs) = discard
proc view(a: var Limbs) = discard

overload scenario on top of devel to see if it's worth merging before this PR?