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

fix calls in generic bodies, delay typecheck when no overloads match #22029

Merged
merged 11 commits into from
Jun 13, 2023

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Jun 6, 2023

fixes #19916, fixes #20514, fixes #20937, fixes #19284, fixes #19365, refs 7fdf299

In generic bodies, the compiler hard-compiles calls in array/range/when static expression parts, including with generic parameter arguments which cannot reasonably be evaluated and have to be special cased in some places in the compiler (example in semLowHigh). There was no such special casing for calls in sigmatch/semcall, instead there was some weird workaround involving not instantiating generic calls in generic bodies which breaks a lot of code that should work.

This workaround is now removed and generic calls are always instantiated. In order for this to work, special casing has also been added to sigmatch/semcall so that calls with unresolved generic parameters don't match typedesc parameters of procs, and calls with no matching overloads delay their typechecking to instantiation time.

tyGenericParam is added to types.isCompileTimeOnly to make T; U: static T work, due to how static T behaves the same as static foo() and just converts to a static type when the expression returns a typedesc. Note that static[T] is not affected and worked before.

There is was also a new untyped foo() syntax (inspired by the "untyped escape hatch" in nim-lang/RFCs#168) to stop hard compilation in generic bodies and delay to instantiation time when the compiler can't delay it by itself. But this isn't needed currently in this PR and can be detached from it (although it might be useful to work around future bugs, like (U + 1) * 2 not working). Update: It was detached from the PR.

@@ -250,7 +250,7 @@ proc replaceTypeVarsN(cl: var TReplTypeVars, n: PNode; start=0): PNode =
result = newNodeI(nkRecList, n.info)
of nkStaticExpr:
var n = prepareNode(cl, n)
n = reResolveCallsWithTypedescParams(cl, n)
#n = reResolveCallsWithTypedescParams(cl, n)
Copy link
Member

Choose a reason for hiding this comment

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

Oh yes, that is sweet!

@@ -1500,7 +1503,7 @@ proc compatibleEffects*(formal, actual: PType): EffectsCompat =


proc isCompileTimeOnly*(t: PType): bool {.inline.} =
result = t.kind in {tyTypeDesc, tyStatic}
result = t.kind in {tyTypeDesc, tyStatic, tyGenericParam}
Copy link
Collaborator Author

@metagn metagn Jun 11, 2023

Choose a reason for hiding this comment

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

This change exists in this PR for the sole purpose of making T; U: static T work (though might be more correct anyway). In any case the compiler treats static T the same as static foo() and evaluates it as a constant expression which returns static[T]. static[T] on the other hand goes through semStaticType in semtypes which doesn't have a reason to fail with generic params.

One might think to add elif op.id == ord(wStatic): result = semStaticType(...) here instead, but this might not be correct and could break some code and I don't want to complicate this PR.

Edit: I believe this also fixes #15760, will add test case doesn't

@metagn
Copy link
Collaborator Author

metagn commented Jun 11, 2023

The main issue here is that the compiler hard-compiles calls in array/range/when (not tested) static expression parts, including with generic parameter arguments which cannot reasonably be evaluated and have to be special cased in some places in the compiler (example in semLowHigh). This special casing is not properly performed for calls in sigmatch/semcall, instead there is some weird workaround involving not instantiating generic calls which breaks a lot of code that should work.

The reason statics work without this workaround is we already know their type when they're constrained, i.e. when T: static int we can compile T + 1 early without problem. When they're not constrained, like T; U: static T, the same problem is there.

The new untyped foo() syntax prevents this hard compilation, but the user has to think to use it without any good suggestions, and the compiler has feigned automatically detecting it so far so maybe it should properly do it. This untyped also doesn't seem to always work with the old workaround.

Right now I am not able to either make the compiler smarter on the broken cases or make the call to use nkStaticExpr on every call node in generic range types and lose information people might depend on/performance. For this reason this behavior is now opt-in, but currently this is done with an experimental switch which won't work if it's turned on at the type declaration but not at the site of the instantiation. Maybe it can be attached to the type instead, i.e. Foo[T] {.instantiateCalls.} = ..., or this is inferred in a context with the experimental feature turned on. Broken cases work

@metagn metagn changed the title sacrifice "tgenericshardcases" for working statics fix calls in generic bodies, delay typecheck when no overloads match Jun 12, 2023
@metagn
Copy link
Collaborator Author

metagn commented Jun 12, 2023

OK I now made it so:

  1. unresolved generic parameters can't be passed to typedesc arguments (sigmatch change)
  2. calls without matching overloads get a tyFromExpr type instead of erroring in generic contexts (semcall change, with required followups in semexprs)

This accomplishes "making the compiler smarter" to make the old cases still work. So we don't need any opting in anymore.

This also means the untyped feature is not needed in the scope of this proc, but might be needed to workaround bugs not covered here down the line (just realized (U + 1) * 2 is one such bug). I can either remove it to maybe put in another PR later maybe with an RFC (it's a small change), or keep it and document it etc (without making much noise).

@metagn metagn marked this pull request as ready for review June 12, 2023 22:55
@Araq
Copy link
Member

Araq commented Jun 13, 2023

I can either remove it to maybe put in another PR

What would this removal look like?

@metagn
Copy link
Collaborator Author

metagn commented Jun 13, 2023

This diff from semexprs gets removed:

   of tyUntyped: 
     if c.inGenericContext > 0: 
       # could add `and efDetermineType in flags`, but no flags param 
       result = n[1] 
       result.typ = makeTypeFromExpr(c, result.copyTree) 
       return 
     else: 
       localError(c.config, n.info, "illegal type conversion to '$1'" % typeToString(targetType)) 
   of tyAnything, tyTyped:

And it's used once in a test which is a copy of another test without it. That's it.

@Araq
Copy link
Member

Araq commented Jun 13, 2023

So remove it please.

@metagn
Copy link
Collaborator Author

metagn commented Jun 13, 2023

Done

@Araq Araq merged commit 894a19c into nim-lang:devel Jun 13, 2023
@github-actions
Copy link
Contributor

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

Hint: mm: orc; opt: speed; options: -d:release
167854 lines; 12.710s; 610.969MiB peakmem

metagn added a commit to metagn/Nim that referenced this pull request Jun 16, 2023
@metagn metagn mentioned this pull request Jun 16, 2023
Araq pushed a commit that referenced this pull request Jun 16, 2023
* add test to close #7209

was fixed by #22029

* fix echo => doAssert
metagn added a commit to metagn/Nim that referenced this pull request Aug 17, 2023
Araq pushed a commit that referenced this pull request Aug 17, 2023
narimiran pushed a commit that referenced this pull request Apr 17, 2024
fixes #22490, fixes #22491, adapts #22029 to type conversions

(cherry picked from commit 98c39e8)
narimiran pushed a commit that referenced this pull request Apr 17, 2024
fixes #22490, fixes #22491, adapts #22029 to type conversions

(cherry picked from commit 98c39e8)
Araq pushed a commit that referenced this pull request Jul 20, 2024
fixes #19819, fixes #23339

Since #22029 `tyFromExpr` does not match anything in overloading, so
generic bodies can know which call expressions to delay until the type
can be evaluated. However generic type invocations also run overloading
to check for generic constraints even in generic bodies. To prevent them
from failing early from the overload not matching, pretend that
`tyFromExpr` matches. This mirrors the behavior of the compiler in more
basic cases like:

```nim
type
  Foo[T: int] = object
    x: T
  Bar[T] = object
    y: Foo[T]
```

Unfortunately this case doesn't respect the constraint (#21181, some
other bugs) but `tyFromExpr` should easily use the same principle when
it does.
Araq pushed a commit that referenced this pull request Aug 17, 2024
fixes #10753, fixes #22021, refs #19365 (was fixed by #22029, but more
faithful test added)

For whatever reason `compileTime` proc calls did not fold if the proc
was generic ([since this folding was
introduced](c25ffbf#diff-539da3a63df08fa987f1b0c67d26cdc690753843d110b6bf0805a685eeaffd40)).
I'm guessing the intention was for *unresolved* generic procs to not
fold, which is now the logic.

Non-magic `compileTime` procs also now don't fold at compile time in
`typeof` contexts to avoid possible runtime errors (only the important)
and prevent double/needless evaluation.
Araq pushed a commit that referenced this pull request Aug 20, 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
#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.
Araq pushed a commit that referenced this pull request Aug 26, 2024
…n fixes (#24005)

fixes #4228, fixes #4990, fixes #7006, fixes #7008, fixes #8406, fixes
#8551, fixes #11112, fixes #20027, fixes #22647, refs #23854 and #23855
(remaining issue fixed), refs #8545 (works properly now with
`cast[static[bool]]` changed to `cast[bool]`), refs #22342 and #22607
(disabled tests added), succeeds #23194

Parameter and return type nodes in generic procs now undergo the same
`inGenericContext` treatment that nodes in generic type bodies do. This
allows many of the fixes in #22029 and followups to also apply to
generic proc signatures. Like #23983 however this needs some more
compiler fixes, but this time mostly in `sigmatch` and type
instantiations.

1. `tryReadingGenericParam` no longer treats `tyCompositeTypeClass` like
a concrete type anymore, so expressions like `Foo.T` where `Foo` is a
generic type don't look for a parameter of `Foo` in non-generic code
anymore. It also doesn't generate `tyFromExpr` in non-generic code for
any generic LHS. This is to handle a very specific case in `asyncmacro`
which used `FutureVar.astToStr` where `FutureVar` is generic.
2. The `tryResolvingStaticExpr` call when matching `tyFromExpr` in
sigmatch now doesn't consider call nodes in general unresolved, only
nodes with `tyFromExpr` type, which is emitted on unresolved expressions
by increasing `c.inGenericContext`. `c.inGenericContext == 0` is also
now required to attempt instantiating `tyFromExpr`. So matching against
`tyFromExpr` in proc signatures works in general now, but I'm
speculating it depends on constant folding in `semExpr` for statics to
match against it properly.
3. `paramTypesMatch` now doesn't try to change nodes with `tyFromExpr`
type into `tyStatic` type when fitting to a static type, because it
doesn't need to, they'll be handled the same way (this was a workaround
in place of the static type instantiation changes, only one of the
fields in the #22647 test doesn't work with it).
4. `tyStatic` matching now uses `inferStaticParam` instead of just range
type matching, so `Foo[N div 2]` can infer `N` in the same way `array[N
div 2, int]` can. `inferStaticParam` also disabled itself if the
inferred static param type already had a node, but `makeStaticExpr`
generates static types with unresolved nodes, so we only disable it if
it also doesn't have a binding. This might not work very well but the
static type instantiation changes should really lower the amount of
cases where it's encountered.
5. Static types now undergo type instantiation. Previously the branch
for `tyStatic` in `semtypinst` was a no-op, now it acts similarly to
instantiating any other type with the following differences:
- Other types only need instantiation if `containsGenericType` is true,
static types also get instantiated if their value node isn't a literal
node. Ideally any value node that is "already evaluated" should be
ignored, but I'm not sure of a better way to check this, maybe if
`evalConstExpr` emitted a flag. This is purely for optimization though.
- After instantiation, `semConstExpr` is called on the value node if
`not cl.allowMetaTypes` and the type isn't literally a `static` type.
Then the type of the node is set to the base type of the static type to
deal with `semConstExpr` stripping abstract types.
We need to do this because calls like `foo(N)` where `N` is `static int`
and `foo`'s first parameter is just `int` do not generate `tyFromExpr`,
they are fully typed and so `makeStaticExpr` is called on them, giving a
static type with an unresolved node.
Araq pushed a commit that referenced this pull request Aug 28, 2024
…#24018)

updated version of #22193

After #22029 and the followups #23983 and #24005 which fixed issues with
it, `tyFromExpr` no longer match any proc params in generic type bodies
but delay all non-matching calls until the type is instantiated.
Previously the mechanism `fauxMatch` was used to pretend that any
failing match against `tyFromExpr` actually matched, but prevented the
instantiation of the type until later.

Since this mechanism is not needed anymore for `tyFromExpr`, it is now
only used for `tyError` to prevent cascading errors and changed to a
bool field for simplicity. A change in `semtypes` was also needed to
prevent calling `fitNode` on default param values resolving to type
`tyFromExpr` in generic procs for params with non-generic types, as this
would try to coerce the expression into a concrete type when it can't be
instantiated yet.

The aliases `tyProxy` and `tyUnknown` for `tyError` and `tyFromExpr` are
also removed for uniformity.
Araq pushed a commit that referenced this pull request Sep 6, 2024
fixes #16700, fixes #20916, refs #24010

Fixes the instantiation issues for proc param default values encountered
in #24010 by:

1. semchecking generic default param values with `inGenericContext` for
#22029 and followups to apply (the bigger change in semtypes),
2. rejecting explicit generic instantiations with unresolved generic
types inside `inGenericContext` (sigmatch change),
3. instantiating the default param values using `prepareNode` rather
than an insufficient manual method (the bigger change in seminst).

This had an important side effect of references to other parameters not
working since they would be resolved as a symbol belonging to the
uninstantiated original generic proc rather than the later instantiated
proc. There is a more radical way to fix this which is generating ident
nodes with `tyFromExpr` in specifically this context, but instead we
just count them as belonging to the same proc in
`hoistParamsUsedInDefault`.

Other minor bugfixes:

* To make the error message in t20883 make sense, we now give a "cannot
instantiate" error when trying to instantiate a proc generic param with
`tyFromExpr`.
* Object constructors as default param values generated default values
of private fields going through `evalConstExpr` more than once, but the
VM doesn't mark the object fields as `nfSkipFieldChecking` which gives a
"cannot access field" error. So the VM now marks object fields it
generates as `nfSkipFieldChecking`. Not sure if this affects VM
performance, don't see why it would.
* The nkRecWhen changes in #24042 didn't cover the case where all
conditions are constantly false correctly, this is fixed with a minor
change. This isn't needed for this PR now but I encountered it after
forgetting to `dec c.inGenericContext`.
Araq pushed a commit that referenced this pull request Sep 6, 2024
fixes #22342, fixes #22607

Another followup of #22029, `when` expressions in general in generic
type bodies now behave like `nkRecWhen` does since #24042, leaving them
as `tyFromExpr` if a condition is uncertain. The tests for the issues
were originally added but left disabled in #24005.
Araq pushed a commit that referenced this pull request Sep 8, 2024
fixes #15959

Another followup of #22029 and #24005, subscript expressions now
recognize when their parameters are generic types, then generating
tyFromExpr. `typeof` also now properly sets `tfNonConstExpr` to make it
usable in proc signatures. `lent` with brackets like `lent[T]` is also
now allowed.
narimiran pushed a commit that referenced this pull request Sep 13, 2024
fixes #10753, fixes #22021, refs #19365 (was fixed by #22029, but more
faithful test added)

For whatever reason `compileTime` proc calls did not fold if the proc
was generic ([since this folding was
introduced](c25ffbf#diff-539da3a63df08fa987f1b0c67d26cdc690753843d110b6bf0805a685eeaffd40)).
I'm guessing the intention was for *unresolved* generic procs to not
fold, which is now the logic.

Non-magic `compileTime` procs also now don't fold at compile time in
`typeof` contexts to avoid possible runtime errors (only the important)
and prevent double/needless evaluation.

(cherry picked from commit f7c11a8)
narimiran pushed a commit that referenced this pull request 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)
Araq pushed a commit that referenced this pull request Nov 6, 2024
fixes issue described in https://forum.nim-lang.org/t/12579

In #24065 explicit generic parameter matching was made to fail matches
on arguments with unresolved types in generic contexts (the sigmatch
diff, following #24010), similar to what is done for regular calls since
#22029. However unlike regular calls, a failed match in a generic
context for a standalone explicit generic instantiation did not convert
the expression into one with `tyFromExpr` type, which means it would
error immediately given any unresolved parameter. This is now done to
fix the issue.

For explicit generic instantiations on single non-overloaded symbols, a
successful match is still instantiated. For multiple overloads (i.e.
symchoice), if any of the overloads fail the match, the entire
expression is considered untyped and any instantiations are not used, so
as to not void overloads that would match later. This means even
symchoices without unresolved arguments aren't instantiated, which may
be too restrictive, but it could also be too lenient and we might need
to make symchoice instantiations always untyped. The behavior for
symchoice is not sound anyway given it causes #9997 so this is something
to consider for a redesign.

Diff follows #24276.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment