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

js: support variant destructors #1267

Merged

Conversation

zerbina
Copy link
Collaborator

@zerbina zerbina commented Apr 3, 2024

Summary

Fix destructors not being properly executed for fields within case
objects when changing the active branch, and using the JS backend.

Details

The destructor produced by produceDestructorForDiscriminator took a
reference to the discriminant field and used pointer arithmetic to
compute the address of the enclosing object. Not only is this
unnecessarily complex, it also doesn't work with the JS backend.

Instead, the destructor now takes a mutable reference to the
discriminant field's enclosing object type -- the MIR emitted for the
call is adjusted accordingly.

Since the destructor definitely modifies the object, ekMutate is used
instead of ekInvalidate for the parameter passing. This results in a
small move analyzer regression:

var x = Object()
var y = x.fieldOutsideOfVariant # not automatically moved anymore
x.kind = ... # potential branch switch that requires destruction

Finally, a test is added for ensuring proper destruction when changing
the active branch.

zerbina added 3 commits April 3, 2024 23:19
It doesn't rely on using `--gc:arc`, and thus works, in theory, across
all backends.
* don't rely on pointer arithmetic
* instead, pass a mutable reference to the full object
* the switch statement lowering is adjusted accordingly
* `ekInvalidate` only makes sense prior to the lowering, after the
  lowering, `ekMutate` is now used for the destructor argument

Removing the need for pointer arithmetic makes variant destructors
work with the JS backend, and it's also simpler.
@zerbina zerbina added bug Something isn't working compiler/backend Related to backend system of the compiler labels Apr 3, 2024
@zerbina zerbina added this to the MIR phase milestone Apr 3, 2024
@saem
Copy link
Collaborator

saem commented Apr 4, 2024

/merge

Copy link

github-actions bot commented Apr 4, 2024

Merge requested by: @saem

Contents after the first section break of the PR description has been removed and preserved below:


Notes for Reviewers

  • fixing the regression requires moving the mnkSwitch lowering to a separate pass, which is then run after the move-optimization pass
  • fixing the test for the VM is doable, but requires some changes to the lowerBranchSwitch pass

@chore-runner chore-runner bot added this pull request to the merge queue Apr 4, 2024
Merged via the queue into nim-works:devel with commit ae54312 Apr 4, 2024
31 checks passed
@zerbina zerbina deleted the js-support-variant-destructors branch April 8, 2024 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working compiler/backend Related to backend system of the compiler
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants