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.

Sign up for GitHub

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

Jump to bottom

Fix overload resolution when conflict is with members of address #2936

Merged
merged 3 commits into from
Sep 29, 2017
Merged

Fix overload resolution when conflict is with members of address #2936

merged 3 commits into from
Sep 29, 2017

Conversation

axic
Copy link
Member

@axic axic commented Sep 20, 2017

Fixes #2655.

chriseth
chriseth reviewed Sep 20, 2017
View reviewed changes
libsolidity/analysis/TypeChecker.cpp Outdated
(
// Overloaded functions without declaration (e.g. transfer(), send(), call(), etc.)
it->type->category() == Type::Category::Function &&
!dynamic_cast<FunctionType const&>(*it->type).hasDeclaration()
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks rather hacky. What about removing the members as part of the type instead of the overload resolution?
I would like var f = x.balance; f(); to work in the same way as x.balance();. If you need access to the account balance, you can use address(x).balance.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is very hacky, should be removed entirely at the breaking release when a contract isn't an child of address.

Do you have a non-hacky solution for keeping the members in non-conflicting cases?

@chriseth
Copy link
Contributor

You could try it in ContractType::nativeMembers - although that might still not solve cases that use using for.

@axic axic force-pushed the proper-address-overload-resolution branch 4 times, most recently from 0192700 to 1ae3ee5 Compare September 27, 2017 11:17
axic
axic commented Sep 27, 2017
View reviewed changes
libsolidity/ast/Types.cpp Outdated
// Members must overload functions without clash
(
member.type->category() == Type::Category::Function &&
dynamic_cast<FunctionType const&>(*member.type).hasEqualArgumentTypes(dynamic_cast<FunctionType const&>(*it->type))
Copy link
Member Author

Choose a reason for hiding this comment

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

This is needed because simply comparing the type takes visibility specifiers into account, which is a can of worms.

@axic
Copy link
Member Author

axic commented Sep 27, 2017

@chriseth can you review please?

chriseth
chriseth reviewed Sep 27, 2017
View reviewed changes
test/libsolidity/SolidityNameAndTypeResolution.cpp
contract D {
function f() {
var x = (new C()).balance();
x;
Copy link
Contributor

Choose a reason for hiding this comment

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

This produces "statement does not have an effect" warning. If you want to force "no warnings" I think you should use CHECK_SUCCESS_NO_WARNINGS or something like that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I thought CHECK_SUCCESS_NO_WARNINGS stands for success and not even warnings. Maybe it should read CHECK_SUCCESS_IGNORE_WARNINGS .

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, that's what it stands for, but I thought that because of the comments above (to avoid pureness warning) you were aiming for a test without warnings.

Copy link
Member Author

Choose a reason for hiding this comment

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

So it ignores warnings or doesn't?

Actually I don't mind the warnings myself in this code.

chriseth
chriseth reviewed Sep 27, 2017
View reviewed changes
Changelog.md Outdated
@@ -2,6 +2,7 @@

Features:
* Parser: Better error message for unexpected trailing comma in parameter lists.
* Type Checker: Overload resolution should work with builtins of ``address``.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is rather about overwriting instead of overloading. What about Fix virtual function lookup when overwriting builtin members of ``address``.?

chriseth
chriseth reviewed Sep 27, 2017
View reviewed changes
libsolidity/ast/Types.cpp Outdated
@@ -1619,7 +1619,8 @@ string ContractType::canonicalName() const
MemberList::MemberMap ContractType::nativeMembers(ContractDefinition const*) const
{
// All address members and all interface functions
MemberList::MemberMap members(IntegerType(160, IntegerType::Modifier::Address).nativeMembers(nullptr));
MemberList::MemberMap const& addressMembers = IntegerType(160, IntegerType::Modifier::Address).nativeMembers(nullptr);
Copy link
Contributor

Choose a reason for hiding this comment

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

This function returns by value, doesn't it?

@axic axic force-pushed the proper-address-overload-resolution branch from 1ae3ee5 to e1eaf4e Compare September 27, 2017 13:05
@axic axic mentioned this pull request Sep 27, 2017
Merged
chriseth
chriseth reviewed Sep 27, 2017
View reviewed changes
libsolidity/ast/Types.cpp Outdated
for (auto it = addressMembers.begin(); it != addressMembers.end(); ++it)
{
bool clash = false;
for (auto const member: members)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you extract this into a function?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or perhaps everything startign from add overloads from address....?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking about the same but not sure it helps much. It is quite specific.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is quite specific, but it will make the containing function shorter.

Basically my thinking is: If you need a comment to describe what a block inside a function does, you should probable move that into its own function. The purpose of the helper function is clear from its name, so you don't need the comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@axic axic force-pushed the proper-address-overload-resolution branch from e1eaf4e to 5270947 Compare September 28, 2017 12:45
@axic axic force-pushed the proper-address-overload-resolution branch from 5270947 to f953b02 Compare September 28, 2017 12:57
@axic axic force-pushed the proper-address-overload-resolution branch from f953b02 to d5d1a08 Compare September 28, 2017 14:02
@chriseth chriseth merged commit b921846 into develop Sep 29, 2017
@axic axic deleted the proper-address-overload-resolution branch September 29, 2017 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chriseth chriseth chriseth approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@axic @chriseth