8000 Incorrect typing for `error_before_exec` · Issue #14907 · ipython/ipython · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Incorrect typing for error_before_exec #14907

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
krassowski opened this issue May 29, 2025 · 12 comments · Fixed by #14908
Closed

Incorrect typing for error_before_exec #14907

krassowski opened this issue May 29, 2025 · 12 comments · Fixed by #14908
Labels
bug good first issue

Comments

@krassowski
Copy link
Member

error_before_exec is typed as bool here:

ipython/IPython/core/interactiveshell.py

Lines 286 to 294 in b854379

class ExecutionResult:
"""The result of a call to :meth:`InteractiveShell.run_cell`
Stores information about what took place.
"""
execution_count: Optional[int] = None
error_before_exec: Optional[bool] = None
error_in_exec: Optional[BaseException] = None

but later it is raised:

ipython/IPython/core/interactiveshell.py

Lines 305 to 310 in b854379

def raise_error(self):
"""Reraises error if `success` is `False`, otherwise does nothing"""
if self.error_before_exec is not None:
raise self.error_before_exec
if self.error_in_exec is not None:
raise self.error_in_exec

The tests suggest that both are BaseException descendants:

ipython/tests/test_interactiveshell.py

Lines 112 to 118 in b854379

def test_syntax_error(self):
res = ip.run_cell("raise = 3")
self.assertIsInstance(res.error_before_exec, SyntaxError)
def test_open_standard_input_stream(self):
res = ip.run_cell("open(0)")
self.assertIsInstance(res.error_in_exec, ValueError)

A PR fixing the issue should start investigating why mypy was not complaining and make CI fail.
@samuraikillers
Copy link
Contributor

Hi @krassowski I would like to work on this issue

@krassowski
Copy link
Member Author

Please do!

@samuraikillers
Copy link
Contributor

Hi @krassowski , The errors seems to be because of the [[tool.mypy.overrides]] section for Interactiveshell in the pyproject.toml file;
We have set ignore_errors = true , by default it is false. btw there are 2 places where this ignore_errors is set to true.
For ref:
https://mypy.readthedocs.io/en/stable/config_file.html#suppressing-errors

See Line 176

ipython/pyproject.toml

Lines 171 to 181 in b854379

[[tool.mypy.overrides]]
module = [
"IPython.core.interactiveshell",
]
disallow_untyped_defs = false
ignore_errors = true
ignore_missing_imports = true
disallow_untyped_calls = false
disallow_incomplete_defs = true
check_untyped_defs = true
disallow_untyped_decorators = false

@samuraikillers
Copy link
Contributor

Sharing the difference:

With default ignore_errors = true, we see this output when using mypy:

Image

Changing ignore_errors = false, we started seeing numerous errors:

Image

There are around 54 errors, but the one which you are interested was line 308 which we can see in the above snippet 2nd last line.

@krassowski
Copy link
Member Author

Great find! Can you try to find a way to flip it to ignore_errors = false while minimising changes to the codebase (e.g. adding ignores for other errors, switching other options). Basically we want to progressively get to the state where there are no overrides but let's not do it all at once to minimise the cost of review and the risk of breaking things.

@samuraikillers
Copy link
Contributor

Sure, let me explore and will raise a PR :)

@samuraikillers
Copy link
Contributor

Just one doubt I had, do you want me to tweak the pyproject.toml configuration only so that we see less errors, or you want me to try modifying some of the existing function in Ineteractiveshell.py to resolve it?

@samuraikillers
Copy link
Contributor
samuraikillers commented May 29, 2025 •
edited
Loading

Hi @krassowski ,
To keep progressing slowly and minimizing the review effort I think I can change 3 parameters in the toml file so that we see only 3 errors as of now:

Image

I think its a good starting point , gradually we can start flipping the necessary parameters along with fixing the issues.

So, if you are satisfied with this much change only. Then I will create the PR.
Thanks

@krassowski
Copy link
Member Author

If a rule can be enabled (or not get disabled) by making small tweaks to typing in interatctiveshell.py then sure, but let's try to find a right balance. I would be worried about any change that changes the code logic (not typing) itself.

@krassowski
Copy link
Member Author

Sure, but let's also try to find what would have caught the error_before_exec issue. (I think it is not on the list).

@samuraikillers
Copy link
Contributor

All the existing parameter in the toml for interactiveshell are fine, and if we just flip the ignore_errors we can see the BaseException for error_before_exec.

But the main paramater which helps in finding this error is check_untyped_defs = true if this is enabled along with ignore_errors=false we can catch this error.

@samuraikillers samuraikillers mentioned this issue May 29, 2025
Merged
@samuraikillers
Copy link
Contributor

Hi @krassowski ,
PR is Raised #14908 ,
have resolved 2 of the errors the one which is left out might require some exploration from my side.

@krassowski krassowski linked a pull request May 30, 2025 that will close this issue
Fix typing of error_before_exec, enhance mypy coverage #14908
Merged
krassowski added a commit that referenced this issue May 30, 2025
@krassowski
Fix typing of error_before_exec, enhance mypy coverage (#14908)
e95f1a6 
This PR fixes issue #14907 


Changes made:

1. `pyproject.toml` => changed ignore_error = false, so that we can
catch BaseException like errors.

Drawback => Testing with mypy we see 54 errors for
`ipython/IPython/core/interactiveshell.py`
Resolution => To flip certain parameter in pyproject.toml so that we get
less errors and try to resolve each of them in segments to minimize
review efforts.


2. `ipython/IPython/core/interactiveshell.py` => Upon making the best
changes to `pyproject.toml` we reduce the errors to 3(we will cover the
rest of them gradually, this is for starters).
Which were:

![image](https://github.com/user-attachments/assets/41d0f0eb-32cc-443c-8e5d-2b64a86ca49b)

I had resolved 2 of them :

https://github.com/ipython/ipython/blob/b854379e3b801ce812c648e95f8846a1334e3195/IPython/core/interactiveshell.py#L334
and

https://github.com/ipython/ipython/blob/b854379e3b801ce812c648e95f8846a1334e3195/IPython/core/interactiveshell.py#L3602

Which eventually results in only 1 error, with current settings:

![image](https://github.com/user-attachments/assets/d48741d4-1ee3-4fb7-9eae-709be972cc63)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug good first issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix typing of error_before_exec, enhance mypy coverage samuraikillers/ipython
2 participants
@krassowski @samuraikillers
0