-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fixes #23755; array static inference during overload resolution #23760
Conversation
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. |
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 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. |
Although this is kinda obvious, I should mention what is happening: In 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. |
compiler/sigmatch.nim
Outdated
@@ -1279,7 +1282,10 @@ proc typeRel(c: var TCandidate, f, aOrig: PType, | |||
result = isGeneric | |||
else: | |||
result = typeRel(c, ff, aa, flags) | |||
|
|||
|
|||
if fRange.kind == tyArray and fRange.indexType.kind == tyGenericParam: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
If this passes the CI, it seems like the workaround can be removed from |
compiler/sigmatch.nim
Outdated
var err = false | ||
let lenOrd = getOrdValueAux(concrete.n[1], err) | ||
if err: | ||
return isNone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
f81c014
to
2934734
Compare
Even after the fixes from this PR, there is still an Don't know if this stops the merge of this PR, or if it will be fixed in some future PR. |
I'm merging it already as the change is a definite improvement. |
Here is a self-contained example that worked before #23137 and it fails with 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 |
Thanks for your hard work on this PR! Hint: mm: orc; opt: speed; options: -d:release |
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: #23823 |
#23755