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

sem: replace addr calls with nkAddr #1465

Merged
merged 4 commits into from
Oct 19, 2024

Conversation

zerbina
Copy link
Collaborator

@zerbina zerbina commented Oct 18, 2024

Summary

In typed AST, address-of operations are now always represented by
nkAddr trees. This simplifies some compiler logic, makes processing
for typed macros easier, and fixes an effect tracking bug with addr.

Details

This is effectively a revert of
nim-lang/Nim#10814.
Not turning calls to mAddr into nkAddr was done to prevent the
unsafe address semantics from being lost, but those no longer exist.

Lowering the call into an nkAddr tree has the benefit that it
simplifies AST analysis and processing, as address-of operation can now
always be detected by PNode.kind == nkAddr. Old code for detecting
mAddr magic calls is removed.

The effect tracking in sempass2 had no special case for mAddr
calls, thus treating them as normal calls, which led to addr(x) being
treated as an indirect invocation of x, when x is of procedural
type. With the mAddr call now being lowered earlier, this is no
longer the case.

This ensures that all address-of operations are represented by `nkAddr`
trees in typed AST. Previously, the calls stayed in the AST as `mAddr`
calls, requiring analysis to also test for those (which not all
analysis did consistently).
There are no more `mAddr` calls in typed AST.
Effect tracking not special casing the `mAddr` call led to `addr x`
being considered an indirect call of `x`. With `mAddr` calls gone, this
is now fixed.
@zerbina zerbina added bug Something isn't working compiler/sem Related to semantic-analysis system of the compiler simplification Removal of the old, unused, unnecessary or un/under-specified language features. labels Oct 18, 2024
@saem
Copy link
Collaborator

saem commented Oct 18, 2024

/merge

Copy link

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 October 18, 2024 22:27
@chore-runner chore-runner bot added this pull request to the merge queue Oct 18, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 18, 2024
@zerbina
Copy link
Collaborator Author

zerbina commented Oct 19, 2024

/merge

The tslow_tables.nim failure is definitely spurious (the timeout probably needs to be increased at some point), and the tinternal_thread_cleanup.nim failure too looks like it is spurious (though I haven't seen the test fail in the past).

I'm giving merging another try.

@chore-runner chore-runner bot added this pull request to the merge queue Oct 19, 2024
Merged via the queue into nim-works:devel with commit 39317bf Oct 19, 2024
35 checks passed
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 simplification Removal of the old, unused, unnecessary or un/under-specified language features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants