Applying type_spec_abi_is_assignable_to to ABI computed type
#567
Conversation
pyteal/ast/abi/address.py
Outdated
| match value: | ||
| case ComputedValue(): | ||
| # TODO need to see again if we need to change on type system or not, _set_with_computed_type? | ||
| pts = value.produced_type_spec() | ||
| if pts == AddressTypeSpec() or pts == StaticArrayTypeSpec( | ||
| ByteTypeSpec(), AddressLength.Bytes |
There was a problem hiding this comment.
question on this part: we let type_spec_is_assignable_to to be false when we want to check if byte[32] is assignable to address, while in this case, we do want to assign byte[32] to address.
What should be changed:
- the type system?
- or the implementation, say we leave a note here for special corner case?
There was a problem hiding this comment.
It's a good question.
My 2c and happy to discuss live: I favor changing the type system because it feels analogous to the bi-directional treatment of NameTuple and Tuple.
There was a problem hiding this comment.
I agree the type system to be changed to not allow Address().set(StaticArrary[Bytes, 32]()) (and the ComputedValue variant)
There was a problem hiding this comment.
Summarizing a verbal with @ahangsu and @jasonpaulos with these action items that @ahangsu will follow up on:
- Realized there's an inconsistency in
type_spec_is_assignable_to. Agreed to disallow assignment of Tuple to NamedTuple. Address.setshould followtype_spec_is_assignable_torules. Implies:- Cannot assign
StaticBytes[32]toAddress. Remove it from method signature. - Offer a util method like
unsafe_castto enable conversion when the user knows conversion is safe.
- Cannot assign
- Revert the following assignability change: 6da7ab4#diff-61b0d50c921418a3bcbad74d9887b354c015606277515b24d0094e7e5e8ae3c0L526.
Addresscan be assigned toStaticBytes[32].
There was a problem hiding this comment.
change reflected in 3e776f8
There was a problem hiding this comment.
defer unsafe_cast implementation to a PR after this one get merged.
There was a problem hiding this comment.
@ahangsu Thanks for the changes + unsafe_cast in follow on PR - I'm good to resolve.
I have a new open question, but can move discussion out of this thread on my side.
There was a problem hiding this comment.
just a reminder, #580 carries the unsafe_cast implementation, we can always rebase to this PR and get all the code together.
The final question is on another thread, i.e., on the type signature of Address.set, and the behavior related with mypy.
| @@ -58,18 +59,14 @@ def get(self) -> Expr: | |||
| """ | |||
| return self.stored_value.load() | |||
|
|
|||
| def set( | |||
| def set( # type: ignore[override] | |||
There was a problem hiding this comment.
I'm not sure which is preferable - What do folks think?
Options:
- Keep the PR as is with mypy ignore.
- Re-add removed type parameters with usage of the to-be-added
castmethod.
During our live discussion yesterday, I didn't realize the type error will happen. It feels to me like we should not ignore the error.
Another alternative (that's more expansive in scope) is to rework Address so that it extends TypeSpec and uses composition for value storage/retrieval.
| or value.type_spec() | ||
| == StaticArrayTypeSpec(ByteTypeSpec(), AddressLength.Bytes) | ||
| ): | ||
| if type_spec_is_assignable_to(value.type_spec(), AddressTypeSpec()): | ||
| return self.stored_value.store(value.stored_value.load()) | ||
|
|
There was a problem hiding this comment.
@ahangsu Unrelated to the line - Based on the PR's changes, the TealInputError at the end of the method should remove StaticArray from its message.
Not requesting an immediate change because there's more pressing threads open. I leave the question to avoid forgetting the concern prior to merge.
|
closing for #583 opened, and more thinking need to be done, as we realize the scope of the problem is much larger. |
lifting strict equality type spec check to relaxed type check