-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
handle explicit generic routine instantiations in sigmatch #24010
Conversation
While developing this PR initially I didn't know what to do with partial generic instantiations, i.e. instantiations with missing generic parameters, before I decided to just disallow them and PR packages which had them. This was the comment I wrote about it: There is a problem remaining of what to do with partial generic instantiations. This is in proc doStuff[T;Y](a: SomeInteger, b: Y): Y = discard
assert typeof(doStuff[int](100, 1.0)) is float And a package uses it as evidenced by this error:
It's supposed to emulate doing something like proc foo[I, T](a: array[I, T]) = discard
proc foo[T](x: openarray[T]) = discard
foo[string]([1, 2, 3]) # invalid type kind for lastOrd: string Specifically a test which does The first thing I tried was only allowing non-overloaded procs to be partially instantiated, but the above cases failed. So my question is do we:
The 3rd one seems the most impossible to me, the 2nd one gives us more work of PRing packages, the 1st one shouldn't cause issues but is still weird design, the instant you add an overload it gives an error. On top of 1 and 2 we can also add the Edit: PR'd the package anyway: PMunch/combparser#6, also the original PR that added the test just seems like it made a mistake Edit: Also cligen, seems like a non-overloaded case:
One takeaway is that we can probably still support inferring generic parameters without instantiating them by writing |
b884cdd
to
9a5b07f
Compare
closes #1969, closes #7547, closes #7737, closes #11838, closes #12283, closes #12714, closes #12720, closes #14053, closes #16118, closes #19670, closes #22645 I was going to wait on these but regression tests even for recent PRs are turning out to be important in wide reaching PRs like #24010. The other issues with the working label felt either finnicky (#7385, #9156, #12732, #15247), excessive to test (#12405, #12424, #17527), or I just don't know what fixed them/what the issue was (#16128: the PR link gives a server error by Github, #12457, #12487).
4407e5e
to
a2f8dca
Compare
Thanks for your hard work on this PR! Hint: mm: orc; opt: speed; options: -d:release |
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`.
refs #24010, refs #24125 (comment) The generic mismatch errors added in #24010 made it possible for `nArg` to be `nil` in the error reporting since it checked the call argument list, not the generic parameter list for the mismatching argument node, which causes a segfault. This is fixed by checking the generic parameter list immediately on any generic mismatch error. Also the `typedesc` type is skipped for the value of the generic params since it's redundant and the generic parameter constraints don't have it.
…#24140) refs nim-lang#24010, refs nim-lang#24125 (comment) The generic mismatch errors added in nim-lang#24010 made it possible for `nArg` to be `nil` in the error reporting since it checked the call argument list, not the generic parameter list for the mismatching argument node, which causes a segfault. This is fixed by checking the generic parameter list immediately on any generic mismatch error. Also the `typedesc` type is skipped for the value of the generic params since it's redundant and the generic parameter constraints don't have it.
refs #8064, refs #24010 Error messages for standalone explicit generic instantiations are revamped. Failing standalone explicit generic instantiations now only error after overloading has finished and resolved to the default `[]` magic (this means `[]` can now be overloaded for procs but this isn't necessarily intentional, in #24010 it was documented that it isn't possible). The error messages for failed instantiations are also no longer a simple `cannot instantiate: foo` message, instead they now give the same type mismatch error message as overloads with mismatching explicit generic parameters. This is now possible due to the changes in #24010 that delegate all explicit generic proc instantiations to overload resolution. Old code that worked around this is now removed. `maybeInstantiateGeneric` could maybe also be removed in favor of just `explicitGenericSym`, the `result == n` case is due to `explicitGenericInstError` which is only for niche cases. Also, to cause "ambiguous identifier" error messages when the explicit instantiation is a symchoice and the expression context doesn't allow symchoices, we semcheck the sym/symchoice created by `explicitGenericSym` with the given expression flags. #8064 isn't entirely fixed because the error message is still misleading for the original case which does `values[1]`, as a consequence of #12664.
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.
fixes #16376
The way the compiler handled generic proc instantiations in calls (like
foo[int](...)
) up to this point was to instantiatefoo[int]
, create a symbol for the instantiated proc (or a symchoice for multiple procs excluding ones with mismatching generic param counts), then perform overload resolution on this symbol/symchoice. The exception to this was when the called symbol was already a symchoice node, in which case it wasn't instantiated and overloading was called directly (these lines).This has several problems:
The way overload resolution handles explicit instantiations by itself is also buggy:
Here is how this PR deals with these problems:
initCandidate
, as long as the symbol resolves to a routine symbol.semcall
.Unfortunately we can't entirely remove instantiations because overload resolution can't handle some cases with uninstantiated types even though it's resolved in the binding (see the last 2 blocks in
texplicitgenerics
). There are also some instantiation issues with default params that #24005 didn't fix but I didn't want this to become the 3rd huge generics PR in a row so I didn't dive too deep into trying to fix them. There is still a minor instantiation fix insemtypinst
though for subscripts in calls.Additional changes:
[]
wasn't documented properly, it somewhat is now because we need to mention the limitation that it can't be done for generic procs/types.-d:testsConciseTypeMismatch
in the command.Package PRs: