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

always reinstantiate nominal values of generic instantiations #24425

Open
wants to merge 11 commits into
base: devel
Choose a base branch
from

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Nov 9, 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 enums1) 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.

Footnotes

  1. See this test 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.

@metagn
Copy link
Collaborator Author

metagn commented Nov 9, 2024

Hits a pre-existing bug:

type
  Foo[T] = object of RootObj
    x: T
  Bar = object of Foo[int]

proc foo(x: Foo) = discard

foo(Bar())

This works if x: T is removed before this PR.

Same for tinvalid_rebind, this compiles but shouldn't:

type Foo[T] = object
  x: T

proc main =
  var f: Foo[int]

proc `=destroy`[T](f: var Foo[T]) =
  # should say: cannot bind another '=destroy' to: Foo
  discard

This one is lower priority though IMO

@metagn metagn changed the title test always instantiating nominal generic insts always reinstantiate nominal values of generic instantiations Nov 10, 2024
@metagn
Copy link
Collaborator Author

metagn commented Nov 11, 2024

Will try to split the fixes for the above pre-existing issues as well as the one in jsonutils into their own PRs, as I don't think they require the main change. But I might be wrong.

Araq pushed a commit that referenced this pull request Nov 12, 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.
Araq pushed a commit that referenced this pull request Nov 12, 2024
split from #24425

Matching `tyGenericBody` performs a match on the last child of the
generic body, in this case the uninstantied `tyObject` type. If the
object contains no generic fields, this ends up being the same type as
all instantiated ones, but if it does, a new type is created. This fails
the `sameObjectTypes` check that subtype matching for object types uses.
To fix this, also consider that the pattern type could be the generic
uninstantiated object type of the matched type in subtype matching.
@metagn
Copy link
Collaborator Author

metagn commented Nov 14, 2024

Did not move the fix for tinvalid_rebind to its own PR since it removes information from the error message, and this PR is already related to destructors. I know not skipping tyGenericInst instead was the intended solution but I can't think of how this wouldn't be beneficial.

@metagn metagn marked this pull request as ready for review November 14, 2024 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong destructors injected for phantom types Wrong instance of the generic destructor is called
1 participant