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

Explicit keyword for abstract contracts #649

Closed
Antanukas opened this issue Jun 14, 2016 · 29 comments

Comments

@Antanukas
Copy link

@Antanukas Antanukas commented Jun 14, 2016

Compiler doesn't produce errors, but returns empty bytecode when child contract does not call inherited constructor.

To reproduce:

contract parent {
  function parent(address _someValue) {}
}

contract child is parent {
//  Uncomment to fixes compilation
//  function child(address a) parent(a) {}
}

run sold child.sol --bin
output:


======= child =======
Binary: 


======= parent =======
Binary: 
60606040526040516020806038833981016040528080519060200190919050505b5b50600a80602e6000396000f360606040526008565b00

solc version:

solc, the solidity compiler commandline interface
Version: 0.3.4-0/RelWithDebInfo-Linux/g++/Interpreter

Expected: some error message that child must define constructor

@Antanukas Antanukas changed the title Empty binary output when child constructor doesn't call parent Empty bytecode output when child constructor doesn't call parent Jun 14, 2016
@chriseth

This comment has been minimized.

Copy link
Contributor

@chriseth chriseth commented Jun 14, 2016

If you do not proved base contract constructor arguments, your contract is abstract, as if you did not implement one of the methods. There is nothing wrong with that, so it is not possible to show any error.

Having said that, we plan to change the way the compiler is invoked so you can explicitly specify which contract you want to compile (instead of all non-abstract ones). In that case, the compiler can provide an error message and should also tell you the reason why a contract is abstract.

@Antanukas

This comment has been minimized.

Copy link
Author

@Antanukas Antanukas commented Jun 15, 2016

So maybe a simple keyword 'abstract' could be added like in Java, Scala, C#? This way compiler will have enough information to warn user if his intention was not an abstract contract. Cause I spent some time until I have realized this mistake.

In my concrete case I had a big contract which was inheriting from a contract with constructor and I have missed the constructor in derived contract. This resulted in empty bytecode from compiler which seems incorrect.

@chriseth chriseth changed the title Empty bytecode output when child constructor doesn't call parent Explicit keyword for abstract contracts Jun 17, 2016
@axic

This comment has been minimized.

Copy link
Member

@axic axic commented Jul 28, 2016

So right now the only way to test if it is an abstract contract is by seeing if a bytecode is returned during compilation. For real abstract contracts, that is empty.

Fun fact, the below is not abstract:

contract A {
}

because it contains only the implicit definition of the fallback function.

@axic

This comment has been minimized.

Copy link
Member

@axic axic commented Aug 5, 2016

I think having an explicit keyword could aid error reporting. Should it be called abstract contract or interface though?

@redsquirrel

This comment has been minimized.

Copy link
Contributor

@redsquirrel redsquirrel commented Aug 5, 2016

I think it would be A Very Good Thing to have an explicit abstract keyword to denote abstract contracts. It would improve the readability of Solidity code even more than the constant keyword.

@axic I wouldn't use interface because (at least in Java) interfaces can't implement any methods (all methods are implicitly abstract), whereas Solidity's abstract contracts can implement methods, leaving at least one method abstract. (Very similar to Java's abstract classes.)

@axic

This comment has been minimized.

Copy link
Member

@axic axic commented Aug 5, 2016

@redsquirrel forgot that it can have implemented as well as non-implemented functions.

I'm not sure abstract contracts make sense in that case, perhaps marking such functions clearly abstract makes more sense?

contract Weird {
  function a() abstract;
  function b() {
    if (msg.value > 0)
       a();
  }
}

I would personally prefer to have a clean interface definition in any case, so that we can have a clean, interface-only Token (and Mango Repository) object to be used for linking. Basically the Solidity source code version of the JSON ABI.

interface WeirdInterface {
  function a();
  function b();
}
@redsquirrel

This comment has been minimized.

Copy link
Contributor

@redsquirrel redsquirrel commented Aug 5, 2016

@axic In Java, you can mark methods and classes as abstract. For solidity, I recommend requiring an explicit abstract keyword for abstract (un-instantiable) contracts, and letting abstract functions be implicitly abstract.

Seems like the interface idea deserves it's own issue/discussion.

@chriseth

This comment has been minimized.

Copy link
Contributor

@chriseth chriseth commented Aug 11, 2016

As also commented on the "interface" issue:

I think having another type of contract is not a good idea and will confuse users. What about:

Warn if contract is abstract but not "marked" as abstract.
You can mark a contract as abstract by using contract a is abstract {}.
It is a bit weird, because abstract would be a keyword that is not inherited, i.e. contracts that derive from a will not necessarily be abstract.

Also having the distinction between explicitly abstract contract and purely abstract (interface) is too fine and confusing. Please let's not go down the path C# went.

@chriseth chriseth added this to the 3-abstract- milestone Aug 11, 2016
@redsquirrel

This comment has been minimized.

Copy link
Contributor

@redsquirrel redsquirrel commented Aug 11, 2016

Warn if contract is abstract but not "marked" as abstract.

I'd prefer to see it more strictly enforced with an error, but a warning is fine.

You can mark a contract as abstract by using contract a is abstract {}.

I think this is going to confuse people because it looks like inheritance. I'd prefer:

abstract contract a {}

@chriseth

This comment has been minimized.

Copy link
Contributor

@chriseth chriseth commented Aug 11, 2016

Yep, that is how it is done in Typescript, so we could also follow that.

@axic

This comment has been minimized.

Copy link
Member

@axic axic commented Aug 15, 2016

You can mark a contract as abstract by using contract a is abstract {}.

I think this is a bad idea. It suggests that it is derived from an abstract parent, but that wouldn't mean your contract cannot be instantiated.

Also having the distinction between explicitly abstract contract and purely abstract (interface) is too fine and confusing. Please let's not go down the path C# went.

Abstract contracts are probably useful to map out source code in a more sensible way.

Interfaces on the other hand map the ABI 1-to-1 and have a different use case. It helps to avoid the mistake of using an abstract contract, which has a few extra methods defined, and pointing it to an instance which only has the interface methods available.

Take this example:

contract MyInterface {
    function a() returns (bytes32);
}

contract Abstract is MyInterface {
    function a() returns (bytes32);
    function b() returns (bytes32) {
        return a();
    }
}

contract Real is MyInterface {
    function a() returns (bytes32) {
        return 0x4242;
    }
}

contract Test {
    function a() returns (bytes32) {
        return Abstract(<the address of Real>).b(); // this compiles, but will fail
        return MyInterface(<the address of Real>).b(); // this will raise the compilation error
    }
}

MyInterface is the interface (i.e. Token) and Real is an actual contract implementing that interface (i.e. a real token).

@redsquirrel

This comment has been minimized.

Copy link
Contributor

@redsquirrel redsquirrel commented Aug 15, 2016

return Abstract(<the address of Real>).b(); // this compiles, but will fail

Why would this fail?

@axic

This comment has been minimized.

Copy link
Member

@axic axic commented Aug 15, 2016

Because the instance of Real doesn't implement the method b. Real is derivate of the interface and not the abstract, but when interfaces are defined as abstracts (as today), this can easily be uncaught.

@redsquirrel

This comment has been minimized.

Copy link
Contributor

@redsquirrel redsquirrel commented Aug 15, 2016

Ahhh, I see it now. I thought that Real inherited from Abstract. Now I see what you were doing. Thanks.

@asinyagin

This comment has been minimized.

Copy link
Contributor

@asinyagin asinyagin commented Aug 25, 2016

I like both abstract contract and interface but the second one is shorter.

@chriseth

This comment has been minimized.

Copy link
Contributor

@chriseth chriseth commented Mar 7, 2018

I think we should discuss this again. Currently, there are many situations that lead to a contract being abstract, but you get not notified about that unless you try to instantiate such a contract. If we require a keyword of some sort for such cases, it is much easier to detect. (suggested by @Recmo)

@chriseth chriseth added this to Planned in Backlog (non-breaking) via automation Mar 7, 2018
@axic

This comment has been minimized.

Copy link
Member

@axic axic commented May 8, 2018

Since we have interface for interfaces, the only choice here remains (based on the above): abstract contract.

I would suggest an alternative: partial contract.

I'm still a proponent of making this required.

@chriseth

This comment has been minimized.

Copy link
Contributor

@chriseth chriseth commented Jun 14, 2018

I think I would prefer contract abstract Name { although I also don't particularly like it.

@ekpyron

This comment has been minimized.

Copy link
Contributor

@ekpyron ekpyron commented Jun 14, 2018

What about contract Name abstract {? That's closer to a modifier. Since we have is for inheritance, contract Name abstract is Base { is still unambiguous. Or one also could consider contract Name is Base abstract {, although that looks stranger to me.
For some reason in post notation I think contract Name partial { looks better than partial contract does, so in that ordering I'd actually be fine with that as well.

@fulldecent

This comment has been minimized.

Copy link
Contributor

@fulldecent fulldecent commented Sep 21, 2018

I consider abstract contract as a type.

Example from https://github.com/0xcert/ethereum-utils/blob/master/contracts/ownership/Ownable.sol

pragma solidity ^0.5.0;

abstract contract Ownable {
  address public owner;

  // event OwnershipTransferred...

  constructor() public {
    owner = msg.sender;
  }

  modifier onlyOwner() {
    require(msg.sender == owner);
    _;
  }

  function transferOwnership(address _newOwner) onlyOwner public {
    // ...
  }
}

Nobody in their right mind would deploy the Ownable contract, even though it is presently possible. The keyword abstract removes the ability to compile this contract. And in the Remix IDE, abstract contracts won't even show up as compile targets.

In addition to allowing abstract keyword to compileable contracts. It is probably helpful to require abstract for non-compilable contracts.

pragma solidity ^0.4.25;

contract A {
    function x();
}

^^^ Presently this code is legal. I propose that it should be an error "Contracts with unimplemented methods must be explicitly marked as abstract." Or better, "Contract A must implement function x() or be marked as abstract". And of course, interfaces are implicitly abstract.

axic pushed a commit that referenced this issue Nov 20, 2018
axic pushed a commit that referenced this issue Nov 20, 2018
axic pushed a commit that referenced this issue Nov 20, 2018
axic pushed a commit that referenced this issue Nov 20, 2018
axic pushed a commit that referenced this issue Nov 20, 2018
axic pushed a commit that referenced this issue Nov 20, 2018
axic pushed a commit that referenced this issue Nov 20, 2018
@chriseth chriseth added this to Inbox in Backlog (breaking) via automation Dec 13, 2018
@chriseth chriseth added this to To do in 0.6.0 via automation Dec 13, 2018
@chriseth chriseth removed this from Inbox in Backlog (non-breaking) Dec 13, 2018
@chriseth chriseth moved this from Inbox to high priority in Backlog (breaking) Dec 13, 2018
@chriseth chriseth moved this from To do to Ready to be Worked On in 0.6.0 Feb 19, 2019
@chriseth

This comment has been minimized.

Copy link
Contributor

@chriseth chriseth commented Feb 19, 2019

Quick decision by show of hands during meeting: In favour for abstract contract instead of contract abstract.

@chriseth

This comment has been minimized.

Copy link
Contributor

@chriseth chriseth commented Feb 19, 2019

To clarify the implementation proposal again:

  • cannot be applied to interfaces (for now)
  • cannot be applied to libraries (for now)
  • abstract is a setting for the contract
  • if !isFullyImplemented() (as implemented now) has a different value than abstract(), issue a warning.
@chriseth

This comment has been minimized.

Copy link
Contributor

@chriseth chriseth commented Feb 19, 2019

Idea by @axic: An empty contract should be considered not implemented (and thus require abstract).

@nventuro

This comment has been minimized.

Copy link
Contributor

@nventuro nventuro commented Aug 14, 2019

Nobody in their right mind would deploy the Ownable contract, even though it is presently possible. The keyword abstract removes the ability to compile this contract. And in the Remix IDE, abstract contracts won't even show up as compile targets.

That can also be achieved by making the constructor internal:

contract Ownable {
  constructor() internal {
    _owner = msg.sender();
  }

 ...
}

This is IMO a cleaner approach, since it directly tackles the actual objective: preventing the contract from being deployed.

I'd have the abstract keyword cause a compiler error when applied to a contract that has no pure virtual functions. That way, it will always have the same meaning: the contract contains function declarations with no definition.

@aarlt aarlt mentioned this issue Sep 4, 2019
6 of 6 tasks complete
@leonardoalt leonardoalt moved this from ToDo to In progress in 0.6.0 Sep 10, 2019
@chriseth

This comment has been minimized.

Copy link
Contributor

@chriseth chriseth commented Nov 4, 2019

Implemented in #7358

@chriseth chriseth closed this Nov 4, 2019
0.6.0 automation moved this from In progress to Done Nov 4, 2019
@leonardoalt leonardoalt moved this from To do to Done in Consolidate inheritance rules Nov 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
0.6.0
  
Done
9 participants
You can’t perform that action at this time.