Err instead of panic for host object attribute error #288

ychenfo · 2022-05-15T02:09:52+08:00

ychenfo commented

2022-05-15 02:09:52 +08:00

This is a fix to #281: proper error message is returned now instead of panic

sb10q reviewed 2022-05-16 12:34:38 +08:00

nac3artiq/src/symbol_resolver.rs Outdated

													
				@ -674,0 +674,4 @@

				                                Ok(d) => d,

				                                Err(e) => {

				                                    if e.is_instance::<PyAttributeError>(py) {

				                                        return Ok(Err(format!("{}", e).replace("AttributeError: ", "")));

sb10q commented

2022-05-16 12:34:38 +08:00

Why?

sb10q commented

2022-05-16 12:36:19 +08:00

Won't other types of errors have the same issue? I don't see what's special about AttributeError. The default behavior of printing AttributeError("...") seems acceptable to me (for now) and should be simple enough.

Won't other types of errors have the same issue? I don't see what's special about AttributeError. The default behavior of printing ``AttributeError("...")`` seems acceptable to me (for now) and should be simple enough.

ychenfo commented

2022-05-17 01:05:39 +08:00

Poster

Here I think that it should only catch AttributeError because we are using getattr to get the member of an object, so we are only expecting AttributeError to happen here. Other errors should not happen.

Here I think that it should only catch `AttributeError` because we are using `getattr` to get the member of an object, so we are only expecting `AttributeError` to happen here. Other errors should not happen.

ychenfo commented

2022-05-17 01:07:17 +08:00

Poster

Hmm maybe it's better to use unreachable! to mark that internal error and panic here?

Hmm maybe it's better to use `unreachable!` to mark that internal error and panic here?

ychenfo force-pushed 281-host-obj-attribute-err from ba0de43526 to 54b7af270d

2022-05-17 01:12:29 +08:00

Compare

ychenfo commented

2022-05-17 01:13:47 +08:00

Poster

force-pushed to use unreachable! to handle internal error in which exception other than AttributeError are thrown from python interpreter

force-pushed to use `unreachable!` to handle internal error in which exception other than `AttributeError` are thrown from python interpreter

sb10q commented

2022-05-17 10:59:41 +08:00

No, there can be other exceptions. For example:

>>> class X:
...   def __getattr__(self, attr):
...     raise TypeError
... 
>>> getattr(X(), "foo")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 3, in __getattr__
TypeError

Please remove the special-casing.

No, there can be other exceptions. For example: ``` >>> class X: ... def __getattr__(self, attr): ... raise TypeError ... >>> getattr(X(), "foo") Traceback (most recent call last): File "<stdin>", line 1, in <module> File "<stdin>", line 3, in __getattr__ TypeError ``` Please remove the special-casing.

ychenfo force-pushed 281-host-obj-attribute-err from 54b7af270d to 09820e5aed

2022-05-18 02:54:47 +08:00

Compare

ychenfo commented

2022-05-18 03:00:09 +08:00

Poster

Please remove the special-casing.

Thanks for pointing this out! Force-pushed to catch all types of exceptions and updated to the latest master branch.

> Please remove the special-casing. Thanks for pointing this out! Force-pushed to catch all types of exceptions and updated to the latest master branch.

sb10q merged commit 09820e5aed into master

2022-05-18 06:36:08 +08:00

sb10q deleted branch 281-host-obj-attribute-err

2022-05-18 06:36:08 +08:00

Sign in to join this conversation.

No reviewers