-
Notifications
You must be signed in to change notification settings - Fork 6.3k
User defined value types #11806
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
User defined value types #11806
Conversation
73c2097 to
1b14c2c
Compare
54411e7 to
99ea7e8
Compare
docs/types/value-types.rst
Outdated
| ``==`` is disallowed. One can use operators or bound member functions by explicitly converting it | ||
| to the underlying type. | ||
|
|
||
| The following example demonstrates a custom type ``MyAddress`` which is an abstraction over the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we use an example that also shows the usefulness of this concept?
99ea7e8 to
4a030b4
Compare
test/libsolidity/syntaxTests/userDefinedValueType/all_value_types.sol
Outdated
Show resolved
Hide resolved
| type MyAddress is address; | ||
|
|
||
| function f() pure { | ||
| // TODO Should we allow explicit conversions from literals? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think yes. This is similar to how newtype semantics work in haskell and rust (?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not too sure about it - wouldn't it create ambiguities? We have to at least check that implicit conversion from the literal to the underlying type is possible.
4a030b4 to
55683a7
Compare
libsolidity/ast/Types.h
Outdated
| } | ||
| bool nameable() const override { return m_underlyingType.nameable(); } | ||
|
|
||
| std::string toString(bool _short) const override { return "UserDefinedValueType(" + m_underlyingType.toString(_short) + ")"; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this also include the name of the type? (as we do for struct)
| } | ||
| else if (m_actualType->category() == Category::UserDefinedValueType) | ||
| { | ||
| auto& userDefined = dynamic_cast<UserDefinedValueType const&>(*m_actualType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| auto& userDefined = dynamic_cast<UserDefinedValueType const&>(*m_actualType); | |
| auto const& userDefined = dynamic_cast<UserDefinedValueType const&>(*m_actualType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the const here is redundant, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is, but may be nice to mention it - we usually do.
| if (kind == FunctionType::Kind::Wrap) | ||
| solAssert(argumentType->isImplicitlyConvertibleTo(userDefinedType->underlyingType()), ""); | ||
|
|
||
| acceptAndConvert(*arguments[0], *function.parameterTypes()[0], true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need to do cleanup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you assert that the implicit conversion is possible, please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the cleanup and added one more assert.
| Type const* argumentType = arguments.at(0)->annotation().type; | ||
| solAssert(argumentType, ""); | ||
| FunctionType::Kind kind = functionType->kind(); | ||
| auto userDefinedType = kind == FunctionType::Kind::Wrap ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be easier and less confusing to retrieve this from _functionCall.expression? Maybe not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On that note test cases for stuff like (MyType).wrap and the like would be good.
b2b01ae to
0cdac18
Compare
docs/abi-spec.rst
Outdated
| +-------------------------------+-----------------------------------------------------------------------------+ | ||
| |:ref:`enum<enums>` |``uint8`` | | ||
| +-------------------------------+-----------------------------------------------------------------------------+ | ||
| |:ref:`user defined value types |exactly as its underlying value type | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, the "exactly" was not meant literally. The text above table says "on the right column the ABI types that represent them", so it should be enough to say "its underlying value type" or "the underlying value type".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
| case FunctionType::Kind::Wrap: | ||
| case FunctionType::Kind::Unwrap: | ||
| { | ||
| typeCheckFunctionGeneralChecks(_functionCall, functionType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah nice!
| else | ||
| solAssert(type(*arguments.at(0)).category() == Type::Category::UserDefinedValueType, ""); | ||
|
|
||
| define(_functionCall, *arguments.at(0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually skips one conversion - but would be fine for me, since the conversion is a no-op (and I hope it stays like that).
0cdac18 to
a04be1c
Compare
a04be1c to
7e95555
Compare
| * User defined value types, i.e., custom types, for example, `type MyInt is int`. Allows creating a | ||
| * zero cost abstraction over value type with stricter type requirements. | ||
| */ | ||
| class UserDefinedValueTypeDefinition: public Declaration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think this is a really bad name for the node :-D - but well.
| (MyInt).wrap; | ||
| (MyInt).wrap(5); | ||
| (MyInt).unwrap; | ||
| (MyInt).unwrap(MyInt.wrap(5)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Syntactically this is really not interesting - this should be a semantics test!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can move this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did it here: #11914
fulldecent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fixed math library claims the division function is to the "nearest integer" but actually it rounds down.
|
@fulldecent Thanks for noticing. Will update the comment. |
|
Thank you! Looks good now |
Closes #11531
TODO
NatSpec?