Repository navigation
docfx_yaml: materialize move_markdown_pages cwd list per call (mutable-default leak) - #19044
Hisernberg wants to merge 2 commits into
Conversation
The recursion mutates cwd in place (append/pop). With the shared mutable default [], a failure between the append and the pop leaves state in the default list, and every later top-level call then looks in the wrong markdown directory (reproduced: a mid-recursion copy error on one call made the next call skip top-level files entirely). Default to None and materialize a fresh list per top-level call.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request resolves a mutable default argument issue in move_markdown_pages by changing the default value of cwd to None and initializing it inside the function. However, the initialization block was placed before the function's docstring, which prevents Python from recognizing it as a docstring. It is recommended to move this block immediately after the docstring.
| if cwd is None: | ||
| # Materialize a fresh list per top-level call: the recursion below | ||
| # mutates ``cwd`` in place (append/pop), which would otherwise keep | ||
| # state in the shared default across calls. | ||
| cwd = [] |
There was a problem hiding this comment.
In Python, the docstring must be the very first statement in the function body. Placing the if cwd is None: block before the docstring prevents Python from recognizing it as a docstring (i.e., it won't be assigned to __doc__), which can break documentation generation tools like Sphinx. Please move this initialization block to be immediately after the function's docstring.
|
Hi @Hisernberg, Thanks for providing a fix! Please could you sign the CLA? Once it's signed, please mark the PR as |
The if-block before the docstring prevented it from being recognized as the function docstring (spotted by gemini-code-assist review).
|
Moved the |
|
You're welcome! To ensure this fix remains effective and to prevent future regressions, I recommend adding a unit test that simulates a failure during the recursive file processing. This would verify that the |
Description
move_markdown_pages()declarescwd: list[str] | None = []and then mutatescwdin place during recursion (cwd.append(mdfile.name)/cwd.pop()in docfx_yaml/markdown_utils.py). The default[]is therefore a shared list across every top-level call. If an exception escapes between the append and the pop (e.g. a failing file copy mid-recursion), the leftover entry stays in the default list forever, and every later top-level call looks in the wrong markdown directory.Reproduction (local, stubbed sphinx): a mid-recursion
shutil.copyfailure on call 1 makes call 2 skip top-level files entirely (cwdstill holds the leaked subdirectory). With this change the second call processes top-level files normally.Fix
Default
cwdtoNoneand materialize a fresh list per top-level call, matching the existinglist[str] | Noneannotation (the recursion still receives the same shared list, so sub-directory behavior is unchanged).Before: second call after a mid-recursion failure -> top-level
index.mdnever copied (cwd leaked).After: second call works (fresh
cwdper call).AI Generation Disclosure
Developed with AI assistance (agent workflow): found by an AST scan for mutated mutable defaults, reproduced before/after the fix with a functional stub test, and verified.
Release Notes
cwdstate acrossmove_markdown_pagescalls