fix jsonutils macro with generic case object #24429
Merged
Conversation
This file contains hidden or 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
metagn
changed the title
Contributor
|
Thanks for your hard work on this PR! Hint: mm: orc; opt: speed; options: -d:release |
Araq
pushed a commit
that referenced
this pull request
Nov 16, 2024
fixes #22479, fixes #24374, depends on #24429 and #24430 When instantiating generic types which directly have nominal types (object, distinct, ref/ptr object but not enums[^1]) as their values, the nominal type is now copied (in the case of ref objects, its child as well) so that it receives a fresh ID and `typeInst` field. Previously this only happened if it contained any generic types in its structure, as is the case for all other types. This solves #22479 and #24374 by virtue of the IDs being unique, which is what destructors check for. Technically types containing generic param fields work for the same reason. There is also the benefit that the `typeInst` field is correct. However issues like #22445 aren't solved because the compiler still uses structural object equality checks for inheritance etc. which could be removed in a later PR. Also fixes a pre-existing issue where destructors bound to object types with generic fields would not error when attempting to define a user destructor after the fact, but the error message doesn't show where the implicit destructor was created now since it was only created for another instance. To do this, a type flag is used that marks the generic type symbol when a generic instance has a destructor created. Reusing `tfCheckedForDestructor` for this doesn't work. Maybe there is a nicer design that isn't an overreliance on the ID mechanism, but the shortcomings of `tyGenericInst` are too ingrained in the compiler to use for this. I thought about maybe adding something like `tyNominalGenericInst`, but it's really much easier if the nominal type itself directly contains the information of its generic parameters, or at least its "symbol", which the design is heading towards. [^1]: See [this test](https://github.com/nim-lang/Nim/blob/21420d8b0976dc034feb90ab2878ae0dd63121ae/lib/std/enumutils.nim#L102) in enumutils. The field symbols `b0`/`b1` always have the uninstantiated type `B` because enum fields don't expect to be generic, so no generic instance of `B` matches its own symbols. Wouldn't expect anyone to use generic enums but maybe someone does.
narimiran
pushed a commit
that referenced
this pull request
Dec 17, 2024
split from #24425 The added test did not work previously. The result of `getTypeImpl` is the uninstantiated AST of the original type symbol, and the macro attempts to use this type for the result. To fix the issue, the provided `typedesc` argument is used instead. (cherry picked from commit 45e21ce)
narimiran
pushed a commit
that referenced
this pull request
Dec 20, 2024
split from #24425 The added test did not work previously. The result of `getTypeImpl` is the uninstantiated AST of the original type symbol, and the macro attempts to use this type for the result. To fix the issue, the provided `typedesc` argument is used instead. (cherry picked from commit 45e21ce)
narimiran
pushed a commit
that referenced
this pull request
Jan 14, 2025
split from #24425 The added test did not work previously. The result of `getTypeImpl` is the uninstantiated AST of the original type symbol, and the macro attempts to use this type for the result. To fix the issue, the provided `typedesc` argument is used instead. (cherry picked from commit 45e21ce)
narimiran
pushed a commit
that referenced
this pull request
Jan 14, 2025
fixes #22479, fixes #24374, depends on #24429 and #24430 When instantiating generic types which directly have nominal types (object, distinct, ref/ptr object but not enums[^1]) as their values, the nominal type is now copied (in the case of ref objects, its child as well) so that it receives a fresh ID and `typeInst` field. Previously this only happened if it contained any generic types in its structure, as is the case for all other types. This solves #22479 and #24374 by virtue of the IDs being unique, which is what destructors check for. Technically types containing generic param fields work for the same reason. There is also the benefit that the `typeInst` field is correct. However issues like #22445 aren't solved because the compiler still uses structural object equality checks for inheritance etc. which could be removed in a later PR. Also fixes a pre-existing issue where destructors bound to object types with generic fields would not error when attempting to define a user destructor after the fact, but the error message doesn't show where the implicit destructor was created now since it was only created for another instance. To do this, a type flag is used that marks the generic type symbol when a generic instance has a destructor created. Reusing `tfCheckedForDestructor` for this doesn't work. Maybe there is a nicer design that isn't an overreliance on the ID mechanism, but the shortcomings of `tyGenericInst` are too ingrained in the compiler to use for this. I thought about maybe adding something like `tyNominalGenericInst`, but it's really much easier if the nominal type itself directly contains the information of its generic parameters, or at least its "symbol", which the design is heading towards. [^1]: See [this test](https://github.com/nim-lang/Nim/blob/21420d8b0976dc034feb90ab2878ae0dd63121ae/lib/std/enumutils.nim#L102) in enumutils. The field symbols `b0`/`b1` always have the uninstantiated type `B` because enum fields don't expect to be generic, so no generic instance of `B` matches its own symbols. Wouldn't expect anyone to use generic enums but maybe someone does. (cherry picked from commit 05c74d6)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
split from #24425
The added test did not work previously. The result of
getTypeImplis the uninstantiated AST of the original type symbol, and the macro attempts to use this type for the result. To fix the issue, the providedtypedescargument is used instead.