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

open scope for each when nimvm branch #24306

Draft
wants to merge 3 commits into
base: devel
Choose a base branch
from

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Oct 14, 2024

fixes #23687, fixes #23688, refs #13450

Package PRs:

@metagn
Copy link
Collaborator Author

metagn commented Oct 14, 2024

Every case caught in package CI seems correctly failing. Things like this were already not possible but this is maybe still slightly annoying:

when nimvm:
  let a = 123
else:
  let a = 456

Araq pushed a commit that referenced this pull request Nov 5, 2024
ref #24225
related #24306

> Code in branches must not affect semantics of the code that follows
the
`when nimvm` statement. E.g. it must not define symbols that are used in
  the following code.

The test shouldn't have passed when
#24306
would be implemented somehow. Some third packages have already misused
`when nimvm` by defining symbols in the other branch of `when nimvm`.

e.g. in status-im/nim-unittest2#34

```nim
when nimvm:
  discard
else:
  let suiteName {.inject.} = nameParam

use(suiteName)
```
narimiran pushed a commit that referenced this pull request Jan 14, 2025
ref #24225
related #24306

> Code in branches must not affect semantics of the code that follows
the
`when nimvm` statement. E.g. it must not define symbols that are used in
  the following code.

The test shouldn't have passed when
#24306
would be implemented somehow. Some third packages have already misused
`when nimvm` by defining symbols in the other branch of `when nimvm`.

e.g. in status-im/nim-unittest2#34

```nim
when nimvm:
  discard
else:
  let suiteName {.inject.} = nameParam

use(suiteName)
```

(cherry picked from commit c71de10)
@metagn metagn closed this Feb 15, 2025
@metagn metagn reopened this Feb 15, 2025
@metagn metagn changed the title test opening scope for each when nimvm branch open scope for each when nimvm branch Feb 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant