sem generic proc param types like generic types + static instantiation fixes #24005
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fixes nim-lang#8406, fixes nim-lang#8551, refs nim-lang#8545, refs nim-lang#22607
|
Thanks for your hard work on this PR! Hint: mm: orc; opt: speed; options: -d:release |
This was referenced Aug 26, 2024
metagn
added a commit
to metagn/Nim
that referenced
this pull request
Aug 27, 2024
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.
metagn
added a commit
to metagn/Nim
that referenced
this pull request
Aug 31, 2024
Araq
pushed a commit
that referenced
this pull request
Sep 2, 2024
fixes #16376 The way the compiler handled generic proc instantiations in calls (like `foo[int](...)`) up to this point was to instantiate `foo[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](https://github.com/nim-lang/Nim/blob/b7b1313d21deb687adab2b4a162e716ba561a26b/compiler/semexprs.nim#L3366-L3371)). This has several problems: * Templates and macros can't create instantiated symbols, so they couldn't participate in overloaded explicit generic instantiations, causing the issue #16376. * Every single proc that can be instantiated with the given generic params is fully instantiated including the body. #9997 is about this but isn't fixed here since the instantiation isn't in a call. The way overload resolution handles explicit instantiations by itself is also buggy: * It doesn't check constraints. * It allows only partially providing the generic parameters, which makes sense for implicit generics, but can cause ambiguity in overloading. Here is how this PR deals with these problems: * Overload resolution now always handles explicit generic instantiations in calls, in `initCandidate`, as long as the symbol resolves to a routine symbol. * Overload resolution now checks the generic params for constraints and correct parameter count (ignoring implicit params). If these don't match, the entire overload is considered as not matching and not instantiated. * Special error messages are added for mismatching/missing/extra generic params. This is almost all of the diff in `semcall`. * Procs with matching generic parameters now instantiate only the type of the signature in overload resolution, not the proc itself, which also works for templates and macros. 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 in `semtypinst` though for subscripts in calls. Additional changes: * Overloading of `[]` 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. * Tests can now enable the new type mismatch errors with just `-d:testsConciseTypeMismatch` in the command. Package PRs: - using fork for now: [combparser](PMunch/combparser#7) (partial generic instantiation) - merged: [cligen](c-blake/cligen#233) (partial generic instantiation but non-overloaded + template) - merged: [neo](andreaferretti/neo#56) (trying to instantiate template with no generic param)
Araq
pushed a commit
that referenced
this pull request
Sep 3, 2024
fixes #24044 When matching a `tyFromExpr` against a `static` generic parameter, `paramTypesMatch` tries to evaluate it as a constant expression, which causes a segfault in the case of #24044. In #24005 a consequence of the same behavior was an issue where `nkStaticExpr` was created for `tyFromExpr` which made it not instantiate, so only the generation of `nkStaticExpr` was disabled. Instead we now just completely ignore `tyFromExpr` matching a `static` generic parameter in generic contexts and keep it untouched.
This was referenced Sep 5, 2024
metagn
added a commit
to metagn/Nim
that referenced
this pull request
Sep 7, 2024
This was referenced Sep 7, 2024
Araq
pushed a commit
that referenced
this pull request
Sep 8, 2024
I noticed after #24005 the auto-reported boot time in PRs increased from around 8 seconds to 8.8 seconds, but I wasn't sure what could cause a performance problem that made the compiler itself compile slower, most of the changes were related to `static` which the compiler code doesn't use too often. So I figured it was unrelated. However there is still a performance problem with the changes to `tryReadingGenericParam`. If an expression like `a.b` doesn't match any of the default dot field behavior (for example, is actually a call `b(a)`), the compiler does a final check to see if `b` is a generic parameter of `a`. Since #24005, if the type of `a` is not `tyGenericInst` or an old concept type, the compiler does a full traversal of the type of `a` to see if it contains a generic type, only then checking for `c.inGenericContext > 0` to not return `nil`. This happens on *every* dot call. Instead, we now check for `c.inGenericContext > 0` first, only then checking if it contains a generic type, saving performance by virtue of `c.inGenericContext > 0` being both cheap and less commonly true. The `containsGenericType` could also be swapped out for more generic type kind checks, but I think this is incorrect even if it might pass CI.
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.
Araq
pushed a commit
that referenced
this pull request
Sep 9, 2024
Caught in https://github.com/metagn/applicates, I'm not sure which commit causes this but it's also in the 2.0 branch (but not 2.0.2), so it's not any recent PRs. If a proc has a static parameter with type `static Foo[T]`, then another parameter with type `static Bar[T, U]`, the generic instantiation for `Bar` doesn't match `U` which has type `tyGenericParam`, but matches `T` since it has type `tyTypeDesc`. The reason is that `concreteType` returns the type itself for `tyTypeDesc` if `c.isNoCall` (i.e. matching a generic invocation), but returns `nil` for `tyGenericParam`. I'm guessing `tyGenericParam` is received here because of #22618, but that doesn't explain why `T` is still `tyTypeDesc`. I'm not sure. Regardless, we can just copy the behavior for `tyTypeDesc` to `tyGenericParam` and also return the type itself when `c.isNoCall`. This feels like it defeats the purpose of `concreteType` but the way it's used doesn't make sense without it (generic param can't match another generic param?). Alternatively we could loosen the `if concrete == nil: return isNone` checks in some places for specific conditions, whether `c.isNoCall` or `c.inGenericContext == 0` (though this would need #24005).
Araq
pushed a commit
that referenced
this pull request
Sep 11, 2024
fixes regression remaining after #24092 In #24092 `prepareNode` was updated so it wouldn't try to instantiate generic type symbols (like `Generic` when `type Generic[T] = object`, and `prepareNode` is what `tyFromExpr` uses in most of the compiler. An exception is in sigmatch, which is now changed to use `prepareNode` to make generic type symbols work in the same way as usual. However this requires another change to work: Dot fields and matches to `typedesc` on generic types generate `tyFromExpr` in generic contexts since #24005, including generic type symbols. But this means when we try to instantiate the `tyFromExpr` in sigmatch, which increases `c.inGenericContext` for potentially remaining unresolved expressions, dotcalls stay as `tyFromExpr` and so never match. To fix this, we change the "generic type" check in dot fields and `typedesc` matching to an "unresolved type" check which excludes generic body types; and for generic body types, we only generate `tyFromExpr` if the dot field is a generic parameter of the generic type (so that it gets resolved only at instantiation). Notes for the future: * Sigmatch shouldn't have to `inc c.inGenericContext`, if a `tyFromExpr` can't instantiate it's fine if we just fail the match (i.e. redirect the instantiation errors from `semtypinst` to a match failure). Then again maybe this is the best way to check for inability to instantiate. * The `elif c.inGenericContext > 0 and t.containsUnresolvedType` check in dotfields could maybe be simplified to just checking for `tyFromExpr` and `tyGenericParam`, but I don't know if this is an exhaustive list.
Araq
pushed a commit
that referenced
this pull request
Sep 29, 2024
This was referenced Oct 5, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 tocast[bool]), refs #22342 and #22607 (disabled tests added), succeeds #23194Parameter and return type nodes in generic procs now undergo the same
inGenericContexttreatment 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 insigmatchand type instantiations.tryReadingGenericParamno longer treatstyCompositeTypeClasslike a concrete type anymore, so expressions likeFoo.TwhereFoois a generic type don't look for a parameter ofFooin non-generic code anymore. It also doesn't generatetyFromExprin non-generic code for any generic LHS. This is to handle a very specific case inasyncmacrowhich usedFutureVar.astToStrwhereFutureVaris generic.tryResolvingStaticExprcall when matchingtyFromExprin sigmatch now doesn't consider call nodes in general unresolved, only nodes withtyFromExprtype, which is emitted on unresolved expressions by increasingc.inGenericContext.c.inGenericContext == 0is also now required to attempt instantiatingtyFromExpr. So matching againsttyFromExprin proc signatures works in general now, but I'm speculating it depends on constant folding insemExprfor statics to match against it properly.paramTypesMatchnow doesn't try to change nodes withtyFromExprtype intotyStatictype 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 generic types parameter asserts the compiler (#2) #22647 test doesn't work with it).tyStaticmatching now usesinferStaticParaminstead of just range type matching, soFoo[N div 2]can inferNin the same wayarray[N div 2, int]can.inferStaticParamalso disabled itself if the inferred static param type already had a node, butmakeStaticExprgenerates 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.tyStaticinsemtypinstwas a no-op, now it acts similarly to instantiating any other type with the following differences:containsGenericTypeis 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 ifevalConstExpremitted a flag. This is purely for optimization though.semConstExpris called on the value node ifnot cl.allowMetaTypesand the type isn't literally astatictype. Then the type of the node is set to the base type of the static type to deal withsemConstExprstripping abstract types.We need to do this because calls like
foo(N)whereNisstatic intandfoo's first parameter is justintdo not generatetyFromExpr, they are fully typed and somakeStaticExpris called on them, giving a static type with an unresolved node.