Skip to content

Conversation

@randolf-scholz
Copy link
Contributor

Fixes #14965

@randolf-scholz randolf-scholz marked this pull request as draft November 2, 2025 10:17
@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2025

Diff from mypy_primer, showing the effect of this PR on open source code:

static-frame (https://github.com/static-frame/static-frame)
+ static_frame/core/node_values.py:98: error: Unused "type: ignore" comment  [unused-ignore]
+ static_frame/core/node_values.py:272: error: Unused "type: ignore" comment  [unused-ignore]

@randolf-scholz randolf-scholz marked this pull request as ready for review November 2, 2025 10:41
@randolf-scholz
Copy link
Contributor Author

@srittau This should do the trick.

@srittau srittau merged commit 7fde970 into python:main Nov 2, 2025
63 checks passed
Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Does this intentionally drop the __call__: None?

@randolf-scholz
Copy link
Contributor Author

@hauntsaninja I believe this is/was just some relic for annotating that this class couldn't be instantiated before @type_check_only.

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Nov 2, 2025

iirc it was a trick that gave better error messages when users did raise notimplemented() instead of raise notimplementederror()

maybe worth re-running pyright and mypy to see what the errors are like these days

@randolf-scholz
Copy link
Contributor Author

Oh, is that a thing? I've never even considered raising NotImplemented rather than NotImplementedError, it doesn't seem to work at runtime:

>>> raise NotImplemented()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'NotImplementedType' object is not callable
>>> raise NotImplemented
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: exceptions must derive from BaseException

@randolf-scholz
Copy link
Contributor Author

randolf-scholz commented Nov 2, 2025

Ok, so I just tested the following:

def testA() -> None:
    raise NotImplemented()

def testB() -> None:
    raise NotImplemented

with results (this PR)

mypy 3.9 mypy 3.10 pyright 3.9 pyright 3.10
raise NotImplemented ERROR OK OK OK
raise NotImplemented() ERROR OK ERROR OK

vs commit 8aaa86f (before this PR)

mypy 3.9 mypy 3.10 pyright 3.9 pyright 3.10
raise NotImplemented ERROR ERROR OK OK
raise NotImplemented() ERROR ERROR ERROR OK
  • the __call__: None means that raise NotImplemented() will generate an error of the form error: "None" not callable [misc]
    • How useful is this? Since we subclass Any, NotImplemented.method() or NotImplemented[key] will not raise any error either
    • raise NotImplemented already gets flagged by linters (flake8 rule F901, ruff F901)
  • mypy no longer emits error: Exception must be derived from BaseException; did you mean "NotImplementedError" for 3.10 after this PR for 3.10+.
    • not sure why this changed...

@srittau
Copy link
Collaborator

srittau commented Nov 2, 2025

When re-adding this, we should leave a comment explaining why it's there.

@hauntsaninja
Copy link
Collaborator

Okay, looks like mypy in the intervening years added some special casing here. This explains your last bullet point; the special casing was added assuming a fully resolved name of builtins._NotImplementedType.

Given mypy has some special casing now and pyright's behaviour is unaffected, let's just do the simpler thing and remove this in both places. I opened #14971 to make the definitions consistent in typeshed. Also opening a PR to mypy to fix up the special casing

@hauntsaninja
Copy link
Collaborator

mypy PR here python/mypy#20168

hauntsaninja added a commit to python/mypy that referenced this pull request Nov 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Regression: types.NotImplementedType considered @type_check_only

3 participants