GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-104504: cases generator: Add `--warn-unreachable` to the mypy config by AlexWaygood · Pull Request #108112 · python/cpython

Conversation

AlexWaygood commented Aug 18, 2023 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

Backport typing.assert_never to the cases_generator directory, so that mypy doesn't complain about the fact that we're using a feature new in Python 3.11 when we have --python-version 3.10 in our config file.

Issue: Run mypy on cases_generator #104504

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

AlexWaygood added 3 commits

August 17, 2023 13:58


          Improve the GitHub issue forms

833586f


          Merge branch 'main' of https://github.com/python/cpython

c0a42bc


          cases generator: Add --warn-unreachable to the mypy config

ba097e0

AlexWaygood requested review from Fidget-Spinner, JelleZijlstra and gvanrossum as code owners

August 18, 2023 15:10

bedevere-bot added the awaiting core review label

Aug 18, 2023

AlexWaygood added skip news and removed awaiting core review labels

Aug 18, 2023

AlexWaygood changed the title ~~cases generator: Add --warn-unreachable to the mypy config~~ gh-104504: cases generator: Add --warn-unreachable to the mypy config

Aug 18, 2023

bedevere-bot mentioned this pull request

Aug 18, 2023

Run mypy on cases_generator #104504

Closed

AlexWaygood commented

Aug 18, 2023

View reviewed changes

Comment thread

Tools/cases_generator/generate_cases.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Tools/cases_generator/generate_cases.py

		case parsing.Macro():
		format = self.macro_instrs[thing.name].instr_fmt
		case parsing.Pseudo():
		format = None

AlexWaygood Aug 18, 2023

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, here mypy was eagerly narrowing the type of format to None, meaning that it was inferring the else branch on line 396 to be unreachable.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

gvanrossum Aug 20, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still feels like a mypy bug to me, but your fix is fine.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

AlexWaygood reacted with thumbs up emoji

All reactions

👍 1 reaction


          Actually assign it

e4a9e06

AlexWaygood commented Aug 18, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member Author

Cc. @corona10, @sobolevn

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gvanrossum approved these changes

Aug 18, 2023

View reviewed changes

Comment thread

Tools/cases_generator/generate_cases.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot added the awaiting merge label

Aug 18, 2023

AlexWaygood marked this pull request as draft

August 19, 2023 09:22

bedevere-bot removed the awaiting merge label

Aug 19, 2023

AlexWaygood added 5 commits

August 20, 2023 16:05


          Merge branch 'main' of https://github.com/python/cpython

82bc6f6


          Merge branch 'main' into cases-generator-reachability

a27ffa0


          Fix one of the two locations

2f5206d


          Fix the other one

ec78f7c


          Revert unrelated changes

8497a68

AlexWaygood marked this pull request as ready for review

August 20, 2023 15:18

bedevere-bot added the awaiting core review label

Aug 20, 2023

AlexWaygood commented Aug 20, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member Author

@gvanrossum, are you happier with the new refactor? Now pushed and popped are always str, never None, in get_stack_effect_info(). Similarly, format is always str, never None, in write_metadata(). It resolves the mypy reachability-analysis issue just as well as my previous idea, and keeps the symmetry between the branches that we have on main.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

AlexWaygood requested a review from gvanrossum

August 20, 2023 15:23

AlexWaygood commented Aug 20, 2023

Copy link

Copy Markdown

Member Author

(The test_concurrent_futures failure on Azure Pipelines is unrelated)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gvanrossum approved these changes

Aug 20, 2023

View reviewed changes

gvanrossum left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's check this in.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Tools/cases_generator/generate_cases.py

		# 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):

gvanrossum Aug 20, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clever idea. I wrote a whole paragraph arguing against using enumerate() here, then realized it wasn't worth arguing over. :-)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

AlexWaygood reacted with laugh emoji

All reactions

😄 1 reaction

gvanrossum Aug 24, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, the saga continues. Currently we get errors from pyright on this: "popped is possibly unbound" (on all occurrences of popped and pushed in this function below here).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

AlexWaygood Aug 24, 2023

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, the saga continues. Currently we get errors from pyright on this: "popped is possibly unbound" (on all occurrences of popped and pushed in this function below here).

Yeah, and mypy will also give you that error if you add --enable-error-code=possibly-undefined to the mypy config :/

There is a third way of fixing this. I'll make a PR and see how you like it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Tools/cases_generator/generate_cases.py

		case parsing.Macro():
		format = self.macro_instrs[thing.name].instr_fmt
		case parsing.Pseudo():
		format = None

gvanrossum Aug 20, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still feels like a mypy bug to me, but your fix is fine.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

AlexWaygood reacted with thumbs up emoji

All reactions

👍 1 reaction

bedevere-bot removed the awaiting core review label

Aug 20, 2023

bedevere-bot added the awaiting merge label

Aug 20, 2023

AlexWaygood merged commit 05ef4ca into python:main

Aug 20, 2023

bedevere-bot removed the awaiting merge label

Aug 20, 2023

AlexWaygood deleted the cases-generator-reachability branch

August 20, 2023 23:40

AlexWaygood commented Aug 20, 2023

Copy link

Copy Markdown

Member Author

Thanks for the review!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

AlexWaygood mentioned this pull request

Aug 22, 2023

Fix spurious diff if the cases generator is run on Windows #108319

Merged

gvanrossum reviewed

Aug 24, 2023

View reviewed changes

Comment thread

Tools/cases_generator/generate_cases.py

Comment on lines +389 to 395

		if idx == 0:
		format = target_instr.instr_fmt
		else:
		assert format == target_instr.instr_fmt
		assert format is not None
		case _:
		typing.assert_never(thing)
		assert_never(thing)
		all_formats.add(format)

gvanrossum Aug 24, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And ditto here for format.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

AlexWaygood mentioned this pull request

Aug 24, 2023

gh-104504: Cases generator: enable mypy's possibly-undefined error code #108454

Merged

Reviewers

gvanrossum

gvanrossum approved these changes

corona10

corona10 left review comments

Fidget-Spinner

Awaiting requested review from Fidget-Spinner

JelleZijlstra

Awaiting requested review from JelleZijlstra

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

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.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

AlexWaygood commented Aug 18, 2023 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexWaygood commented Aug 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

AlexWaygood commented Aug 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexWaygood commented Aug 20, 2023

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexWaygood commented Aug 20, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AlexWaygood commented Aug 18, 2023 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

AlexWaygood commented Aug 18, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

AlexWaygood commented Aug 20, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.