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

regression from 1.6/2.0.4 to version-2-0/devel instantiating nested generic objects generic function #23568

Closed
tersec opened this issue May 4, 2024 · 3 comments · Fixed by #23571

Comments

@tersec
Copy link
Contributor

tersec commented May 4, 2024

Description

type G[T] = object
  j: T
proc s[T](u: int) = discard
proc s[T]() = discard
proc c(e: int | int): G[G[G[int]]] = s[G[G[int]]]()
discard c(0)

Nim Version

Builds:

Nim Compiler Version 1.6.21 [Linux: amd64]
Compiled at 2024-05-04
Copyright (c) 2006-2023 by Andreas Rumpf

git hash: 6a63df181f71635538dd29aed0365b663800016e
active boot switches: -d:release
Nim Compiler Version 2.0.4 [Linux: amd64]
Compiled at 2024-05-04
Copyright (c) 2006-2023 by Andreas Rumpf

git hash: b47747d31844c6bd9af4322efe55e24fefea544c
active boot switches: -d:release

Does not bulid:

Nim Compiler Version 2.0.5 [Linux: amd64]
Compiled at 2024-05-04
Copyright (c) 2006-2023 by Andreas Rumpf

git hash: 526e48b2ed2ee3e9dad9d61c1f3a96444225ebc4
active boot switches: -d:release
Nim Compiler Version 2.1.1 [Linux: amd64]
Compiled at 2024-05-04
Copyright (c) 2006-2024 by Andreas Rumpf

git hash: 1ef4d04a1e56f67d85559a7964e9467df06bc2d7
active boot switches: -d:release

Current Output

/root/v.nim(6, 10) template/generic instantiation of `c` from here
/root/v.nim(5, 41) Error: cannot instantiate G [type declared in /root/v.nim(1, 6)]
got: <T>
but expected: <T>

Expected Output

2.0.5 and devel also build this

Possible Solution

No response

Additional Information

No response

@tersec tersec changed the title regression from 1.6/2.0 to version-2-0/devel instantiating nested generic objects generic function regression from 1.6/2.0.4 to version-2-0/devel instantiating nested generic objects generic function May 4, 2024
@narimiran
Copy link
Member

Bisecting on version-2-0 branch says it is this commit: d436728, which was recently backported.

For 2.0.x, we can revert the backport, but ideally: we would have a proper fix in devel, which then I can backport.

cc @metagn @arnetheduck

@metagn
Copy link
Collaborator

metagn commented May 4, 2024

Should be the same issue as #23310, I just realized what the issue is, semtypes calls semGenericStmt in type sections which does the genericsOpenSym behavior which it shouldn't do, we should probably only enable it for the first pass of generic procs by a flag through semGenericStmt into GenericsCtx, should be easy

@metagn
Copy link
Collaborator

metagn commented May 4, 2024

I was wrong, the culprit is this diff: d436728#diff-539da3a63df08fa987f1b0c67d26cdc690753843d110b6bf0805a685eeaffd40R1028-R1031

Reverting these lines fixes the 2.0 branch but breaks devel, I'll look into fixing devel with it

metagn added a commit to metagn/Nim that referenced this issue May 4, 2024
arnetheduck added a commit to status-im/nimbus-eth2 that referenced this issue May 5, 2024
* Revert "revert to v2.0.4"

This reverts commit fc3fad2.

* chronos: bump

Maybe work around nim-lang/Nim#23568
jakubgs added a commit to status-im/nimbus-eth2 that referenced this issue May 6, 2024
Mitigation for:
nim-lang/Nim#23568

Signed-off-by: Jakub Sokołowski <jakub@status.im>
jakubgs added a commit to status-im/nimbus-eth2 that referenced this issue May 6, 2024
Mitigation for:
nim-lang/Nim#23568

Signed-off-by: Jakub Sokołowski <jakub@status.im>
tersec pushed a commit to status-im/nimbus-eth2 that referenced this issue May 6, 2024
Mitigation for:
nim-lang/Nim#23568

Signed-off-by: Jakub Sokołowski <jakub@status.im>
Araq pushed a commit that referenced this issue 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 issue 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants