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

Show unimplemented function if trying to instantiate an abstract class #2687

Merged
merged 4 commits into from Aug 4, 2017

Conversation

Projects
None yet
2 participants
@axic
Member

axic commented Aug 2, 2017

Fixes #1712.

@axic axic added the in progress label Aug 2, 2017

_newExpression.location(),
SecondarySourceLocation().append(
"Missing implementation:",
contract->annotation().unimplementedFunctions.front()->location()

This comment has been minimized.

@axic

axic Aug 2, 2017

Member

Since we do not support a list of secondarylocation yet (I think it may be a good idea), this just prints the first match now.

@axic

axic Aug 2, 2017

Member

Since we do not support a list of secondarylocation yet (I think it may be a good idea), this just prints the first match now.

@axic axic requested a review from chriseth Aug 2, 2017

@axic axic added the nextrelease label Aug 3, 2017

// Set to not fully implemented if at least one flag is false.
for (auto const& it: functions)
for (auto const& funAndFlag: it.second)
if (!funAndFlag.second)
{
_contract.annotation().isFullyImplemented = false;
return;
FunctionDefinition const* function = dynamic_cast<FunctionDefinition const*>(&funAndFlag.first->declaration());

This comment has been minimized.

@axic

axic Aug 4, 2017

Member

Instead of this cast/lookup, the funAndFlag pair could be made into a tuple and include 3 items: flag, type, definition.

@axic

axic Aug 4, 2017

Member

Instead of this cast/lookup, the funAndFlag pair could be made into a tuple and include 3 items: flag, type, definition.

@@ -199,7 +196,7 @@ void TypeChecker::checkContractAbstractFunctions(ContractDefinition const& _cont
if (!function->isImplemented())
// Base contract's constructor is not fully implemented, no way to get
// out of this.
allBaseConstructorsImplemented = false;
_contract.annotation().unimplementedFunctions.push_back(function);

This comment has been minimized.

@chriseth

chriseth Aug 4, 2017

Contributor

I think this case is different from the other "unimplemented functions" case. For error reporting, it can be the same, but I think the annotations deserve a separate boolean for this case. Please also document that unimplementedFuctions can also contains functions from base classes, especially constructors.

@chriseth

chriseth Aug 4, 2017

Contributor

I think this case is different from the other "unimplemented functions" case. For error reporting, it can be the same, but I think the annotations deserve a separate boolean for this case. Please also document that unimplementedFuctions can also contains functions from base classes, especially constructors.

This comment has been minimized.

@axic

axic Aug 4, 2017

Member

annotation().allBaseConstructorsImplemented, but also include it in the unimplementedFunctions list? ?

@axic

axic Aug 4, 2017

Member

annotation().allBaseConstructorsImplemented, but also include it in the unimplementedFunctions list? ?

This comment has been minimized.

@axic

axic Aug 4, 2017

Member

Actually where to use that boolean?

@axic

axic Aug 4, 2017

Member

Actually where to use that boolean?

This comment has been minimized.

@chriseth

chriseth Aug 4, 2017

Contributor

Export it via the json-ast-export.

@chriseth

chriseth Aug 4, 2017

Contributor

Export it via the json-ast-export.

This comment has been minimized.

@axic

axic Aug 4, 2017

Member

I can do, but what it is useful for?

@axic

axic Aug 4, 2017

Member

I can do, but what it is useful for?

This comment has been minimized.

@axic

axic Aug 4, 2017

Member

Discussed offline that it refers to a "feature" which makes no sense: unimplemented, but defined constructors make the contract and derived contracts non-compileable.

@axic

axic Aug 4, 2017

Member

Discussed offline that it refers to a "feature" which makes no sense: unimplemented, but defined constructors make the contract and derived contracts non-compileable.

@chriseth

Fine to merge as long as the documentation of the annotation is a little expanded.

@axic axic merged commit a372941 into develop Aug 4, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@axic axic deleted the show-unimplemented-funcs branch Aug 4, 2017

@axic axic removed in progress labels Aug 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment