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

fix C compilation errors with closures and nested types #1397

Merged

Conversation

zerbina
Copy link
Collaborator

@zerbina zerbina commented Aug 4, 2024

Summary

Fix a bug with hook synthesis that led to C compiler errors when using
some closure procedures or refs of non-top-level object type.

Details

  • sighashes now uses the symbol ID as the object representation for
    anonymous object/enum types and object/enum types not defined at the
    top-level
  • this means that hashType(x) != hashType(y) when
    sameType(x, y) == false, for object and enum types
  • as a consequence, unique hook procedures are used for ref T types
    that used the aforementioned types for T
  • RTTI, which is assigned to types based on type hashes, also uses
    unique instances per object type now

Multiple same-shaped anonymous/non-top-level object types are not
merged into a single one by the C code generator, which due to all
sharing the same set of hook procedures, resulted in C compiler errors
due to implicit conversions between incompatible pointer types.

Summary
=======

Fix a bug with hook synthesis that led to C compiler errors when using
some closure procedures or `ref`s with non-top-level `object` types.

Details
=======

* `sighashes` now uses the symbol ID as the object representation for
  anonymous object/enum types and object/enum types not defined at the
  top-level
* this means that `hashType(x) != hashType(y)` when
  `sameType(x, y) == false`, for object and enum types
* as a consequence, unique hook procedures are used for `ref T` types
  that used the aforementioned types for `T`
* RTTI, which is assigned to types based on type hashes, also uses
  unique instances per object type now

Multiple same-shaped anonymous/non-top-level object types are *not*
merged into a single one by the C code generator, which due to all
sharing the same set of hook procedures, resulted in C compiler errors
due to implicit conversions between incompatible pointer types.
@zerbina zerbina added bug Something isn't working compiler/sem Related to semantic-analysis system of the compiler labels Aug 4, 2024
Copy link
Collaborator

@saem saem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

compiler/sem/sighashes.nim Outdated Show resolved Hide resolved
Comment on lines +167 to +169
if t.sym.flags * {sfAnon, sfGenSym} != {} or
(t.kind == tyObject and t.owner.kind == skType and
tfRefsAnonObj in t.owner.typ.flags):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the top level check wouldn't it make sense to check if the owning symbol is not an skModule, or would that be a problem for generic instantiations?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing whether the owner is an skModule symbol wouldn't be able to detect the Nested type from tseparate_hooks1.nim (where the owner is a module symbol).

sem marks such types' symbols with the sfGenSym at the moment, so that they can be detected here. The only proper solution to this mess is getting rid of sighashes, really.

Co-authored-by: Saem Ghani <[email protected]>
@saem
Copy link
Collaborator

saem commented Aug 4, 2024

/merge

Copy link

github-actions bot commented Aug 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

@chore-runner chore-runner bot enabled auto-merge August 4, 2024 22:07
@chore-runner chore-runner bot added this pull request to the merge queue Aug 4, 2024
Merged via the queue into nim-works:devel with commit f5d8ce9 Aug 4, 2024
31 checks passed
@zerbina zerbina deleted the unique-hash-for-all-object-types branch August 13, 2024 21:03
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/sem Related to semantic-analysis system of the compiler
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants