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

Implement override keyword #7563

Merged
merged 5 commits into from Oct 31, 2019

Conversation

@Marenz
Copy link
Contributor

Marenz commented Oct 23, 2019

Part of #5424

@Marenz Marenz mentioned this pull request Oct 23, 2019
@Marenz Marenz force-pushed the override-rewrite-5424 branch from 797939e to c354b98 Oct 23, 2019
@Marenz Marenz requested review from chriseth, christianparpart and erak Oct 23, 2019
@Marenz Marenz force-pushed the override-rewrite-5424 branch from c354b98 to fe19d35 Oct 23, 2019
@@ -6,4 +6,4 @@ contract B {
}
contract C is A, B {}
// ----
// TypeError: (81-123): Overriding function visibility differs.
// TypeError: (126-147): Functions of the same name f and parameter types defined in two or more base contracts must be overridden in the derived contract.

This comment has been minimized.

Copy link
@erak

erak Oct 23, 2019

Contributor
Suggested change
// TypeError: (126-147): Functions of the same name f and parameter types defined in two or more base contracts must be overridden in the derived contract.
// TypeError: (126-147): Derived contract must override function `f`. Function with the same name and parameter types defined in two or more base contracts.
function set() public {}
}
contract A is I {
uint a;

This comment has been minimized.

Copy link
@erak

erak Oct 23, 2019

Contributor

Needs 4 whitespaces.

/// Checks for functions in different base contracts that conflict with each
/// other and thus need to be overridden explicitly
Comment on lines 96 to 97

This comment has been minimized.

Copy link
@erak

erak Oct 23, 2019

Contributor
Suggested change
/// other and thus need to be overridden explicitly
/// Checks for functions in different base contracts which conflict with each
/// other and thus need to be overridden explicitly.

langutil::ErrorReporter& m_errorReporter;

/// Map of multisets that contain all overridable functions for the given

This comment has been minimized.

Copy link
@erak

erak Oct 23, 2019

Contributor
Suggested change
/// Map of multisets that contain all overridable functions for the given
/// Map of multisets containing all functions which can be overriden for the given contract.

This comment has been minimized.

Copy link
@Marenz

Marenz Oct 24, 2019

Author Contributor

you really don't like "that" compared to "which" :P

function foo() public override(Base1, Base2) {}
}

.. index:: ! overriding;function

This comment has been minimized.

Copy link
@chriseth

chriseth Oct 24, 2019

Contributor

Doesn't the index entry need to be mentioned before the section header?

This comment has been minimized.

Copy link
@Marenz

Marenz Oct 24, 2019

Author Contributor

I tried imitating what I saw, not sure how it works

This comment has been minimized.

Copy link
@Marenz

Marenz Oct 24, 2019

Author Contributor

I think it's fine now

function foo() public override {}
}

For multiple inheritance, all direct base contracts must be specified

This comment has been minimized.

Copy link
@chriseth

chriseth Oct 24, 2019

Contributor
Suggested change
For multiple inheritance, all direct base contracts must be specified
For multiple inheritance, all direct base contracts that define the same function must be specified
Changelog.md Outdated Show resolved Hide resolved
@Marenz Marenz force-pushed the override-rewrite-5424 branch from 023d617 to 4ee94d0 Oct 24, 2019
@@ -87,6 +90,47 @@ Function modifiers can be used to amend the semantics of functions in a declarat
}
}


Function modifiers can override each other. This works similar as function

This comment has been minimized.

Copy link
@erak

erak Oct 24, 2019

Contributor
Suggested change
Function modifiers can override each other. This works similar as function
Function modifiers can override each other. This works similar to function

This comment has been minimized.

Copy link
@Marenz

Marenz Oct 28, 2019

Author Contributor

@ChrisChinchilla we need your authority in relation to the English language here. How should this be phrased?

This comment has been minimized.

Copy link
@ChrisChinchilla

ChrisChinchilla Oct 28, 2019

Contributor

@erak was mostly right, but maybe one small thing.

Suggested change
Function modifiers can override each other. This works similar as function
Function modifiers can override each other. This works in a similar way to functions

This comment has been minimized.

Copy link
@chriseth

chriseth Oct 28, 2019

Contributor

Is it similar or identical?

@@ -87,6 +90,47 @@ Function modifiers can be used to amend the semantics of functions in a declarat
}
}


Function modifiers can override each other. This works similar as function
overriding. The override keyword must be used in the overriding contract:

This comment has been minimized.

Copy link
@erak

erak Oct 24, 2019

Contributor
Suggested change
overriding. The override keyword must be used in the overriding contract:
overriding. The ``override`` keyword must be used in the overriding contract:
@@ -87,6 +90,47 @@ Function modifiers can be used to amend the semantics of functions in a declarat
}
}


This comment has been minimized.

Copy link
@erak

erak Oct 24, 2019

Contributor

Perhaps move this section to inheritance.rst and link it here? This way, we would keep inheritance specs in one place.

@@ -110,6 +110,8 @@ struct FunctionDefinitionAnnotation: ASTAnnotation, DocumentedAnnotation
/// The function this function overrides, if any. This is always the closest
/// in the linearized inheritance hierarchy.
FunctionDefinition const* superFunction = nullptr;
/// Reference to the contract this function is defined in

This comment has been minimized.

Copy link
@erak

erak Oct 24, 2019

Contributor
Suggested change
/// Reference to the contract this function is defined in
/// Pointer to the contract this function is defined in.

This comment has been minimized.

Copy link
@chriseth

chriseth Oct 28, 2019

Contributor

Is this different from FunctionDefinition::m_scope (defined in the Scopable base class)?

function test() internal override returns (uint256);
}
// ----
// TypeError: (184-290): Derived contract must override function foo. Function with the same name and parameter types defined in two or more base classes.

This comment has been minimized.

Copy link
@erak

erak Oct 24, 2019

Contributor
Suggested change
// TypeError: (184-290): Derived contract must override function foo. Function with the same name and parameter types defined in two or more base classes.
// TypeError: (184-290): Derived contract must override function "foo". Function with the same name and parameter types defined in two or more base classes.

This comment has been minimized.

Copy link
@Marenz

Marenz Oct 24, 2019

Author Contributor

hmm other error messages didn't use quotes around the function name...

This comment has been minimized.

Copy link
@erak

erak Oct 24, 2019

Contributor

They do, e.g. syntaxTests/array/no_array_pop.sol

contract C {
    uint data;
    function test() public {
      data.pop();
    }
}
// ----
// TypeError: (63-71): Member "pop" not found or not visible after argument-dependent lookup in uint256.
@@ -80,8 +93,23 @@ class ContractLevelChecker
void checkLibraryRequirements(ContractDefinition const& _contract);
/// Checks base contracts for ABI compatibility
void checkBaseABICompatibility(ContractDefinition const& _contract);
/// Checks for functions in different base contracts which conflict with each
/// other and thus need to be overridden explicitly

This comment has been minimized.

Copy link
@erak

erak Oct 24, 2019

Contributor

I'd prefer to end all sentences properly with a . ;-)

@erak

This comment has been minimized.

Copy link
Contributor

erak commented Oct 24, 2019

Looks good, but still left some minor comments. One thing we definitely need is a paragraph in 060_breaking_changes.rst.

Copy link
Contributor

ChrisChinchilla left a comment

Just some small thoughts

Changelog.md Outdated Show resolved Hide resolved
}

contract Final is Base1, Base2 {
function kill() public override(Base1, Base2) { Base2.kill(); }

This comment has been minimized.

Copy link
@chriseth

chriseth Oct 28, 2019

Contributor

Maybe better to also use super.kill() here.

}

For multiple inheritance, the most derived base contracts that defines the same
function must be specified explicitly. Additionally, the contract deriving from multiple bases

This comment has been minimized.

Copy link
@chriseth

chriseth Oct 28, 2019

Contributor
Suggested change
function must be specified explicitly. Additionally, the contract deriving from multiple bases
function must be specified explicitly after the ``override`` keyword. Additionally, if a contract inherits the same function from multiple (unrelated) bases, it has to explicitly override it:
if (!checkFunctionOverride(_function, **begin))
break;

auto const result = find(

This comment has been minimized.

Copy link
@chriseth

chriseth Oct 28, 2019

Contributor

Instead of these ten lines of code, it might be better to build up a set of contracts called 'expectedContracts' containing all (*begin)->annotation().contract and then after the loop, use auto missingContracts = expectedContracts - specifiedContracts and invalidContracts = specifiedContracts - expectedContracts.

Copy link
Contributor

chriseth left a comment

Apart from checking based on ID instead of path name and some stylistic things this looks good!

@Marenz Marenz force-pushed the override-rewrite-5424 branch from cc50d37 to 0ca3773 Oct 29, 2019
@Marenz

This comment has been minimized.

Copy link
Contributor Author

Marenz commented Oct 29, 2019

Pushed latest changes

@Marenz Marenz force-pushed the override-rewrite-5424 branch from 0ca3773 to f911a99 Oct 29, 2019
@chriseth chriseth mentioned this pull request Oct 30, 2019
4 of 5 tasks complete
For multiple inheritances, the most derived base contracts that defines the same
function must be specified explicitly after the ``override`` keyword.
Additionally, if a contract inherits the same function from multiple (unrelated)
bases, it has to explicitly override it:
Comment on lines 203 to 207

This comment has been minimized.

Copy link
@chriseth

chriseth Oct 30, 2019

Contributor
Suggested change
bases, it has to explicitly override it:
For multiple inheritance, the most derived base contracts that define the same
function must be specified explicitly after the ``override`` keyword.
In other words, you have to specify all base contracts that define the same function and have not yet been overridden by another base contract (on some path through the inheritance graph).
Additionally, if a contract inherits the same function from multiple (unrelated)
bases, it has to explicitly override it:
function foo() public override(Base1, Base2) {}
}

A function defined in a common base contract must not be explicitly overridden

This comment has been minimized.

Copy link
@chriseth

chriseth Oct 30, 2019

Contributor
Suggested change
A function defined in a common base contract must not be explicitly overridden
A function defined in a common base contract does not have to be explicitly overridden
Modifier Overriding
===================

Function modifiers can override each other. This works in a similar way to function

This comment has been minimized.

Copy link
@chriseth

chriseth Oct 30, 2019

Contributor

Either "same way" or "same way (up to the fact that there is no overloading for modifiers)"

@@ -67,6 +67,11 @@ Function Modifiers
Function modifiers can be used to amend the semantics of functions in a declarative way
(see :ref:`modifiers` in the contracts section).

A function modifier must have a unique name. Overloading, that is, having the

This comment has been minimized.

Copy link
@chriseth

chriseth Oct 30, 2019

Contributor

Maybe clearer like that

Suggested change
A function modifier must have a unique name. Overloading, that is, having the
Overloading, that is, having the

?

(unique to what?)

// Check for duplicates in override list
if (_function.overrides() && specifiedContracts.size() != _function.overrides()->overrides().size())
{
// Sort by name to find duplicate for error reporting

This comment has been minimized.

Copy link
@chriseth

chriseth Oct 30, 2019

Contributor

Comment is not updated.

{
Declaration const* baseDecl =
specifier->name().annotation().referencedDeclaration;
resolvedContracts.emplace_back(dynamic_cast<ContractDefinition const*>(baseDecl));

This comment has been minimized.

Copy link
@chriseth

chriseth Oct 30, 2019

Contributor

Can we assert that this is not null or will it break stuff?

m_contractBaseFunctions[_contract] = set;
}

return m_contractBaseFunctions[_contract];

This comment has been minimized.

Copy link
@chriseth

chriseth Oct 30, 2019

Contributor

Why is this not called m_inheritedFunctions?

list[i]->location(),
ssl,
"Duplicate contract \"" +
joinHumanReadable(list[i]->namePath(), ".") +

This comment has been minimized.

Copy link
@chriseth

chriseth Oct 30, 2019

Contributor

This does not have to be indented

if (expectedContracts.size() > 1)
missingContracts = expectedContracts - specifiedContracts;

invalidContracts = specifiedContracts - expectedContracts;

This comment has been minimized.

Copy link
@chriseth

chriseth Oct 30, 2019

Contributor
Suggested change
invalidContracts = specifiedContracts - expectedContracts;
surplusContracts = specifiedContracts - expectedContracts;

?

}
}

decltype(specifiedContracts) missingContracts;

This comment has been minimized.

Copy link
@chriseth

chriseth Oct 30, 2019

Contributor

Maybe declare missing and invalid closer to where they are used?


if (!_function.overrides())
{
overrideError(_function, _super, "Overriding function is missing 'override' specifier.");

This comment has been minimized.

Copy link
@chriseth

chriseth Oct 30, 2019

Contributor

Don't we detect that somewhere else already?

This comment has been minimized.

Copy link
@Marenz

Marenz Oct 30, 2019

Author Contributor

maybe in the past but this specific error is only generated here...


// Function has been explicitly overridden
if (contains_if(
contractFuncs,

This comment has been minimized.

Copy link
@chriseth

chriseth Oct 30, 2019

Contributor

Only single indentation

m_errorReporter.typeError(
modifier->location(),
"Override changes modifier signature."
);

This comment has been minimized.

Copy link
@chriseth

chriseth Oct 30, 2019

Contributor

Indentation

@Marenz Marenz force-pushed the override-rewrite-5424 branch from 524e7b4 to 6c6a905 Oct 30, 2019
@chriseth chriseth merged commit 49e8093 into develop_060 Oct 31, 2019
24 checks passed
24 checks passed
ci/circleci: b_archlinux Your tests passed on CircleCI!
Details
ci/circleci: b_docs Your tests passed on CircleCI!
Details
ci/circleci: b_ems Your tests passed on CircleCI!
Details
ci/circleci: b_osx Your tests passed on CircleCI!
Details
ci/circleci: b_ubu Your tests passed on CircleCI!
Details
ci/circleci: b_ubu18 Your tests passed on CircleCI!
Details
ci/circleci: b_ubu_asan Your tests passed on CircleCI!
Details
ci/circleci: b_ubu_clang Your tests passed on CircleCI!
Details
ci/circleci: b_ubu_cxx20 Your tests passed on CircleCI!
Details
ci/circleci: b_ubu_ossfuzz Your tests passed on CircleCI!
Details
ci/circleci: b_ubu_release Your tests passed on CircleCI!
Details
ci/circleci: chk_buglist Your tests passed on CircleCI!
Details
ci/circleci: chk_coding_style Your tests passed on CircleCI!
Details
ci/circleci: chk_proofs Your tests passed on CircleCI!
Details
ci/circleci: chk_spelling Your tests passed on CircleCI!
Details
ci/circleci: t_ems_solcjs Your tests passed on CircleCI!
Details
ci/circleci: t_osx_cli Your tests passed on CircleCI!
Details
ci/circleci: t_ubu_asan_cli Your tests passed on CircleCI!
Details
ci/circleci: t_ubu_asan_constantinople Your tests passed on CircleCI!
Details
ci/circleci: t_ubu_clang_soltest Your tests passed on CircleCI!
Details
ci/circleci: t_ubu_cli Your tests passed on CircleCI!
Details
ci/circleci: t_ubu_release_cli Your tests passed on CircleCI!
Details
ci/circleci: t_ubu_release_soltest Your tests passed on CircleCI!
Details
ci/circleci: t_ubu_soltest Your tests passed on CircleCI!
Details
@chriseth chriseth deleted the override-rewrite-5424 branch Oct 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.