◐ Shell
reader mode source ↗
Skip to content

_overload_dummy raises NotImplementedError, not RuntimeError#98351

Merged
JelleZijlstra merged 1 commit into
python:mainfrom
sobolevn:use-more-precise-exception-in-test-typing
Oct 20, 2022
Merged

_overload_dummy raises NotImplementedError, not RuntimeError#98351
JelleZijlstra merged 1 commit into
python:mainfrom
sobolevn:use-more-precise-exception-in-test-typing

Conversation

@sobolevn

@sobolevn sobolevn commented Oct 17, 2022

Copy link
Copy Markdown
Member

Source:

cpython/Lib/typing.py

Lines 2536 to 2542 in eae7dad

def _overload_dummy(*args, **kwds):
"""Helper for @overload to raise when called."""
raise NotImplementedError(
"You should not call an overloaded function. "
"A series of @overload-decorated functions "
"outside a stub module should always be followed "
"by an implementation that is not @overload-ed.")

I've found this while looking for implicit abstract classes / methods in CPython.

Test was checking for RuntimeError, it is technically correct:

>>> NotImplementedError.__mro__
(<class 'NotImplementedError'>, <class 'RuntimeError'>, <class 'Exception'>, <class 'BaseException'>, <class 'object'>)

But, we can be more precise. It is a good thing.
I am going to skip creating an issue for this, it is like a typo-fix :)

@bedevere-bot bedevere-bot added awaiting review tests Tests in the Lib/test dir labels Oct 17, 2022
@sobolevn sobolevn added skip issue skip news tests Tests in the Lib/test dir awaiting review and removed tests Tests in the Lib/test dir awaiting review labels Oct 17, 2022
@sobolevn sobolevn changed the title _overload_dummy raise NotImplementedError, not RuntimeError Oct 17, 2022
JelleZijlstra
JelleZijlstra approved these changes Oct 17, 2022
View reviewed changes

@JelleZijlstra JelleZijlstra left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

Thanks for catching this!

I feel like RuntimeError is a better exception to throw than NotImplementedError here, but we've apparently always used NotImplementedError and it's not worth changing.

@JelleZijlstra JelleZijlstra self-assigned this Oct 17, 2022
@JelleZijlstra JelleZijlstra added needs backport to 3.11 only security fixes needs backport to 3.10 only security fixes labels Oct 17, 2022
@JelleZijlstra JelleZijlstra merged commit 1ca6647 into python:main Oct 20, 2022
@miss-islington

miss-islington commented Oct 20, 2022

Copy link
Copy Markdown
Contributor

Thanks @sobolevn for the PR, and @JelleZijlstra for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@miss-islington

miss-islington commented Oct 20, 2022

Copy link
Copy Markdown
Contributor

Sorry, @sobolevn and @JelleZijlstra, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 1ca6647f22794f0a0c982ecb03e764db76d51087 3.10

@bedevere-bot

bedevere-bot commented Oct 20, 2022

Copy link
Copy Markdown

GH-98470 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Oct 20, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 20, 2022
@sobolevn @miss-islington
typing tests: _overload_dummy raises NotImplementedError, not `Ru… …
9d80421 
…ntimeError` (pythonGH-98351)

(cherry picked from commit 1ca6647)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
@JelleZijlstra JelleZijlstra removed the needs backport to 3.10 only security fixes label Oct 20, 2022
@JelleZijlstra

JelleZijlstra commented Oct 20, 2022

Copy link
Copy Markdown
Member

These tests don't exist in 3.10.

miss-islington added a commit that referenced this pull request Oct 20, 2022
@miss-islington @sobolevn
typing tests: _overload_dummy raises NotImplementedError, not `Ru… …
5f5fa4c 
…ntimeError` (GH-98351)

(cherry picked from commit 1ca6647)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
carljm added a commit to carljm/cpython that referenced this pull request Oct 20, 2022
@carljm
Merge branch 'main' into typewatch …
8bba5da 
* main: (40 commits)
  pythongh-98461: Fix source location in comprehensions bytecode (pythonGH-98464)
  pythongh-98421: Clean Up PyObject_Print (pythonGH-98422)
  pythongh-98360: multiprocessing now spawns children on Windows with correct argv[0] in virtual environments (pythonGH-98462)
  CODEOWNERS: Become a typing code owner (python#98480)
  [doc] Improve logging cookbook example. (pythonGH-98481)
  Add more tkinter.Canvas tests (pythonGH-98475)
  pythongh-95023: Added os.setns and os.unshare functions (python#95046)
  pythonGH-98363: Presize the list for batched() (pythonGH-98419)
  pythongh-98374: Suppress ImportError for invalid query for help() command. (pythongh-98450)
  typing tests: `_overload_dummy` raises `NotImplementedError`, not `RuntimeError` (python#98351)
  pythongh-98354: Add unicode check for 'name' attribute in _imp_create_builtin (pythonGH-98412)
  pythongh-98257: Make _PyEval_SetTrace() reentrant (python#98258)
  pythongh-98414: py.exe launcher does not use defaults for -V:company/ option (pythonGH-98460)
  pythongh-98417: Store int_max_str_digits on the Interpreter State (pythonGH-98418)
  Doc: Remove title text from internal links (python#98409)
  [doc] Refresh the venv introduction documentation, and correct the statement about VIRTUAL_ENV (pythonGH-98350)
  Docs: Bump sphinx-lint and fix unbalanced inline literal markup (python#98441)
  pythongh-92886: Replace assertion statements in `handlers.BaseHandler` to support running with optimizations (`-O`) (pythonGH-93231)
  pythongh-92886: Fix tests that fail when running with optimizations (`-O`) in `_test_multiprocessing.py` (pythonGH-93233)
  pythongh-92886: Fix tests that fail when running with optimizations (`-O`) in `test_py_compile.py` (pythonGH-93235)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes

@gvanrossum gvanrossum gvanrossum approved these changes

@Fidget-Spinner Fidget-Spinner Fidget-Spinner approved these changes

Assignees

@JelleZijlstra JelleZijlstra

Labels

skip issue skip news tests Tests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@sobolevn @miss-islington @bedevere-bot @JelleZijlstra @gvanrossum @Fidget-Spinner