diff --git a/tests/test_update_shape_add.py b/tests/test_update_shape_add.py index 9f3a5053..eade4e0d 100644 --- a/tests/test_update_shape_add.py +++ b/tests/test_update_shape_add.py @@ -430,6 +430,8 @@ def test_add_refuses_to_overwrite_a_file_the_root_already_has(root, standard): assert result.returncode == 2, result.stdout assert "update-add-would-overwrite" in result.stderr assert ADDED in result.stderr + assert (f"`git show {standard['c2']}:templates/assembly-root/{ADDED}` " + "in a shape clone") in result.stderr, result.stderr assert (root / ADDED).read_text(encoding="utf-8") == own assert pin(root)["commit"].lower() == standard["c1"] assert ADDED not in pin_rows(root) diff --git a/tests/test_update_shape_kept_helper.py b/tests/test_update_shape_kept_helper.py index 92c90cbc..827ce1cd 100644 --- a/tests/test_update_shape_kept_helper.py +++ b/tests/test_update_shape_kept_helper.py @@ -46,15 +46,16 @@ The project's own edit is committed, as a project's is, so `check` reads the edited copy as `both` or `locally-modified`, and `--branch` meets this refusal rather than `update-branch-uncommitted`. The readers are also held, -in process, to the real importers on this checkout, and to what they cannot -tell. Every file is written as BYTES, so no platform's newline reaches a -digest. +in process, to the real importers on this checkout, to what they cannot +tell, and to what Python never runs (#675). Every file is written as BYTES, +so no platform's newline reaches a digest. """ from __future__ import annotations import os import re +import shlex import shutil import subprocess import sys @@ -62,7 +63,8 @@ import pytest -from conftest import ORG, REPO, cloned_just_pushed, git, run_script +from conftest import (ORG, REPO, WINDOWS_SKIP, cloned_just_pushed, git, + run_script) #: The scaffold every update test starts from, and the in-process load of #: `update-shape.py`, imported from the module that owns them; the fixture is #: found by pytest here because it is imported. @@ -914,14 +916,27 @@ def test_what_the_check_cannot_tell_or_is_answered_it_does_not_refuse( b" except ImportError:\n pass\n" b"except ImportError:\n NEW = None\n", id="an inner handler passes, inside one that binds"), + pytest.param(b"try:\n pass\nexcept ImportError:\n NEW = None\n" + b"else:\n from repo_shape import NEW\n", + id="an import in the try's else, outside its guard"), + pytest.param(b"try:\n pass\nexcept ImportError:\n NEW = None\n" + b"finally:\n from repo_shape import NEW\n", + id="an import in the try's finally, outside its guard"), + pytest.param(GUARDED + b" if False:\n NEW = None\n", + id="the handler's fallback under if False"), + pytest.param(GUARDED + b" raise SystemExit\n NEW = None\n", + id="the handler's fallback after its raise"), ]) def test_a_guard_whose_first_handler_binds_no_fallback_still_refuses( update_shape, tmp_path, written): """A `try` that catches the `ImportError` answers a name only where the first handler that catches it binds that same local name itself: the copy's own fallback. A handler that exits, re-raises or passes - leaves the copy stopping there, or on a `NameError` later, and a - `def` in the `try` imports when it is called, outside the handler.""" + leaves the copy stopping there, or on a `NameError` later, and so does + one whose binding never runs (#675). A `def` in the `try` imports when + it is called, outside the handler, and an import in the `try`'s `else` + or `finally` runs outside its guard, where no handler of that `try` + catches it (#675).""" refused = refusal(update_shape, tmp_path, {REPO_SHAPE: b"OLD = 1\n"}, {BOOTSTRAP: written}) assert refused is not None, "nothing the copy runs binds NEW" @@ -974,6 +989,44 @@ def test_a_handler_that_misses_a_missing_name_still_refuses(update_shape, assert "lacks NEW" in refused.detail, refused.detail +@pytest.mark.parametrize("copy, taken", [ + pytest.param(b"if False:\n from repo_shape import NEW\n", False, + id="under if False"), + pytest.param(b"if 0:\n from repo_shape import NEW\n", False, + id="under if 0"), + pytest.param(b"if None:\n from repo_shape import NEW\n", False, + id="under if None"), + pytest.param(b"try:\n raise RuntimeError\n" + b" from repo_shape import NEW\n" + b"except RuntimeError:\n pass\n", False, + id="after a raise in a try body"), + pytest.param(b"raise SystemExit\nfrom repo_shape import NEW\n", False, + id="after a raise at module scope"), + pytest.param(b"if False:\n pass\n" + b"else:\n raise SystemExit\n" + b" from repo_shape import NEW\n", False, + id="after a raise in the else of if False"), + pytest.param(b"if False:\n pass\n" + b"else:\n from repo_shape import NEW\n", True, + id="in the else of if False"), + pytest.param(b"if 1:\n from repo_shape import NEW\n", True, + id="under a true constant"), + pytest.param(b"from repo_shape import NEW\nraise SystemExit\n", True, + id="before a raise"), +]) +def test_an_import_python_never_reaches_is_not_taken(update_shape, copy, + taken): + """`_blocks_inside` (#675): Python starts a copy whose `from repo_shape + import NEW` sits under `if False:`, `if 0:` or `if None:`, or after an + unconditional `raise` in its block (a `try` body, the module's own, the + `else` of `if False:`), whatever `repo_shape.py` defines, so that import + is no name the copy takes, as one under `if TYPE_CHECKING:` was not + already. What does run is still read: the `else` of `if False:`, a + true constant's body, and the statements before a `raise`.""" + imported = update_shape._names_imported(copy).get("repo_shape", set()) + assert ("NEW" in imported) is taken, imported + + @pytest.mark.parametrize("written, how", [ pytest.param({"tools/bootstrap.py": imports("NEW")}, "upstream-changed", id="an importer in another directory"), @@ -1267,15 +1320,86 @@ def test_a_kept_copy_importing_a_name_the_written_module_drops_is_refused( id="a name the bytes on disk lack too"), pytest.param(TAKES_GONE, DEFINES_GONE, DEFINES_GONE + b"OTHER = 1\n", id="a name the written bytes still define"), + pytest.param(b"if False:\n from repo_shape import GONE\n", + DEFINES_GONE, DROPS_GONE, + id="a kept importer's import under if False"), + pytest.param(TAKES_GONE, DEFINES_GONE, + DROPS_GONE + b"if False:\n pass\nelse:\n GONE = 1\n", + id="written bytes binding it in the else of if False"), + pytest.param(TAKES_GONE, DEFINES_GONE, + DROPS_GONE + b"try:\n GONE = 1\n raise RuntimeError\n" + b"except RuntimeError:\n pass\n", + id="written bytes binding it before a raise"), + pytest.param(TAKES_GONE, DEFINES_GONE, + DROPS_GONE + b"while False:\n GONE = 1\n", + id="written bytes binding it under while False, read as " + "running"), ]) def test_what_the_reverse_cannot_tell_or_the_run_keeps_it_does_not_refuse( update_shape, tmp_path, importer, now, after): """Ruling 3 of #604, the forward check's rule: what it cannot tell, it does not refuse, and the validators answer after the writes. And what - this run does not take away is not its doing.""" + this run does not take away is not its doing. A kept import Python + never reaches takes nothing, a binding Python does run is one, and the + rest of what never runs, `while False:` among it, is read as running: + what this cannot tell, it does not refuse (#675).""" assert reverse(update_shape, tmp_path, importer, now, after) is None +def assert_the_written_module_drops_gone(update_shape, root: Path, + binding: bytes) -> None: + """The issue's case (#675): the bytes this run writes to + `repo_shape.py` bind `GONE` only in `binding`, where Python never runs + it, so the kept `bootstrap.py` that takes it is refused, on the line + that says the written module no longer defines it.""" + refused = reverse(update_shape, root, after=DROPS_GONE + binding) + assert refused is not None, "GONE is bound only where Python never runs" + assert dropped_line(BOOTSTRAP, "GONE") in refused.detail.splitlines(), \ + refused.detail + + +@pytest.mark.parametrize("binding", [ + pytest.param(b"from typing import TYPE_CHECKING\n" + b"if TYPE_CHECKING:\n GONE = 1\n", id="if TYPE_CHECKING"), + pytest.param(b"import typing\nif typing.TYPE_CHECKING:\n GONE = 1\n", + id="if typing.TYPE_CHECKING"), + pytest.param(b"if False:\n GONE = 1\n", id="if False"), + pytest.param(b"if 0:\n GONE = 1\n", id="if 0"), + pytest.param(b"if None:\n GONE = 1\n", id="if None"), + pytest.param(b"if KEPT:\n pass\nelif False:\n GONE = 1\n", + id="elif False"), +]) +def test_a_binding_under_an_if_that_never_runs_binds_nothing(update_shape, + tmp_path, + binding): + """`_same_scope` (#675): the body of an `if` whose test is + `TYPE_CHECKING` or a false constant never runs, so a binding there is + not one. Read as one, the written module still defined `GONE`, and + the kept importer's `ImportError` got through.""" + assert_the_written_module_drops_gone(update_shape, tmp_path, binding) + + +@pytest.mark.parametrize("binding", [ + pytest.param(b"try:\n raise RuntimeError\n GONE = 1\n" + b"except RuntimeError:\n pass\n", + id="after a raise in a try body"), + pytest.param(b"try:\n if KEPT:\n raise RuntimeError\n" + b" GONE = 1\nexcept RuntimeError:\n pass\n", + id="after a raise in a block inside a try"), + pytest.param(b"try:\n if False:\n pass\n else:\n" + b" raise RuntimeError\n GONE = 1\n" + b"except RuntimeError:\n pass\n", + id="after a raise in the else of if False"), +]) +def test_a_binding_after_a_raise_in_its_block_binds_nothing(update_shape, + tmp_path, + binding): + """`_bound_at` (#675): a statement after an unconditional `raise` in + its block never runs. In a `try` body whose handler catches what it + raised, the module imports, and it does not bind `GONE`.""" + assert_the_written_module_drops_gone(update_shape, tmp_path, binding) + + def test_a_module_this_run_adds_takes_nothing_away(update_shape, tmp_path): """An `--add` file has no bytes on disk yet, so it defines nothing a kept copy could lose: whatever the kept copy imports from it, it stops @@ -1337,6 +1461,55 @@ def test_both_directions_are_one_refusal_with_every_pair_sorted( assert (VALIDATORS_PASS in refused.detail) is said, refused.detail +#: A copy beside `bootstrap.py` that no shipped root carries: a written +#: importer of `NEW` below, and the kept module of the pastes at the end. +HELPER = "scripts/helper.py" + + +def validator_taking(*names: str, module: str = "repo_shape") -> dict: + """`validate-pins.py`, one of a project's validators, taking `names` + from `module`.""" + return {VALIDATE_PINS_COPY: imports(*names, module=module)} + + +@pytest.mark.parametrize("kept, written, current, said", [ + pytest.param({REPO_SHAPE: b"OLD = 1\n"}, {BOOTSTRAP: imports("NEW")}, + validator_taking("OLD"), True, + id="a validator taking other names from the module"), + pytest.param({REPO_SHAPE: b"OLD = 1\n"}, {BOOTSTRAP: imports("NEW")}, + validator_taking("NEW"), False, + id="a validator taking the name the kept module lacks"), + pytest.param({REPO_SHAPE: b"OLD = 1\n"}, {HELPER: imports("NEW")}, + validator_taking("main", module="helper"), False, + id="a validator importing the written importer"), + pytest.param({REPO_SHAPE: b"OLD = 1\n"}, + {HELPER: imports("NEW"), + **validator_taking("main", module="helper")}, None, False, + id="a validator this run writes, importing the importer"), + pytest.param({BOOTSTRAP: TAKES_GONE}, {REPO_SHAPE: DROPS_GONE}, + {REPO_SHAPE: DEFINES_GONE, **validator_taking("GONE")}, + False, + id="a validator taking the name the written module drops"), + pytest.param({BOOTSTRAP: TAKES_GONE}, {REPO_SHAPE: DROPS_GONE}, + {REPO_SHAPE: DEFINES_GONE, + **validator_taking("main", module="bootstrap")}, False, + id="a validator importing the kept importer"), +]) +def test_the_validators_are_not_said_to_pass_where_one_meets_a_line( + update_shape, tmp_path, kept, written, current, said): + """`_validators_meet` (#675): no importer here is a validator, but a + validator that takes a line's missing name from its module, or + imports a line's importer at all, stops on that line's `ImportError` + when it runs after the writes, as this run leaves it: on disk, or in + the bytes this run writes there. The clause would be false of it. A + validator that takes OTHER names from the module, as every shipped one + takes from `repo_shape`, passes, and the clause is said.""" + refused = refusal(update_shape, tmp_path, kept, written, + state="locally-modified", current=current) + assert refused is not None, "every line here disagrees" + assert (VALIDATORS_PASS in refused.detail) is said, refused.detail + + @pytest.mark.parametrize("kept, written, current, said, unsaid", [ pytest.param({REPO_SHAPE: b"OLD = 1\n"}, {BOOTSTRAP: imports("NEW")}, None, LACKS_EXIT, DROPS_EXIT, @@ -1372,3 +1545,86 @@ def too_big(*_args, **_kwargs): monkeypatch.setattr(type(path), "read_bytes", too_big) assert update_shape._names_defined(path) is None + + +# --- the upstream's bytes a remedy names, pasted (#675) ---------------------- + +#: Where a fork of the standard keeps a copy, with a blank and a `'` in the +#: path: printed bare, `git show` read two arguments and exited 128, or `sh` +#: met a quotation nothing closed. +AWKWARD_SOURCE = "templates/assembly-root/scripts/it's a helper.py" +AWKWARD_BYTES = b"AWKWARD = 1\n" + + +@pytest.fixture +def fork(tmp_path) -> tuple[Path, str]: + """A fork of the standard, built for the test: one commit holding a + copy at `AWKWARD_SOURCE`. Its path, and that commit.""" + standard = tmp_path / "fork" + git("init", "-q", str(standard), cwd=tmp_path) + (standard / AWKWARD_SOURCE).parent.mkdir(parents=True) + (standard / AWKWARD_SOURCE).write_bytes(AWKWARD_BYTES) + git("add", "--", AWKWARD_SOURCE, cwd=standard) + git("commit", "-q", "-m", "A copy at an awkward path", cwd=standard) + return standard, git("rev-parse", "HEAD", cwd=standard).stdout.strip() + + +def awkward_row(update_shape, target: str): + """A `both` row for `HELPER`, whose source in the fork is + `AWKWARD_SOURCE` at `target`.""" + row = update_shape.Row(HELPER, "") + row.state, row.source, row.target = (update_shape.BOTH, AWKWARD_SOURCE, + target) + return row + + +def merge_remedy(update_shape, target: str) -> str: + """`update-conflict`'s remedy for that row (`merge_by_hand`).""" + return update_shape.merge_by_hand([awkward_row(update_shape, target)]) + + +def missing_name_remedy(update_shape, target: str) -> str: + """`update-accept-local-missing-name`'s remedy where that row is the + kept module a written `bootstrap.py` takes `NEW` from.""" + pair = update_shape.Disagreement(HELPER, BOOTSTRAP, ["NEW"], + update_shape.KEPT_MODULE) + return update_shape._missing_name_remediation( + [pair], {HELPER: awkward_row(update_shape, target)}, target) + + +def overwrite_remedy(update_shape, target: str) -> str: + """`update-add-would-overwrite`'s remedy for an `--add` of `HELPER`, + whose source in the fork is `AWKWARD_SOURCE`, where the root already + has a file (`resolve_additions`).""" + added = update_shape.Addition( + update_shape.Copy(HELPER, AWKWARD_SOURCE, False), True, AWKWARD_BYTES) + with pytest.raises(update_shape.Refusal) as refused: + update_shape.resolve_additions([], [added], [HELPER], target) + assert refused.value.code == "update-add-would-overwrite", refused.value + return refused.value.remediation + + +@WINDOWS_SKIP +@pytest.mark.parametrize("remedy", [ + pytest.param(merge_remedy, id="update-conflict"), + pytest.param(missing_name_remedy, id="update-accept-local-missing-name"), + pytest.param(overwrite_remedy, id="update-add-would-overwrite"), +]) +def test_the_git_show_a_remedy_names_pastes_and_runs(update_shape, fork, + remedy): + """Each remedy that names the upstream's bytes prints `git show + :` quoted, as #588 quoted the other printed paths: the + one command a POSIX shell splits it into, and, pasted into `sh` in the + fork, the copy's own bytes. A plain path prints exactly as it did, + which `tests/test_update_shape_messages.py` and the `git show` lines + above hold; `update-add-would-overwrite` printed `` where the + target goes, and `tests/test_update_shape_add.py` holds its line.""" + root, commit = fork + shows = re.findall(r"`(git show [^`]*)`", remedy(update_shape, commit)) + assert len(shows) == 1, shows + assert shlex.split(shows[0]) == ["git", "show", + f"{commit}:{AWKWARD_SOURCE}"], shows + pasted = subprocess.run(["sh", "-c", shows[0]], cwd=str(root), + capture_output=True, check=False) + assert pasted.returncode == 0, pasted.stderr + assert pasted.stdout == AWKWARD_BYTES, pasted.stdout diff --git a/update-shape.py b/update-shape.py index e8e54b7e..b3816c22 100755 --- a/update-shape.py +++ b/update-shape.py @@ -1441,8 +1441,9 @@ def resolve_additions(rows: list[Row], additions: list[Addition], "Two files with one name is a merge, and this command copies " "bytes rather than merging them", "Remediation: the same posture as --accept-local — a human " - "merges. The upstream's bytes are `git show :" - f"{add.source}` in a shape clone. Once a file the project owns " + "merges. The upstream's bytes are " + f"`{upstream_show(target, add.source)}` in a shape clone. " + "Once a file the project owns " f"sits at {path}, `check` stops reporting it: a path the root " "has and the pin does not record is the in-place adoption " "state, which this tool leaves alone by design.") @@ -3720,6 +3721,20 @@ def _refuse_conflicts(rows: list[Row], accepted: set) -> None: conflict_remediation(conflicts)) +def upstream_show(target: str, source: str) -> str: + """`git show :`, the command a remedy names for the + upstream's bytes of a copy, QUOTED for a POSIX shell as #588 (#532) + quoted the other printed paths (#675). A plain path prints the bytes it + always printed, since `shlex.join` leaves a word of `[\\w@%+=:,./-]` + alone; a source path holding a blank or a `'` was two arguments, a git + exit 128, or a quotation `sh` never saw closed, and is one argument now. + `update-conflict`, `update-accept-local-missing-name` and + `update-add-would-overwrite` print it; the last one named a `` + for the person to fill in, and names the run's target now. + """ + return shlex.join(["git", "show", f"{target}:{source}"]) + + def merge_by_hand(rows: list[Row]) -> str: """What `update-conflict` tells the rows it can name a merge for: `both`, and the two states that cannot be compared. The words it always printed, @@ -3735,9 +3750,9 @@ def merge_by_hand(rows: list[Row]) -> str: command each, by the path in the root they are for. A row that is not `both` is named no command, because none reads: `unmapped` has no upstream file, and an `upstream-removed` one is gone at the target, where - `git show` would fail. + `git show` would fail. Each command is `upstream_show`'s, quoted. """ - shows = [(row.path, f"git show {row.target}:{row.source}") + shows = [(row.path, upstream_show(row.target, row.source)) for row in rows if row.state == BOTH and row.source and row.target] if len(shows) == 1: @@ -3885,12 +3900,14 @@ def _refuse_kept_copies_missing_names(root: Path, rows: list[Row], written, so nothing is rolled back, and nobody is asked for a yes to a run that would leave a copy that cannot start. - UNLESS AN IMPORTER IS ONE OF THE VALIDATORS. `kind.validators` are what + UNLESS A VALIDATOR MEETS THE LINE ITSELF. `kind.validators` are what `_refuse_red_validators` runs after the writes, so a validator that imports a name its module lacks, whether this run writes the validator - or keeps it, goes red there, and the run is rolled back. This still - refuses it first, before the yes and before a byte is written, but says - the validators pass only when no importer it names is one of them: a + or keeps it, goes red there, and the run is rolled back; and so does + one that imports such an importer, or takes the same name from the + same module (#675). This still refuses it first, before the yes and + before a byte is written, but says the validators pass only where + `_validators_meet` finds none of them meeting a line it names: a clause that is false of one line in the list is false. WHAT IS ASKED, AND OF WHAT: BOTH DIRECTIONS, between a KEPT copy, a @@ -3935,13 +3952,24 @@ def _refuse_kept_copies_missing_names(root: Path, rows: list[Row], answer after the writes, as they did before #536, and roll the run back where a validator is the copy that cannot import. A name bound by `:=`, a `match` capture, `type X =` or a `global` declared in a function is - read as defined (`_bound_at` says what it reads as a binding). A name - bound only through `globals()`, `setattr` on the module or `exec` reads - as missing: refused where a kept module binds it that way (#536), or - where the written bytes do and the bytes on disk bound it plainly - (#604). One bound and then deleted (`NAME = 1`, then `del NAME`) reads - as defined, so that copy's `ImportError` is still its first run's to - meet. + read as defined (`_bound_at` says what it reads as a binding). One + bound, or imported, only where Python never runs it, under `if + TYPE_CHECKING:`, `if False:` or `if 0:`, or after an unconditional + `raise` in its block, is not read at all (#675). The rest of what + never runs is read as running, and this paragraph's rule holds there + for a BINDING only, which reads as defined, as one bound and then + deleted does. An IMPORT read as running is TAKEN, so a copy whose + import Python never reaches is refused although it starts: one in the + `else` of `if True:` or of `if not TYPE_CHECKING:`, under `while + False:`, in the `else` of a `try` whose body always raises, or after a + `break`, a `continue` or a `return` (`_names_imported`): a refusal + Python would not make, though still one before the yes and before any + write. A name bound only through `globals()`, `setattr` on the module + or `exec` reads as missing: refused where a kept module binds it that + way (#536), or where the written bytes do and the bytes on disk bound + it plainly (#604). One bound and then deleted (`NAME = 1`, then `del + NAME`) reads as defined, so that copy's `ImportError` is still its + first run's to meet. """ kept = {row.path: row for row in rows if row.state in (BOTH, LOCALLY_MODIFIED) and row.path in accepted} @@ -3951,7 +3979,8 @@ def _refuse_kept_copies_missing_names(root: Path, rows: list[Row], pairs = sorted(_kept_copies_lacking(root, written, kept) + _kept_copies_importing_dropped(root, written, kept)) if pairs: - raise _missing_name_refusal(pairs, kept, target, kind) + raise _missing_name_refusal( + pairs, kept, target, _validators_meet(root, written, pairs, kind)) #: Which side of a pair `update-accept-local-missing-name` names is the @@ -4062,15 +4091,50 @@ def _names_dropped(root: Path, path: str, data: bytes | None) -> set[str]: return now - after +def _validators_meet(root: Path, written: dict, pairs: list, + kind: Kind) -> bool: + """Would one of `kind.validators` stop on a line's `ImportError` itself + when `_refuse_red_validators` runs it after the writes (#536, #604, + #675)? Where one would, the refusal must not say the validators pass. + + Asked of each validator as this run leaves it, the bytes it writes + there or else the bytes on disk, by `_names_imported`, the reading + the lines themselves are made by. A validator meets a line where it is + the line's importer; where it imports anything from a line's importer, + which stops on its own import before the validator gets a name; or + where it takes from a line's module a name that line says the module + lacks. A validator that takes OTHER names from a line's module passes, + as every shipped one does: each takes from `repo_shape` only names + `repo_shape.py` defines, so no shipped copy reaches this today. What + `_names_imported` cannot read, a star import or `import M`, is not + found here either, nor a validator that reaches a line's importer + through another copy: one hop is read. + """ + importers = {pair.importer for pair in pairs} + lacking = {(pair.module, name) for pair in pairs for name in pair.names} + for validator in kind.validators: + if validator in importers: + return True + data = written.get(validator) + taken = (_imports_on_disk(root, validator) if data is None + else _names_imported(data)) + for module, names in taken.items(): + beside = _module_beside(validator, module) + if beside in importers or any((beside, name) in lacking + for name in names): + return True + return False + + def _missing_name_refusal(pairs: list, kept: dict, target: str, - kind: Kind) -> Refusal: + validated: bool) -> Refusal: """The one `update-accept-local-missing-name` refusal, one line per pair, in either direction (#536, #604). Every sentence in it is true of every line it lists, whichever side of the line is the kept copy; - the validators clause is said only where no importer named is one of - them, as `_refuse_kept_copies_missing_names` says why. + the validators clause is said only where `validated`, which + `_validators_meet` answers, is false, as + `_refuse_kept_copies_missing_names` says why. """ - validated = any(pair.importer in kind.validators for pair in pairs) return Refusal( "update-accept-local-missing-name", "--accept-local keeps this project's own bytes of a copy that " @@ -4103,12 +4167,13 @@ def _missing_name_remediation(pairs: list, kept: dict, target: str) -> str: again with the same `--accept-local`; or drop the edit, so the upstream's copy is taken. Each change names the lines it settles, so neither is read as settling the other's. `--accept-local` is named in - prose, and no NEXT line passes it. + prose, and no NEXT line passes it. The upstream's bytes are named by + `upstream_show`, quoted, since #604 made a kept copy any `.py` path. """ named = sorted({pair.module if pair.side == KEPT_MODULE else pair.importer for pair in pairs}) - upstreams = "; ".join(f"`git show {target}:{kept[path].source}` for " - f"{path}" for path in named) + upstreams = "; ".join(f"`{upstream_show(target, kept[path].source)}` " + f"for {path}" for path in named) both = [path for path in named if kept[path].state == BOTH] sides = {pair.side for pair in pairs} changes = [] @@ -4190,9 +4255,17 @@ def _names_imported(data: bytes) -> dict[str, set[str]]: A STAR IMPORT NAMES NOTHING, so it adds no name, and an `import ` is not read: which attributes it uses is not something `ast` says. - A NAME THE COPY ITSELF ANSWERS FOR IS NOT TAKEN. Nothing inside an `if - TYPE_CHECKING:` body is, since it never runs. Inside the body of a - `try` that catches the `ImportError` a missing name raises + A NAME THE COPY ITSELF ANSWERS FOR IS NOT TAKEN, NOR ONE PYTHON NEVER + REACHES. Nothing inside an `if` body that never runs is (`_never_runs`: + `if TYPE_CHECKING:`, `if False:`, `if 0:`), nor an import after an + unconditional `raise` in its own block (`_reached`, #675). ANY OTHER + IMPORT PYTHON NEVER REACHES IS READ AS RUNNING, AND SO AS TAKEN: one in + the `else` of `if True:` or of `if not TYPE_CHECKING:`, under `while + False:`, in the `else` of a `try` whose body always raises, or after a + `break`, a `continue` or a `return`. Where its module lacks the name, + that copy is refused although Python starts it: for an import, "what + this cannot tell, it does not refuse" does not hold. Inside the body + of a `try` that catches the `ImportError` a missing name raises (`_catches_import_error`), a name is still taken unless the first handler that catches it binds that same local name itself, its `as` name else the name, as `_bound_at` reads a binding: that is the copy's @@ -4226,7 +4299,8 @@ def _imports_with_fallbacks(module: ast.Module): innermost `try` around it, in its own scope, binds, since that handler is the one that catches it. Empty outside any such `try`. """ - pending = [(statement, frozenset()) for statement in module.body] + pending = [(statement, frozenset()) + for statement in _reached(module.body)] while pending: node, answered = pending.pop() if isinstance(node, ast.ImportFrom): @@ -4238,15 +4312,17 @@ def _blocks_inside(node: ast.AST, answered: frozenset) -> list: """The clauses directly inside `node`, each with the names a fallback answers there: `_fallback_in` for `node`'s own `body`, and what `node` found for the rest, since a `try`'s handlers, `else` and `finally` are - outside its own guard. An `if TYPE_CHECKING:` body is not entered. + outside its own guard. The body of an `if` that `_never_runs` is not + entered, only its `else`, and no block past its first `raise` + (`_reached`, #675). """ - if isinstance(node, ast.If) and _is_type_checking(node.test): - return [(clause, answered) for clause in node.orelse] + if isinstance(node, ast.If) and _never_runs(node.test): + return [(clause, answered) for clause in _reached(node.orelse)] inside = _fallback_in(node, answered) return [(clause, inside if field == "body" else answered) for field, value in ast.iter_fields(node) if isinstance(value, list) - for clause in value if isinstance(clause, _BLOCKS)] + for clause in _reached(value) if isinstance(clause, _BLOCKS)] def _fallback_in(node: ast.AST, answered: frozenset) -> frozenset: @@ -4282,6 +4358,34 @@ def _is_type_checking(test: ast.expr) -> bool: and test.attr == "TYPE_CHECKING")) +def _never_runs(test: ast.expr) -> bool: + """Is `test` an `if` test whose body never runs (#675)? `TYPE_CHECKING` + (`_is_type_checking`), and a false constant: `if False:`, `if 0:`, `if + None:`. Any other test is read as one that may be true. + + `TYPE_CHECKING` IS READ BY ITS NAME, NOT ITS VALUE, as `_blocks_inside` + has read an import under it since #536 and `_same_scope` reads a + binding there now: a module that sets its own `TYPE_CHECKING = True`, + or tests an `obj.TYPE_CHECKING` that is not `typing`'s, is still read + as one whose body never runs. + """ + return _is_type_checking(test) or (isinstance(test, ast.Constant) + and not test.value) + + +def _reached(block: list) -> list: + """`block` up to and including its first unconditional `raise`, after + which no statement in the same block runs (#675): `raise RuntimeError` + then `NEW = 1` in a `try` body binds nothing, whatever the handler + does. Only a list of statements holds a `raise`, so any other list a + node carries comes back whole. + """ + for at, node in enumerate(block): + if isinstance(node, ast.Raise): + return block[:at + 1] + return block + + def _names_defined(path: Path) -> set[str] | None: """Every name the copy at `path` binds at module scope (#536), as `_names_defined_in` reads its bytes, or None when it cannot tell: also @@ -4343,6 +4447,17 @@ def _bound_at(statements: list) -> set[str]: stand, and never inside a function, class or lambda body, which bind names of their own scope. + NOR WHAT PYTHON NEVER RUNS (#675): the body of an `if` whose test is + `TYPE_CHECKING` or a false constant (`_never_runs`: `if False:`, `if + 0:`), whose `else` is read; and a statement after an unconditional + `raise` in its own block (`_reached`), a `try` body's among them. A + binding there is not a binding, so a written module whose only `NEW` + sits there drops it, and a kept one lacks it. The rest of what never + runs is read as running: `while False:`, a test that is false only + once evaluated, the `else` of a `try` whose body always raises, a + statement after a call that exits. What this cannot tell, it does not + refuse. + NOT READ: a name bound through `globals()`, `setattr` on the module or `exec`, which reads as missing; and `del`, so a name bound and then deleted reads as bound. @@ -4354,14 +4469,22 @@ def _bound_at(statements: list) -> set[str]: def _same_scope(statements: list): """Every node under `statements` that runs in their own scope. A `def`, `class` or `lambda` is yielded, since its name is bound there, and not - entered. + entered. Nor is what never runs (#675): of an `if` that `_never_runs`, + only the `else` is entered, and no block past its first `raise` + (`_reached`). """ - pending = list(statements) + pending = list(_reached(statements)) while pending: node = pending.pop() yield node - if not isinstance(node, _SCOPES): - pending.extend(ast.iter_child_nodes(node)) + if isinstance(node, ast.If) and _never_runs(node.test): + pending.extend(_reached(node.orelse)) + elif not isinstance(node, _SCOPES): + pending.extend(child for _, value in ast.iter_fields(node) + for child in (_reached(value) + if isinstance(value, list) + else [value]) + if isinstance(child, ast.AST)) def _names_bound(node: ast.AST) -> set[str]: