From 833586f853de4bbe823a7d97b2d25df85ed02d6d Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Thu, 17 Aug 2023 13:58:29 +0100 Subject: [PATCH 1/6] Improve the GitHub issue forms --- .github/ISSUE_TEMPLATE/bug.yml | 6 +++--- .github/ISSUE_TEMPLATE/feature.yml | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/ISSUE_TEMPLATE/bug.yml b/.github/ISSUE_TEMPLATE/bug.yml index 05f4f317ccc97e..bfd06048c83e77 100644 --- a/.github/ISSUE_TEMPLATE/bug.yml +++ b/.github/ISSUE_TEMPLATE/bug.yml @@ -19,7 +19,7 @@ body: options: - label: I am confident this is a bug in CPython, not a bug in a third-party project required: false - - label: | + - label: > I have searched the [CPython issue tracker](https://github.com/python/cpython/issues?q=is%3Aissue+sort%3Acreated-desc), and am confident this bug has not been reported before required: false @@ -55,9 +55,9 @@ body: required: false - type: textarea attributes: - label: "A clear and concise description of the bug:" + label: "Bug description:" description: > - Tell us what happened. + Give a clear and concise description of what happened. Include a [minimal, reproducible example](https://stackoverflow.com/help/minimal-reproducible-example) if possible. Put any code blocks inside triple backticks. diff --git a/.github/ISSUE_TEMPLATE/feature.yml b/.github/ISSUE_TEMPLATE/feature.yml index 0200e623d2a3b0..05f9e0e40d7c0d 100644 --- a/.github/ISSUE_TEMPLATE/feature.yml +++ b/.github/ISSUE_TEMPLATE/feature.yml @@ -9,7 +9,7 @@ body: You'll need to demonstrate widespread support for your idea among the community. - Major feature proposals should generally be discussed on [Discourse](https://discuss.python.org/c/ideas/6) before opening a GitHub issue. Wait until it's clear that most people support your idea before filling in this form. + Major feature proposals should generally be discussed on [Discourse](https://discuss.python.org/c/ideas/6) before opening a GitHub issue. Wait until it's clear that most people support your idea before filling in this form. Anything that proposes new syntax or a change to a builtin class or function counts as a major feature proposal. - type: dropdown attributes: label: Has this already been discussed elsewhere? From ba097e014a6bc190f98ac145f7ce066943315020 Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Fri, 18 Aug 2023 16:08:24 +0100 Subject: [PATCH 2/6] cases generator: Add `--warn-unreachable` to the mypy config --- Tools/cases_generator/_typing_backports.py | 15 +++++++++++++++ Tools/cases_generator/analysis.py | 7 ++++--- Tools/cases_generator/generate_cases.py | 21 ++++++++++----------- Tools/cases_generator/mypy.ini | 9 ++++----- 4 files changed, 33 insertions(+), 19 deletions(-) create mode 100644 Tools/cases_generator/_typing_backports.py diff --git a/Tools/cases_generator/_typing_backports.py b/Tools/cases_generator/_typing_backports.py new file mode 100644 index 00000000000000..c2aa50804cefe2 --- /dev/null +++ b/Tools/cases_generator/_typing_backports.py @@ -0,0 +1,15 @@ +"""Backports from newer versions of the typing module. + +We backport these features here so that Python can still build +while using an older Python version for PYTHON_FOR_REGEN. +""" + +from typing import NoReturn + + +def assert_never(obj: NoReturn) -> NoReturn: + """Statically assert that a line of code is unreachable. + + Backport of typing.assert_never (introduced in Python 3.11). + """ + raise AssertionError(f"Expected code to be unreachable, but got: {obj}") diff --git a/Tools/cases_generator/analysis.py b/Tools/cases_generator/analysis.py index 48f2db981c95b6..72fb2d761dbfae 100644 --- a/Tools/cases_generator/analysis.py +++ b/Tools/cases_generator/analysis.py @@ -2,6 +2,7 @@ import sys import typing +from _typing_backports import assert_never from flags import InstructionFlags, variable_used from formatting import prettify_filename, UNUSED from instructions import ( @@ -172,7 +173,7 @@ def parse_file(self, filename: str, instrs_idx: dict[str, int]) -> None: self.pseudos[name] = thing self.everything.append(thing) case _: - typing.assert_never(thing) + assert_never(thing) if not psr.eof(): raise psr.make_syntax_error(f"Extra stuff at the end of {filename}") @@ -368,7 +369,7 @@ def analyze_macro(self, macro: parsing.Macro) -> MacroInstruction: # SAVE_IP in a macro is a no-op in Tier 1 flags.add(instr.instr_flags) case _: - typing.assert_never(component) + assert_never(component) format = "IB" if flags.HAS_ARG_FLAG else "IX" if offset: format += "C" + "0" * (offset - 1) @@ -409,5 +410,5 @@ def check_macro_components( case parsing.CacheEffect(): components.append(uop) case _: - typing.assert_never(uop) + assert_never(uop) return components diff --git a/Tools/cases_generator/generate_cases.py b/Tools/cases_generator/generate_cases.py index de31129ac0548d..f1269ee04a94c9 100644 --- a/Tools/cases_generator/generate_cases.py +++ b/Tools/cases_generator/generate_cases.py @@ -13,6 +13,7 @@ from collections.abc import Iterator import stacking # Early import to avoid circular import +from _typing_backports import assert_never from analysis import Analyzer from formatting import Formatter, list_effect_size from flags import InstructionFlags, variable_used @@ -154,8 +155,8 @@ def effect_str(effects: list[StackEffect]) -> str: return str(n_effect) instr: AnyInstruction | None - popped: str | None - pushed: str | None + popped: str | None = None + pushed: str | None = None match thing: case parsing.InstDef(): if thing.kind != "op" or self.instrs[thing.name].is_viable_uop(): @@ -171,7 +172,6 @@ def effect_str(effects: list[StackEffect]) -> str: popped, pushed = stacking.get_stack_effect_info_for_macro(instr) case parsing.Pseudo(): instr = self.pseudo_instrs[thing.name] - popped = pushed = None # Calculate stack effect, and check that it's the the same # for all targets. for target in self.pseudos[thing.name].targets: @@ -189,7 +189,7 @@ def effect_str(effects: list[StackEffect]) -> str: assert popped == target_popped assert pushed == target_pushed case _: - typing.assert_never(thing) + assert_never(thing) return instr, popped, pushed @contextlib.contextmanager @@ -388,7 +388,6 @@ def write_metadata(self, metadata_filename: str, pymetadata_filename: str) -> No case parsing.Macro(): format = self.macro_instrs[thing.name].instr_fmt case parsing.Pseudo(): - format = None for target in self.pseudos[thing.name].targets: target_instr = self.instrs.get(target) assert target_instr @@ -398,7 +397,7 @@ def write_metadata(self, metadata_filename: str, pymetadata_filename: str) -> No assert format == target_instr.instr_fmt assert format is not None case _: - typing.assert_never(thing) + assert_never(thing) all_formats.add(format) # Turn it into a sorted list of enum values. @@ -488,7 +487,7 @@ def write_metadata(self, metadata_filename: str, pymetadata_filename: str) -> No self.pseudo_instrs[thing.name] ) case _: - typing.assert_never(thing) + assert_never(thing) with self.metadata_item( "const struct opcode_macro_expansion " @@ -525,7 +524,7 @@ def write_metadata(self, metadata_filename: str, pymetadata_filename: str) -> No case parsing.Pseudo(): pass case _: - typing.assert_never(thing) + assert_never(thing) with self.metadata_item( "const char * const _PyOpcode_uop_name[OPCODE_UOP_NAME_SIZE]", "=", ";" @@ -774,7 +773,7 @@ def write_instructions( case parsing.Pseudo(): pass case _: - typing.assert_never(thing) + assert_never(thing) print( f"Wrote {n_instrs} instructions and {n_macros} macros " @@ -818,7 +817,7 @@ def write_executor_instructions( case parsing.Pseudo(): pass case _: - typing.assert_never(thing) + assert_never(thing) print( f"Wrote {n_instrs} instructions and {n_uops} ops to {executor_filename}", file=sys.stderr, @@ -850,7 +849,7 @@ def write_abstract_interpreter_instructions( case parsing.Pseudo(): pass case _: - typing.assert_never(thing) + assert_never(thing) print( f"Wrote some stuff to {abstract_interpreter_filename}", file=sys.stderr, diff --git a/Tools/cases_generator/mypy.ini b/Tools/cases_generator/mypy.ini index 7480841bf07edc..54ccb8c5c1a3a2 100644 --- a/Tools/cases_generator/mypy.ini +++ b/Tools/cases_generator/mypy.ini @@ -2,13 +2,12 @@ files = Tools/cases_generator/ pretty = True +# Make sure Python can still be built +# using Python 3.10 for `PYTHON_FOR_REGEN`... python_version = 3.10 -# Be strict: +# ...And be strict: strict = True strict_concatenate = True enable_error_code = ignore-without-code,redundant-expr,truthy-bool - -# Don't enable this one yet; -# it has a lot of false positives on `cases_generator` -warn_unreachable = False +warn_unreachable = True From e4a9e06b4506eb4d976c5284f27e48d5bdb28c4c Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Fri, 18 Aug 2023 16:20:22 +0100 Subject: [PATCH 3/6] Actually assign it --- Tools/cases_generator/generate_cases.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tools/cases_generator/generate_cases.py b/Tools/cases_generator/generate_cases.py index f1269ee04a94c9..af4d752dbaf053 100644 --- a/Tools/cases_generator/generate_cases.py +++ b/Tools/cases_generator/generate_cases.py @@ -379,7 +379,7 @@ def write_metadata(self, metadata_filename: str, pymetadata_filename: str) -> No # Compute the set of all instruction formats. all_formats: set[str] = set() for thing in self.everything: - format: str | None + format: str | None = None match thing: case OverriddenInstructionPlaceHolder(): continue From 2f5206dde37af8d48d5452aafd3b7ef1df63a199 Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Sun, 20 Aug 2023 16:11:25 +0100 Subject: [PATCH 4/6] Fix one of the two locations --- Tools/cases_generator/generate_cases.py | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/Tools/cases_generator/generate_cases.py b/Tools/cases_generator/generate_cases.py index af4d752dbaf053..b1db558b9231dc 100644 --- a/Tools/cases_generator/generate_cases.py +++ b/Tools/cases_generator/generate_cases.py @@ -147,7 +147,7 @@ class Generator(Analyzer): def get_stack_effect_info( self, thing: parsing.InstDef | parsing.Macro | parsing.Pseudo - ) -> tuple[AnyInstruction | None, str | None, str | None]: + ) -> tuple[AnyInstruction | None, str, str]: def effect_str(effects: list[StackEffect]) -> str: n_effect, sym_effect = list_effect_size(effects) if sym_effect: @@ -155,8 +155,6 @@ def effect_str(effects: list[StackEffect]) -> str: return str(n_effect) instr: AnyInstruction | None - popped: str | None = None - pushed: str | None = None match thing: case parsing.InstDef(): if thing.kind != "op" or self.instrs[thing.name].is_viable_uop(): @@ -174,7 +172,7 @@ def effect_str(effects: list[StackEffect]) -> str: instr = self.pseudo_instrs[thing.name] # Calculate stack effect, and check that it's the the same # for all targets. - for target in self.pseudos[thing.name].targets: + for idx, target in enumerate(self.pseudos[thing.name].targets): target_instr = self.instrs.get(target) # Currently target is always an instr. This could change # in the future, e.g., if we have a pseudo targetting a @@ -182,8 +180,7 @@ def effect_str(effects: list[StackEffect]) -> str: assert target_instr target_popped = effect_str(target_instr.input_effects) target_pushed = effect_str(target_instr.output_effects) - if pushed is None: - assert popped is None + if idx == 0: popped, pushed = target_popped, target_pushed else: assert popped == target_popped @@ -209,7 +206,6 @@ def write_stack_effect_functions(self) -> None: continue instr, popped, pushed = self.get_stack_effect_info(thing) if instr is not None: - assert popped is not None and pushed is not None popped_data.append((instr, popped)) pushed_data.append((instr, pushed)) From ec78f7c69e60aa50bb038b76143c261d37b9d5b8 Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Sun, 20 Aug 2023 16:13:04 +0100 Subject: [PATCH 5/6] Fix the other one --- Tools/cases_generator/generate_cases.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/Tools/cases_generator/generate_cases.py b/Tools/cases_generator/generate_cases.py index b1db558b9231dc..7b1880b98a8feb 100644 --- a/Tools/cases_generator/generate_cases.py +++ b/Tools/cases_generator/generate_cases.py @@ -375,7 +375,6 @@ def write_metadata(self, metadata_filename: str, pymetadata_filename: str) -> No # Compute the set of all instruction formats. all_formats: set[str] = set() for thing in self.everything: - format: str | None = None match thing: case OverriddenInstructionPlaceHolder(): continue @@ -384,14 +383,13 @@ def write_metadata(self, metadata_filename: str, pymetadata_filename: str) -> No case parsing.Macro(): format = self.macro_instrs[thing.name].instr_fmt case parsing.Pseudo(): - for target in self.pseudos[thing.name].targets: + for idx, target in enumerate(self.pseudos[thing.name].targets): target_instr = self.instrs.get(target) assert target_instr - if format is None: + if idx == 0: format = target_instr.instr_fmt else: assert format == target_instr.instr_fmt - assert format is not None case _: assert_never(thing) all_formats.add(format) From 8497a68f2611d4258c0a68dba92cacf4006c125a Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Sun, 20 Aug 2023 16:16:39 +0100 Subject: [PATCH 6/6] Revert unrelated changes --- .github/ISSUE_TEMPLATE/bug.yml | 6 +++--- .github/ISSUE_TEMPLATE/feature.yml | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/ISSUE_TEMPLATE/bug.yml b/.github/ISSUE_TEMPLATE/bug.yml index bfd06048c83e77..05f4f317ccc97e 100644 --- a/.github/ISSUE_TEMPLATE/bug.yml +++ b/.github/ISSUE_TEMPLATE/bug.yml @@ -19,7 +19,7 @@ body: options: - label: I am confident this is a bug in CPython, not a bug in a third-party project required: false - - label: > + - label: | I have searched the [CPython issue tracker](https://github.com/python/cpython/issues?q=is%3Aissue+sort%3Acreated-desc), and am confident this bug has not been reported before required: false @@ -55,9 +55,9 @@ body: required: false - type: textarea attributes: - label: "Bug description:" + label: "A clear and concise description of the bug:" description: > - Give a clear and concise description of what happened. + Tell us what happened. Include a [minimal, reproducible example](https://stackoverflow.com/help/minimal-reproducible-example) if possible. Put any code blocks inside triple backticks. diff --git a/.github/ISSUE_TEMPLATE/feature.yml b/.github/ISSUE_TEMPLATE/feature.yml index 05f9e0e40d7c0d..0200e623d2a3b0 100644 --- a/.github/ISSUE_TEMPLATE/feature.yml +++ b/.github/ISSUE_TEMPLATE/feature.yml @@ -9,7 +9,7 @@ body: You'll need to demonstrate widespread support for your idea among the community. - Major feature proposals should generally be discussed on [Discourse](https://discuss.python.org/c/ideas/6) before opening a GitHub issue. Wait until it's clear that most people support your idea before filling in this form. Anything that proposes new syntax or a change to a builtin class or function counts as a major feature proposal. + Major feature proposals should generally be discussed on [Discourse](https://discuss.python.org/c/ideas/6) before opening a GitHub issue. Wait until it's clear that most people support your idea before filling in this form. - type: dropdown attributes: label: Has this already been discussed elsewhere?