Skip to content
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

Fix exception swallowing by Trait attribute access #959

Merged
merged 3 commits into from
Mar 26, 2020

Conversation

mdickinson
Copy link
Member

@mdickinson mdickinson commented Mar 25, 2020

When doing obj.attr_name on a HasTraits object obj, if attr_name doesn't appear in either the class or instance dictionaries, Traits next tries to look it up as a regular Python attribute (via the PyObject_GenericGetAttr C-API function). If that lookup fails with an exception of any kind, the exception is swallowed and Traits then moves on to try the general prefix-trait machinery. In effect, there's a bare except here, but at C level.

This PR modifies the behaviour to replace the bare except with an except AttributeError, so that exceptions other than AttributeError will no longer be swallowed.

The fix itself is easy. Writing a regression test that worked was ... more interesting.

This fixes one case of #946.

Copy link
Contributor

@corranwebster corranwebster left a comment

Choose a reason for hiding this comment

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

LGTM.

I assume we have a text case somewhere where AttributeError is correctly raised by the regular access machinery and processing carries on.

@mdickinson
Copy link
Member Author

I assume we have a text case somewhere where AttributeError is correctly raised by the regular access machinery and processing carries on.

I'll double check. But I'd be surprised if we didn't: this should be a very common case.

@mdickinson
Copy link
Member Author

I'll double check. But I'd be surprised if we didn't: this should be a very common case.

Hmm. Lots of occurrences (I count 35 cases from running the test suite), but nothing that's directly exercising this case. I'll add another test.

@codecov-io
Copy link

codecov-io commented Mar 26, 2020

Codecov Report

Merging #959 into master will increase coverage by 0.10%.
The diff coverage is 93.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #959      +/-   ##
==========================================
+ Coverage   73.05%   73.16%   +0.10%     
==========================================
  Files          51       51              
  Lines        6514     6507       -7     
  Branches     1309     1308       -1     
==========================================
+ Hits         4759     4761       +2     
+ Misses       1363     1352      -11     
- Partials      392      394       +2     
Impacted Files Coverage Δ
traits/has_traits.py 71.24% <ø> (ø)
traits/trait_handlers.py 61.11% <ø> (ø)
traits/trait_types.py 71.35% <93.33%> (-0.30%) ⬇️
traits/editor_factories.py 79.59% <0.00%> (-9.19%) ⬇️
traits/traits.py 76.92% <0.00%> (+4.04%) ⬆️
traits/etsconfig/etsconfig.py 63.46% <0.00%> (+6.41%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aab02e9...531a713. Read the comment docs.

@corranwebster
Copy link
Contributor

Additional tests look good 👍

@mdickinson mdickinson merged commit 97ff939 into master Mar 26, 2020
@mdickinson mdickinson deleted the fix/swallowed-getattr-error branch March 26, 2020 10:59
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.

None yet

3 participants