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

lambdalifting: ignore AST literals for var capture #697

Conversation

saem
Copy link
Collaborator

@saem saem commented May 9, 2023

Summary

nkNimNodeLit are no longer scanned during var capture detection,
ensuring that literals do not alter capture analysis of a routine
holding typed literal AST with symbols from the outer routine.

Details

Capture detection is done in two modules lambdalifting and sempass2
in the detectCapturedVars and detectCapture procedures,
respectively. With this change both procedures do not traverse into
nkNimNodeLit (treating them as more opaque literal data) for literal
analysis. A regression tests has been added a test for this (thanks to
Zerbina).

`nkNimNodeLit` are no longer scanned during `detectCapturedVars`,
ensuring that literals do not alter capture analysis of a routine
holding typed literal AST with symbols from the outer routine.
@saem saem added the compiler/sem Related to semantic-analysis system of the compiler label May 9, 2023
Copy link
Collaborator

@zerbina zerbina left a comment

Choose a reason for hiding this comment

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

Thanks. liftCapturedVars also needs to skip nkNimNodeLit nodes, but otherwise looks good to me.

As for a test case, the following snippet could be used:

proc sym() =
  discard

proc outer() {.compileTime.} =
  var x = 0

  proc sym() {.compileTime.} =
    # close over an outer local in order to make `sym` a closure procedure:
    x = 0

  proc inner() {.compileTime.} =
    let x = bindSym"sym" # create a symbol choice that includes the closure
                         # procedure

  # `inner` doesn't close over a local and also doesn't access a closure
  # procedure, so it must not be a closure procedure
  doAssert inner isnot "closure"

static:
  # call the procedure to make sure it passes the lambda-lifting pass
  outer()

Unfortunately, sempass2.detectCaptures also doesn't skip nkNimNodeLit nodes, so the test would fail even with the changes to lambdalifting.

compiler/sem/lambdalifting.nim Outdated Show resolved Hide resolved
@saem saem added the bug Something isn't working label May 11, 2023
@saem
Copy link
Collaborator Author

saem commented May 11, 2023

/merge

@github-actions
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 added this pull request to the merge queue May 11, 2023
Merged via the queue into nim-works:devel with commit 3b80b24 May 11, 2023
@saem saem deleted the saem-lambdalifting-capture-detection-ignores-nknimnodelit branch May 11, 2023 20:32
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
None yet
Development

Successfully merging this pull request may close these issues.

2 participants