delay resolved procvar check for proc params + acknowledge unresolved statics #23188
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
metagn
commented
Jan 9, 2024
| @@ -1,5 +1,6 @@ | |||
| discard """ | |||
| targets: "c cpp js" | |||
| # just tests that this doesn't crash the compiler | |||
| errormsg: "cannot instantiate: 'a:type'" | |||
There was a problem hiding this comment.
This test only compiled before because a was never used, if you tried to use it you would get this same error. I have a stashed diff that gets this test working and lets you actually use a (except cast[static[bool]](a) which makes no sense) but this PR is already crowded. (edit: #23194)
Araq
reviewed
Jan 10, 2024
| @@ -34,7 +34,8 @@ block: #15622 | |||
| proc test1[T](a: T, b: static[string] = "") = discard | |||
| test1[int64](123) | |||
| proc test2[T](a: T, b: static[string] = "") = discard | |||
| test2[int64, static[string]](123) | |||
| doAssert not (compiles do: | |||
| test2[int64, static[string]](123)) | |||
There was a problem hiding this comment.
Huh? Previously this worked and now it doesn't? But it should work as you can pass 123 to T = int64. What am I missing?
There was a problem hiding this comment.
The problem is the extra generic parameter static[string], test2 only accepts one generic parameter, and even the implicit generic param which would be the value of b doesn't make sense to be set to literally the type static[string] (which is why this errors now).
|
Thanks for your hard work on this PR! Hint: mm: orc; opt: speed; options: -d:release |
narimiran
pushed a commit
that referenced
this pull request
Apr 19, 2024
… statics (#23188) fixes #23186 As explained in #23186, generics can transform `genericProc[int]` into a call `` `[]`(genericProc, int) `` which causes a problem when `genericProc` is resemmed, since it is not a resolved generic proc. `[]` needs unresolved generic procs since `mArrGet` also handles explicit generic instantiations, so delay the resolved generic proc check to `semFinishOperands` which is intentionally not called for `mArrGet`. The root issue for [t6137](https://github.com/nim-lang/Nim/blob/devel/tests/generics/t6137.nim) is also fixed (because this change breaks it otherwise), the compiler doesn't consider the possibility that an assigned generic param can be an unresolved static value (note the line `if t.kind == tyStatic: s.ast = t.n` below the change in sigmatch), now it properly errors that it couldn't instantiate it as it would for a type param. ~~The change in semtypinst is just for symmetry with the code above it which also gives a `cannot instantiate` error, it may or may not be necessary/correct.~~ Now removed, I don't think it was correct. Still possible that this has unintended consequences. (cherry picked from commit e8092a5)
narimiran
pushed a commit
that referenced
this pull request
May 3, 2024
… statics (#23188) fixes #23186 As explained in #23186, generics can transform `genericProc[int]` into a call `` `[]`(genericProc, int) `` which causes a problem when `genericProc` is resemmed, since it is not a resolved generic proc. `[]` needs unresolved generic procs since `mArrGet` also handles explicit generic instantiations, so delay the resolved generic proc check to `semFinishOperands` which is intentionally not called for `mArrGet`. The root issue for [t6137](https://github.com/nim-lang/Nim/blob/devel/tests/generics/t6137.nim) is also fixed (because this change breaks it otherwise), the compiler doesn't consider the possibility that an assigned generic param can be an unresolved static value (note the line `if t.kind == tyStatic: s.ast = t.n` below the change in sigmatch), now it properly errors that it couldn't instantiate it as it would for a type param. ~~The change in semtypinst is just for symmetry with the code above it which also gives a `cannot instantiate` error, it may or may not be necessary/correct.~~ Now removed, I don't think it was correct. Still possible that this has unintended consequences. (cherry picked from commit e8092a5)
Araq
pushed a commit
that referenced
this pull request
May 8, 2024
fixes #23568, fixes #23310 In #23091 `semFinishOperands` was changed to not be called for `mArrGet` and `mArrPut`, presumably in preparation for #23188 (not sure why it was needed in #23091, maybe they got mixed together), since the compiler handles these later and needs the first argument to not be completely "typed" since brackets can serve as explicit generic instantiations in which case the first argument would have to be an unresolved generic proc (not accepted by `finishOperand`). In this PR we just make it so `mArrGet` and `mArrPut` specifically skip calling `finishOperand` on the first argument. This way the generic arguments in the explicit instantiation get typed, but not the unresolved generic proc.
narimiran
pushed a commit
that referenced
this pull request
May 8, 2024
fixes #23568, fixes #23310 In #23091 `semFinishOperands` was changed to not be called for `mArrGet` and `mArrPut`, presumably in preparation for #23188 (not sure why it was needed in #23091, maybe they got mixed together), since the compiler handles these later and needs the first argument to not be completely "typed" since brackets can serve as explicit generic instantiations in which case the first argument would have to be an unresolved generic proc (not accepted by `finishOperand`). In this PR we just make it so `mArrGet` and `mArrPut` specifically skip calling `finishOperand` on the first argument. This way the generic arguments in the explicit instantiation get typed, but not the unresolved generic proc. (cherry picked from commit 09bd9d0)
This was referenced Jun 18, 2024
Araq
pushed a commit
that referenced
this pull request
Jun 18, 2024
…3731) fixes #23730 Since #23188 the compiler errors when matching a type variable to an uninstantiated static value. However sometimes an uninstantiated static value is given even when only a type match is being performed to the base type of the static type, in the given issue this case is: ```nim proc foo[T: SomeInteger](x: T): int = int(x) proc bar(x: static int): array[foo(x), int] = discard discard bar(123) ``` To deal with this issue we only error when matching against a type variable constrained to `static`. Not sure if the `q.typ.kind == tyGenericParam and q.typ.genericConstraint == tyStatic` check is necessary, the code above for deciding whether the variable becomes `skConst` doesn't use it.
narimiran
pushed a commit
that referenced
this pull request
Jun 24, 2024
…3731) fixes #23730 Since #23188 the compiler errors when matching a type variable to an uninstantiated static value. However sometimes an uninstantiated static value is given even when only a type match is being performed to the base type of the static type, in the given issue this case is: ```nim proc foo[T: SomeInteger](x: T): int = int(x) proc bar(x: static int): array[foo(x), int] = discard discard bar(123) ``` To deal with this issue we only error when matching against a type variable constrained to `static`. Not sure if the `q.typ.kind == tyGenericParam and q.typ.genericConstraint == tyStatic` check is necessary, the code above for deciding whether the variable becomes `skConst` doesn't use it. (cherry picked from commit 128090c)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #23186
As explained in #23186, generics can transform
genericProc[int]into a call`[]`(genericProc, int)which causes a problem whengenericProcis resemmed, since it is not a resolved generic proc.[]needs unresolved generic procs sincemArrGetalso handles explicit generic instantiations, so delay the resolved generic proc check tosemFinishOperandswhich is intentionally not called formArrGet.The root issue for t6137 is also fixed (because this change breaks it otherwise), the compiler doesn't consider the possibility that an assigned generic param can be an unresolved static value (note the line
if t.kind == tyStatic: s.ast = t.nbelow the change in sigmatch), now it properly errors that it couldn't instantiate it as it would for a type param.The change in semtypinst is just for symmetry with the code above it which also gives aNow removed, I don't think it was correct.cannot instantiateerror, it may or may not be necessary/correct.Still possible that this has unintended consequences.