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.

Sign up for GitHub

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

Jump to bottom

try all call kinds in generic type node handling #23411

Closed
wants to merge 20 commits into from
Closed

try all call kinds in generic type node handling #23411

wants to merge 20 commits into from

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Mar 15, 2024 •
edited
Loading

fixes #23406

Apparently it was intentional that this only checked nkCall but maybe things have changed since, we can see what CI says.

Also n.kind in nkCallKinds is implied already, probably just c.inGenericContext > 0 is enough.

Temporary note: ttypetraits needs to be non-joinable because of unrelated type name changing

@metagn
Copy link
Collaborator Author

metagn commented Mar 15, 2024

The problem is only encountered in CI with -> so whitelisting nkCommand will probably not break anything. Someone could depend on the difference outside of CI though and checking the node type is still weird. Basically semGenericStmt doesn't capture generic parameter symbols? I think this is the minimal case for tclosure:

template foo(x: untyped): untyped = x

type Foo[A] = foo(A) # `foo A` or just `A` works

proc bar[A](x: Foo[A]) = discard

bar(123) # undeclared identifier: A

Doesn't matter if foo is entirely untyped or not.

Normally in seminst each generic parameter is added manually to scope so the generic parameters not being captured in semGenericStmt doesn't matter. In semtypinst however when typing tyFromExpr nodes the generic parameters are not in scope. I don't know if semtypinst is even aware of generic parameter symbols outside of the binding map, I don't know how generic parameter symbols even enter the scope for regular semtypes behavior.

@metagn
Copy link
Collaborator Author

metagn commented Mar 21, 2024 •
edited
Loading

We should probably remove this if branch altogether and use only the else branch, then fix #23406 for semTypeExpr.

@metagn metagn mentioned this pull request Aug 19, 2024
Open
@metagn metagn mentioned this pull request Aug 19, 2024
Closed
@metagn
Copy link
Collaborator Author

metagn commented Aug 19, 2024

I wanted to split this into multiple PRs for each of the fixes before finally merging all the call kinds but merging the call kinds seems minor in comparison. And most of the changes go hand in hand, like the tfNonConstExpr is only required for derefs to function correctly, but it's also better logic overall and reduces the burden on the compiler. I've lost track of some other interactions. I'm also delaying working on #23194 until after these changes.

I'll open a new PR branched off from this one later and try to explain every change to keep things clear.

@metagn metagn mentioned this pull request Aug 20, 2024
Merged
Araq pushed a commit that referenced this pull request Aug 20, 2024
@metagn
sem all call nodes in generic type bodies + many required fixes (#23983)
ab18962 
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
#23406), the behavior for all call kinds is now to call `semTypeExpr`.

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.
@metagn metagn closed this Aug 21, 2024
narimiran pushed a commit that referenced this pull request Sep 16, 2024
@metagn @narimiran
sem all call nodes in generic type bodies + many required fixes (#23983)
a22d52c 
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
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Different symbol resolution in generic types depending on f(x) vs f x call syntax
1 participant
@metagn