Conversation
friendlymatthew
force-pushed
the
hasattr
branch
from
ad96b08 to
e5e0154
Compare
friendlymatthew
force-pushed
the
hasattr
branch
from
e5e0154 to
873ee89
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| let Value::InternString(name_id) = name else { | ||
| return Err(SimpleException::new_msg(ExcType::TypeError, "hasattr(): attribute name must be string").into()); | ||
| }; |
There was a problem hiding this comment.
🔴 hasattr rejects heap-allocated strings as attribute name, unlike getattr
hasattr uses let Value::InternString(name_id) = name (line 38) which only matches interned string literals. The equivalent getattr implementation uses name.as_either_str(vm.heap) (crates/monty/src/builtins/getattr.rs:41) which correctly handles both Value::InternString and heap-allocated strings (Value::Ref → HeapData::Str). This means dynamically constructed strings (e.g., from concatenation like 'st' + 'art') passed as the attribute name to hasattr will incorrectly raise TypeError: attribute name must be string even though the value is a valid Python string.
Prompt for agents
In crates/monty/src/builtins/hasattr.rs, lines 38-43, the attribute name is checked with `let Value::InternString(name_id) = name` which only handles interned strings. It should instead use `name.as_either_str(vm.heap)` like getattr does in crates/monty/src/builtins/getattr.rs:41-46. This would handle both InternString and heap-allocated Str values. The pattern should be:
let Some(attr) = name.as_either_str(vm.heap) else {
let ty = name.py_type(vm);
return Err(SimpleException::new_msg(ExcType::TypeError, format!("attribute name must be string, not '{ty}'")).into());
};
Then use `&attr` instead of `&EitherStr::Interned(*name_id)` in the py_getattr call. You'll need to import PyTrait for py_type, and adjust the import of EitherStr if it's no longer used directly.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // | ||
| // TODO: might need to support this case? | ||
| return Err( | ||
| SimpleException::new_msg(ExcType::TypeError, "getattr(): attribute is not a simple value").into(), |
There was a problem hiding this comment.
🟡 Error message says "getattr()" instead of "hasattr()" due to copy-paste
Line 54 contains the error message "getattr(): attribute is not a simple value" but this is inside the builtin_hasattr function. This is a copy-paste error from the getattr implementation. The message should say "hasattr()" to correctly identify the calling function.
| SimpleException::new_msg(ExcType::TypeError, "getattr(): attribute is not a simple value").into(), | |
| SimpleException::new_msg(ExcType::TypeError, "hasattr(): attribute is not a simple value").into(), |
Was this helpful? React with 👍 or 👎 to provide feedback.
| hasattr(s, 123) | ||
| assert False, 'hasattr() with non-string name should raise TypeError' | ||
| except TypeError as e: | ||
| assert 'attribute name must be string' in str(e), 'Error message should mention string requirement' |
There was a problem hiding this comment.
🔴 Test uses assert 'x' in str(e) pattern which violates CLAUDE.md rule
CLAUDE.md explicitly states: "Do NOT Write tests like assert 'thing' in msg it's lazy and inexact unless explicitly told to do so, instead write tests like assert msg == 'expected message'". Line 44 uses assert 'attribute name must be string' in str(e) which is exactly this prohibited pattern.
| assert 'attribute name must be string' in str(e), 'Error message should mention string requirement' | |
| assert str(e) == 'hasattr(): attribute name must be string', 'Error message should mention string requirement' |
Was this helpful? React with 👍 or 👎 to provide feedback.
| except TypeError: | ||
| pass |
There was a problem hiding this comment.
🔴 Test catches TypeError without checking exception message (no-args case)
CLAUDE.md explicitly states: "IMPORTANT: don't just check that an exception is raised, you should always check the exception message." The try/except block at lines 22-26 catches TypeError for hasattr() with no args but only does pass without verifying str(exc).
| except TypeError: | |
| pass | |
| except TypeError as e: | |
| assert str(e) == 'hasattr expected 2 arguments, got 0', 'hasattr() no-args error message' |
Was this helpful? React with 👍 or 👎 to provide feedback.
| except TypeError: | ||
| pass |
There was a problem hiding this comment.
🔴 Test catches TypeError without checking exception message (1-arg case)
CLAUDE.md explicitly states: "IMPORTANT: don't just check that an exception is raised, you should always check the exception message." The try/except block at lines 28-32 catches TypeError for hasattr(s) but only does pass without verifying str(exc).
| except TypeError: | |
| pass | |
| except TypeError as e: | |
| assert str(e) == 'hasattr expected 2 arguments, got 1', 'hasattr() 1-arg error message' |
Was this helpful? React with 👍 or 👎 to provide feedback.
| except TypeError: | ||
| pass |
There was a problem hiding this comment.
🔴 Test catches TypeError without checking exception message (3-args case)
CLAUDE.md explicitly states: "IMPORTANT: don't just check that an exception is raised, you should always check the exception message." The try/except block at lines 34-38 catches TypeError for hasattr(s, 'start', 'extra') but only does pass without verifying str(exc).
| except TypeError: | |
| pass | |
| except TypeError as e: | |
| assert str(e) == 'hasattr expected 2 arguments, got 3', 'hasattr() 3-args error message' |
Was this helpful? React with 👍 or 👎 to provide feedback.
| except TypeError: | ||
| pass |
There was a problem hiding this comment.
🔴 Test catches TypeError without checking exception message (None name case)
CLAUDE.md explicitly states: "IMPORTANT: don't just check that an exception is raised, you should always check the exception message." The try/except block at lines 46-50 catches TypeError for hasattr(s, None) but only does pass without verifying str(exc).
| except TypeError: | |
| pass | |
| except TypeError as e: | |
| assert str(e) == 'hasattr(): attribute name must be string', 'hasattr() None-name error message' |
Was this helpful? React with 👍 or 👎 to provide feedback.
| SimpleException::new_msg(ExcType::TypeError, "getattr(): attribute is not a simple value").into(), | ||
| ); | ||
| } | ||
| Err(_) => false, |
There was a problem hiding this comment.
🚩 hasattr swallows all exception types, not just AttributeError
In CPython, hasattr(obj, name) calls getattr(obj, name) and catches only AttributeError — any other exception type propagates to the caller. The Monty implementation at crates/monty/src/builtins/hasattr.rs:57 uses Err(_) => false which catches ALL error types indiscriminately and returns False. This means that if py_getattr raises something other than AttributeError (e.g., a RuntimeError from a custom __getattr__), hasattr would silently return False instead of propagating the exception. This is a semantic divergence from CPython's behavior. Currently this may not be triggerable since Monty doesn't support custom __getattr__ on user classes, but it's worth noting for when that support is added.
Was this helpful? React with 👍 or 👎 to provide feedback.
| Ok(_) => { | ||
| // hasattr() only tests attribute values — OS calls, external calls, | ||
| // method calls, and awaits are not supported here | ||
| // | ||
| // TODO: might need to support this case? | ||
| return Err( | ||
| SimpleException::new_msg(ExcType::TypeError, "getattr(): attribute is not a simple value").into(), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🚩 Ok(_) branch in hasattr raises TypeError instead of returning False
The Ok(_) branch at crates/monty/src/builtins/hasattr.rs:48-56 handles cases where py_getattr returns a non-Value CallResult (e.g., an OS call or external function). In this case, hasattr raises a TypeError. This is debatable — since the attribute technically exists (getattr succeeded), returning True might be more correct Python semantics. The TODO comment acknowledges uncertainty. This is unlikely to be hit in practice for typical hasattr use cases, but if it does get hit, it would be surprising to get a TypeError from hasattr() which is documented as never raising exceptions other than for bad arguments.
Was this helpful? React with 👍 or 👎 to provide feedback.
Codecov Results 📊✅ Patch coverage is 84.21%. Project has 4126 uncovered lines. Files with missing lines (1)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 83.78% 86.45% +2.67%
==========================================
Files 118 119 +1
Lines 31354 30456 -898
Branches 71312 65491 -5821
==========================================
+ Hits 26269 26330 +61
- Misses 5085 4126 -959
- Partials 3408 1925 -1483Generated by Codecov Action |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
2 participants
This PR implements
hasattr(object,name)which returnsTrueif the object has the named attribute,Falseotherwise. Unlike #65, this function never raises an AttributeErrorThis implementation calls
py_get_attr()directly and converts the result to a boolean rather than reusingbuiltin_getattr(), since the functions have different signatures to validate