Skip to content

Add warning for scope of exec#47

Open
Eddy114514 wants to merge 3 commits intosoftdevteam:migrationfrom
Eddy114514:execscope_warn
Open

Add warning for scope of exec#47
Eddy114514 wants to merge 3 commits intosoftdevteam:migrationfrom
Eddy114514:execscope_warn

Conversation

@Eddy114514
Copy link
Copy Markdown
Collaborator

exec behaves differently in Python 2 and Python 3 inside function scope. In Python 2, a plain exec statement can write back into the enclosing function's local variables. In Python 3, exec() does not reliably write back to function locals unless an explicit locals mapping is provided.

Example:
In Python 2
def f(): b = 42 exec "b = 99" return b
return 99

In Python 3
def f(): b = 42 exec("b = 99") return b
return 42 as exec didn't update local b.

In order to allow exec to modify local b. A safer version will be
def g(): b = 42 ns = {"b": b} exec("b = 99", globals(), ns) return ns["b"]

Thus, in order to warn about such scope issue. I mainly modified cevel.c that

  1. Before exec runs, snapshot the current fast locals.
  2. After exec, compare fast locals and record which local slots changed.
  3. If one of those locals is later read (LOAD_FAST), emit the warning.
  4. If it is overwritten or deleted before being read, clear the pending state and do not warn.

Comment thread Python/ceval.c Outdated
Comment thread Python/ceval.c Outdated
Comment thread Python/ceval.c Outdated
Comment thread Python/ceval.c Outdated
Comment thread Python/ceval.c Outdated
Comment thread Python/ceval.c Outdated
Comment thread Python/ceval.c Outdated
@ltratt
Copy link
Copy Markdown
Member

ltratt commented Apr 21, 2026

This mostly looks OK at a high-level (except the code around track_locals which needs explaining, because it's not obvious) and I have some low-level comments. I also don't know if CPython prefers explicitly saying if (x == NULL) or the terser (and valid, exactly equivalent) if (x). If the latter, the PR can be simplified further.

@Eddy114514
Copy link
Copy Markdown
Collaborator Author

@ltratt Thank you for your comments, I have fixed the low levels issues. For the track_loclas, I renamed it track_exec_writeback as it is a variable to controls whether we snapshot and later compare fast locals around a plain exec, so we only record bindings that exec actually wrote back. I also added a short comment explaining that path.

For the case of if(x = null) vs if (x), for what I observed in cevel.c, it tend to use the if(x=null).

@ltratt
Copy link
Copy Markdown
Member

ltratt commented Apr 23, 2026

Thanks! Please squash.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants