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

Different symbol resolution in generic types depending on f(x) vs f x call syntax #23406

Closed
SirNickolas opened this issue Mar 14, 2024 · 1 comment · Fixed by #23983
Closed

Comments

@SirNickolas
Copy link
Contributor

Description

When a generic type is defined as a routine invocation, it—surprisingly—matters whether the call has parentheses or not.

template helper(_: untyped): untyped =
  int

type # Each of them should always be `int`.
  GenA[T] = helper int
  GenB[T] = helper(int)
  GenC[T] = helper helper(int)

block:
  template helper(_: untyped): untyped =
    float

  type
    A = GenA[int]
    B = GenB[int]
    C = GenC[int]

  assert A is int # OK.
  assert B is int # Fails; it is `float`!
  assert C is int # OK.

Nim Version

Nim Compiler Version 2.0.2 [Linux: amd64]
Compiled at 2023-12-15
Copyright (c) 2006-2023 by Andreas Rumpf

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

Current Output

Error: unhandled exception: /home/nickolas/c/asynciters/a.nim(19, 3) `B is int`  [AssertionDefect]

Expected Output

No response

Possible Solution

No response

Additional Information

It seems to be irrelevant whether helper has untyped, typed, or type parameter / return value. As I see, the only thing that matters is whether the command-call syntax is used for the top-level call.

This bug has been existing since at least 0.19—that’s the oldest Nim I have on my system to check.

@metagn
Copy link
Collaborator

metagn commented Mar 15, 2024

Culprit is probably:

if c.inGenericContext > 0 and n.kind == nkCall:

Should be n.kind in nkCallKinds, but apparently this was intentional at the time this line was added. Maybe things are different now though. Worst case we just whitelist nkCommand too.

metagn added a commit to metagn/Nim that referenced this issue Mar 15, 2024
metagn added a commit to metagn/Nim that referenced this issue Aug 16, 2024
metagn added a commit to metagn/Nim that referenced this issue Aug 19, 2024
metagn added a commit to metagn/Nim that referenced this issue Aug 20, 2024
@Araq Araq closed this as completed in ab18962 Aug 20, 2024
narimiran pushed a commit that referenced this issue Sep 16, 2024
fixes #23406, closes #23854, closes #23855 (test code of both compiles
but separate issue exists), refs #23432, follows #23411

In generic bodies, previously all regular `nkCall` nodes like `foo(a,
b)` were directly treated as generic statements and delayed immediately,
but other call kinds like `a.foo(b)`, `foo a, b` etc underwent
typechecking before making sure they have to be delayed, as implemented
in #22029. Since the behavior for `nkCall` was slightly buggy (as in

However the vast majority of calls in generic bodies out there are
`nkCall`, and while there isn't a difference in the expected behavior,
this exposes many issues with the implementation started in #22029 given
how much more code uses it now. The portion of these issues that CI has
caught are fixed in this PR but it's possible there are more.

1. Deref expressions, dot expressions and calls to dot expressions now
handle and propagate `tyFromExpr`. This is most of the changes in
`semexprs`.
2. For deref expressions to work in `typeof`, a new type flag
`tfNonConstExpr` is added for `tyFromExpr` that calls `semExprWithType`
with `efInTypeof` on the expression instead of `semConstExpr`. This type
flag is set for every `tyFromExpr` type of a node that `prepareNode`
encounters, so that the node itself isn't evaluated at compile time when
just trying to get the type of the node.
3. Unresolved `static` types matching `static` parameters is now treated
the same as unresolved generic types matching `typedesc` parameters in
generic type bodies, it causes a failed match which delays the call
instantiation.
4. `typedesc` parameters now reject all types containing unresolved
generic types like `seq[T]`, not just generic param types by themselves.
(using `containsGenericType`)
5. `semgnrc` now doesn't leave generic param symbols it encounters in
generic type contexts as just identifiers, and instead turns them into
symbol nodes. Normally in generic procs, this isn't a problem since the
generic param symbols will be provided again at instantiation time (and
in fact creating symbol nodes causes issues since `seminst` doesn't
actually instantiate proc body node types).
But generic types can try to be instantiated early in `sigmatch` which
will give an undeclared identifier error when the param is not provided.
Nodes in generic types (specifically in `tyFromExpr` which should be the
only use for `semGenericStmt`) undergo full generic type instantiation
with `prepareNode`, so there is no issue of these symbols remaining as
uninstantiated generic types.
6. `prepareNode` now has more logic for which nodes to avoid
instantiating.
Subscripts and subscripts turned into calls to `[]` by `semgnrc` need to
avoid instantiating the first operand, since it may be a generic body
type like `Generic` in an expression like `Generic[int]`.
Dot expressions cannot instantiate their RHS as it may be a generic proc
symbol or even an undeclared identifier for generic param fields, but
have to instantiate their LHS, so calls and subscripts need to still
instantiate their first node if it's a dot expression.
This logic still isn't perfect and needs the same level of detail as in
`semexprs` for which nodes can be left as "untyped" for overloading/dot
exprs/subscripts to handle, but should handle the majority of cases.

Also the `efDetermineType` requirement for which calls become
`tyFromExpr` is removed and as a result `efDetermineType` is entirely
unused again.

(cherry picked from commit ab18962)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants