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

Wrong destructors injected for phantom types #24374

Closed
darkestpigeon opened this issue Oct 28, 2024 · 1 comment · Fixed by #24425
Closed

Wrong destructors injected for phantom types #24374

darkestpigeon opened this issue Oct 28, 2024 · 1 comment · Fixed by #24425

Comments

@darkestpigeon
Copy link

darkestpigeon commented Oct 28, 2024

Description

Destructors of wrong type are injected for phantom types (with generic parameters that don't appear in the type's fields).

Here is a toy example:

type Phantom[T] = object
  value: int
  # markerField: T

proc initPhantom[T](value: int): Phantom[T] =
  doAssert value >= 0
  echo "created " & $Phantom[T] & " with value " & $value
  result = Phantom[T](value: value)

proc `=wasMoved`[T](x: var Phantom[T]) =
  x.value = -1

proc `=destroy`[T](x: Phantom[T]) =
  if x.value >= 0:
    echo "destroyed " & $Phantom[T] & " with value " & $x.value


let
  x = initPhantom[float](1)
  y = initPhantom[string](2)
  z = initPhantom[byte](3)

Nim Version

Nim Compiler Version 2.2.0 [Linux: amd64]
Compiled at 2024-10-02
Copyright (c) 2006-2024 by Andreas Rumpf

git hash: 78983f1876726a49c69d65629ab433ea1310ece1
active boot switches: -d:release
Nim Compiler Version 2.2.1 [Linux: amd64]
Compiled at 2024-10-27
Copyright (c) 2006-2024 by Andreas Rumpf

git hash: 24aa92c14f2301d1ef63f59bc4ae2ec31a64f9c5
active boot switches: -d:release

Current Output

created Phantom[system.float] with value 1
created Phantom[system.string] with value 2
created Phantom[system.byte] with value 3
destroyed Phantom[system.float] with value 3
destroyed Phantom[system.float] with value 2
destroyed Phantom[system.float] with value 1

Expected Output

created Phantom[system.float] with value 1
created Phantom[system.string] with value 2
created Phantom[system.byte] with value 3
destroyed Phantom[system.byte] with value 3
destroyed Phantom[system.string] with value 2
destroyed Phantom[system.float] with value 1

Known Workarounds

The problem disappears if the field markerField: T is uncommented.

Additional Information

No response

@darkestpigeon darkestpigeon changed the title Incorrect type inference on destructor injection Wrong destructors injected for phantom types Oct 30, 2024
@metagn
Copy link
Collaborator

metagn commented Oct 30, 2024

Related/same #22479

@Araq Araq closed this as completed in 05c74d6 Nov 16, 2024
narimiran pushed a commit that referenced this issue 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
Labels
None yet
Projects
None yet
2 participants