Skip to content

Remove importlib frames from traceback #1104

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.

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

Merged
merged 3 commits into from
Jul 6, 2019

Conversation

palaviv
Copy link
Contributor

@palaviv palaviv commented Jul 4, 2019

Previously the traceback was:

Traceback (most recent call last):
  File "/tmp/a.py", line 3, in <module>
    import b
  File "_frozen_importlib", line 1093, in __import__
  File "_frozen_importlib", line 1014, in _gcd_import
  File "_frozen_importlib", line 991, in _find_and_load
  File "_frozen_importlib", line 975, in _find_and_load_unlocked
  File "_frozen_importlib", line 686, in _load_unlocked
  File "_frozen_importlib", line 677, in _load_unlocked
  File "_frozen_importlib", line 671, in _load_unlocked
  File "_frozen_importlib_external", line 778, in exec_module
  File "_frozen_importlib", line 219, in _call_with_frames_removed
  File "/tmp/b.py", line 1, in <module>
    1/0
ZeroDivisionError: 'integer division by zero'

Now it is:

Traceback (most recent call last):
  File "/tmp/a.py", line 3, in <module>
    import b
  File "/tmp/b.py", line 1, in <module>
    1/0
ZeroDivisionError: 'integer division by zero'

@palaviv
Copy link
Contributor Author

palaviv commented Jul 4, 2019

This is a adaptation of the remove_importlib_frames from Cpython.

@@ -144,3 +144,35 @@ fn find_source(vm: &VirtualMachine, current_path: PathBuf, name: &str) -> Result
None => Err(format!("No module named '{}'", name)),
}
}

pub fn remove_importlib_frames(vm: &VirtualMachine, exc: &PyObjectRef) -> PyObjectRef {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure you want to add this? This might be a good opportunity to keep the rustpython interpreter transparent to users, and show them what is actually going on. What is your opinion on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that it gives too much information. I will add a TODO that when running on verbose this function will do nothing.

Copy link
Contributor

@windelbouwman windelbouwman left a comment

Choose a reason for hiding this comment

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

Except for a minor comment, this looks good to me!

vm/src/import.rs Outdated
if let Ok(tb) = vm.get_attribute(exc.clone(), "__traceback__") {
if objtype::isinstance(&tb, &vm.ctx.list_type()) {
let mut tb_entries = objsequence::get_elements_list(&tb).to_vec();
tb_entries.reverse();
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the list is reversed twice. This might not be required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

vm/src/import.rs Outdated
if objtype::isinstance(&tb, &vm.ctx.list_type()) {
let mut tb_entries = objsequence::get_elements_list(&tb).to_vec();
tb_entries.reverse();
let mut new_tb = Vec::with_capacity(tb_entries.len());
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this function is basically a filtering of a list, this might be an excellent opportunity to use the filter_map function, or filter function on rust iterators, and then use collect on the filtered iterator.

Something along this idea:

let new_tb = tb_entries.iter()
    .filter(|tb_entry| {
       // fetch filename etc..
       // funky filtering here, if file_name != frozen etc...
    })
    .collect();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is only a simple filter if the error is from outside the importlib code. That is why we remove only chunks that end in _call_with_frames_removed.

vm/src/import.rs Outdated
new_tb.push(tb_entry.clone())
} else {
current_chunk.push(tb_entry.clone());
let run_obj_name = objstr::get_value(&location_attrs[0]);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the same as file_name earlier on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch. Thanks.

vm/src/import.rs Outdated
let mut new_tb = Vec::with_capacity(tb_entries.len());

for tb_entry in tb_entries.iter() {
let mut current_chunk = vec![];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this variable can be removed right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@palaviv palaviv force-pushed the remove-importlib-frames branch from c9f01cc to 251e33a Compare July 6, 2019 06:16
@windelbouwman windelbouwman merged commit a099ba5 into RustPython:master Jul 6, 2019
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.

2 participants