Skip to content
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

delay resolved procvar check for proc params acknowledge unresolved statics #23188

Merged
merged 6 commits into from
Jan 11, 2024

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Jan 8, 2024

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 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.

@@ -1,5 1,6 @@
discard """
targets: "c cpp js"
# just tests that this doesn't crash the compiler
errormsg: "cannot instantiate: 'a:type'"
Copy link
Collaborator Author

@metagn metagn Jan 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@@ -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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

@metagn metagn Jan 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@Araq Araq merged commit e8092a5 into nim-lang:devel Jan 11, 2024
19 checks passed
Copy link
Contributor

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

Hint: mm: orc; opt: speed; options: -d:release
177693 lines; 7.634s; 768.105MiB peakmem

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)
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression from 2.0 to devel with macros
2 participants