Python: a spliced template's context reaches the file it lands in - #8790
Open
knutwannheden wants to merge 6 commits into
Open
knutwannheden wants to merge 6 commits into
knutwannheden wants to merge 6 commits into
Conversation
A template's `context` parsed the template and nothing else, so a recipe splicing `subprocess.run(...)` under `context=["import subprocess"]` emitted code the target file never imported: it parsed, printed, and raised NameError when run. Recipes shipped that way, with docstrings showing an import the code never emitted. `apply()` now takes the visitor as well as its cursor, and binds each module the context declares in the file being edited — reusing the file's own name where it already binds the module, adding an import where it does not, and leaving alone context the template does not reference. Passing only the cursor raises, since that path cannot reach the file's imports. Also fixes `AddImport._is_referenced`, which looked for the last segment of a dotted module; `import a.b.c` binds `a`.
…taken name Review of the previous commit found three ways the wiring could still emit code that reads the wrong object. A context binding now counts only where the template's own code reads it, judged per occurrence: a name the template binds for itself — a comprehension variable, a parameter — is not the context's, so it is neither renamed nor imported. That also drops context that exists only to type a capture before it can collide with a file that spells the same name. Where the name the spliced code would read is held by something else, applying raises instead of emitting the reference: a scope at the splice site binding it, or a module-scope import of another module under it. Neither is reachable by any import, and the second silently rebinds what the file already reads. `apply()` accepts any `TreeVisitor`, the cursor and after-visit list being all it needs, so a non-`PythonVisitor` no longer falls into the cursor branch. `PythonAddImportVisitor.isReferenced` gets the dotted-module fix its Python half already had; the two are one predicate across the RPC boundary.
Merging a member into a file's existing import of that module is AddImport's, but the alias it merges under comes from the context binding.
A file importing lazily — `def f(): import subprocess` — binds the name where the splice lands, so the refusal for a shadowed name was firing on code the splice would have read correctly. A scope between the splice and the module now satisfies the binding when what it imports is what the context names, and the module scope is left alone; anything else it binds still refuses, since no import reaches past it. Both refusals name what the template reads through the name — the module, or a member of it — rather than reporting that 'subprocess' is not 'subprocess', and the alias they suggest takes the shape of the import it would replace.
A recipe can splice where it is not standing — rewriting a statement of the file it visits, or assembling several results before formatting the subtree once — and until now that meant choosing between the splice site and the imports, since passing the visitor took its cursor for both. `apply(at, visitor=self)` names them separately. Which names are in scope is then read at the splice site rather than at the visitor, so a local import covering the template binds it where the two differ.
The first parameter accepted either a cursor or a visitor standing for its own,
which left two spellings for one call and a parameter named for one of the two
things it held. It is a cursor; `visitor=` names the visitor, as TS spells the
same pair in `tryOn(cursor, node, {visitor: this})`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A Python template's
contextparsed the template and nothing else, so a recipe splicing a name the target file did not import emitted code that raisesNameErrorwhen it runs. It parses, it prints, and it reads as correct in a diff: two recipes in a downstream package shipped that way, their docstrings showing an import the code never emitted and their tests asserting the broken output as expected. Both were found by reading.A template's
contextis now also what it needs bound where it lands:Applied to
out = os.popen('ls'), what the file already binds decides what it gets:subprocess.run('ls', shell=True)import subprocessimport subprocess as spsp.run('ls', shell=True)from subprocess import runsubprocess.run('ls', shell=True)import subprocess— a member import does not bind the moduleif TYPE_CHECKING: import subprocesssubprocess.run('ls', shell=True)import subprocess— a conditional import binds nothing at runtimeimport subprocessin the enclosing functionsubprocess.run('ls', shell=True)subprocessin an enclosing scope, bound to anything elseA context binding counts only where the template's own code reads it, judged per occurrence, so a name the template binds for itself (a comprehension variable, a parameter) is neither renamed nor imported, and context that exists only to type a capture stays out of the file entirely. What does get imported joins the file's existing import of the same module —
from subprocess import PopenbecomesPopen, run as r— under whichever name the context or the file aliases it to, and a relative context import stays relative,from . import utilnaming a sibling module rather than the standard library's.visitor=is what reaches the file's imports; the cursor is where the result lands, and the two differ for a recipe splicing somewhere other than where it stands — the scope questions are asked at the cursor. Omitting the visitor still works for a template whose context binds no module —context=["MyType = int"], or no context at all — and raises otherwise, naming the modules and the call to make.Why automatic
TypeScript already treats this as the template's job.
Template.resolveBindings(visitor)binds each module the context declares and returns the local name it actually bound;apply({bindings})renames the template's references to those names; andrewrite(...).tryOn(cursor, node, {visitor: this})does the whole thing once a pattern matches, throwing if it was given neither a visitor nor bindings (templating/rewrite.ts:65). Python has notryOn—apply()is the splice path — so the wiring and the refusal both land there.Where it refuses rather than deconflicts
TS's
maybeBindpicks another name when the one it wants is taken. This has no such machinery, so it raises in the two cases where emitting the reference would read the wrong object: a scope at the splice site binding the name, and a module-scope import binding it to another module — the second would otherwise rebind a name the file already reads.from subprocess import run as _runin the context is the way out of the latter.Reconciling the two import styles — collapsing a spliced
subprocess.run(...)torun(...)where the file already imports the member — is #8791. It belongs to a normalizer that sees the whole file, not to the splice: binding is per member, so a template reading two members of a partly-imported module can be collapsed for neither use or for one.Tests
tests/python/template/test_template_imports.py, one per decision — the table's five rows, the module-scope import that a taken name refuses, context the template never references, a name the template binds for itself, a splice inside a function whose import still lands at module scope, and the cursor-only call that raises.The dotted-module row turned up a second bug, in both halves of one predicate:
AddImport._is_referencedandPythonAddImportVisitor.isReferencedlooked for the last segment of a dotted module, soimport os.pathwas only ever added to a file already using a namepath.import a.b.cbindsa.