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

Support strict interface contracts #1688

Merged
merged 14 commits into from Mar 21, 2017

Conversation

Projects
None yet
3 participants
@axic
Member

axic commented Feb 13, 2017

Implements #847.

@axic axic added the in progress label Feb 13, 2017

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Feb 14, 2017

Member

For some reason the StructDefinition visitor in TypeChecker is called twice:

interface Token {
^
Spanning multiple lines.
interface2.sol:2:3: Error: Structs cannot be defined in interfaces.
  struct A {
  ^
Spanning multiple lines.
interface2.sol:2:3: Error: Structs cannot be defined in interfaces.
  struct A {
  ^
Spanning multiple lines.
Member

axic commented Feb 14, 2017

For some reason the StructDefinition visitor in TypeChecker is called twice:

interface Token {
^
Spanning multiple lines.
interface2.sol:2:3: Error: Structs cannot be defined in interfaces.
  struct A {
  ^
Spanning multiple lines.
interface2.sol:2:3: Error: Structs cannot be defined in interfaces.
  struct A {
  ^
Spanning multiple lines.
Interfaces are similar to abstract contracts, but they cannot have any functions implemented. There are further restrictions:
#. Cannot inherit other contracts or interfaces.

This comment has been minimized.

@chriseth

chriseth Feb 14, 2017

Contributor

What is the reason behind this? If we allow interfaces to inherit from other interfaces, it would be a convenient way to combine them.

@chriseth

chriseth Feb 14, 2017

Contributor

What is the reason behind this? If we allow interfaces to inherit from other interfaces, it would be a convenient way to combine them.

This comment has been minimized.

@axic

axic Feb 14, 2017

Member

The idea is to keep simple initially and to allow more features as they make sense / are needed.

@axic

axic Feb 14, 2017

Member

The idea is to keep simple initially and to allow more features as they make sense / are needed.

This comment has been minimized.

@VoR0220

VoR0220 Feb 14, 2017

Contributor

Should make note that they are planned to be lifted at a later time.

@VoR0220

VoR0220 Feb 14, 2017

Contributor

Should make note that they are planned to be lifted at a later time.

This comment has been minimized.

@axic

axic Feb 14, 2017

Member

There's such a note there 😉

@axic

axic Feb 14, 2017

Member

There's such a note there 😉

This comment has been minimized.

@VoR0220

VoR0220 Feb 15, 2017

Contributor

I see one for structs but not for inheritance.

@VoR0220

VoR0220 Feb 15, 2017

Contributor

I see one for structs but not for inheritance.

This comment has been minimized.

@axic

axic Mar 15, 2017

Member

Reworded it, please check.

@axic

axic Mar 15, 2017

Member

Reworded it, please check.

Show outdated Hide outdated docs/contracts.rst Outdated
Show outdated Hide outdated docs/contracts.rst Outdated
Show outdated Hide outdated libsolidity/analysis/TypeChecker.cpp Outdated
Show outdated Hide outdated libsolidity/ast/AST.h Outdated
Show outdated Hide outdated libsolidity/parsing/Parser.cpp Outdated
Show outdated Hide outdated libsolidity/parsing/Parser.cpp Outdated
)";
success(text);
}

This comment has been minimized.

@chriseth

chriseth Feb 14, 2017

Contributor

Please add tests for events.

@chriseth

chriseth Feb 14, 2017

Contributor

Please add tests for events.

This comment has been minimized.

@chriseth

chriseth Feb 14, 2017

Contributor

Oh and perhaps also an interface with constructor (with and without implementation).

@chriseth

chriseth Feb 14, 2017

Contributor

Oh and perhaps also an interface with constructor (with and without implementation).

This comment has been minimized.

@axic

axic Mar 15, 2017

Member

There's interface_constructor and interface_events in NameAndTypeResolution.

@axic

axic Mar 15, 2017

Member

There's interface_constructor and interface_events in NameAndTypeResolution.

)";
success(text);
}

This comment has been minimized.

@chriseth

chriseth Feb 14, 2017

Contributor

Can interfaces have private or internal functions?

@chriseth

chriseth Feb 14, 2017

Contributor

Can interfaces have private or internal functions?

This comment has been minimized.

@axic

axic Feb 14, 2017

Member

No.

@axic

axic Feb 14, 2017

Member

No.

This comment has been minimized.

@VoR0220

VoR0220 Feb 14, 2017

Contributor

Makes sense. Interfaces should mandate strict external usage and be the player of "no mans land". Internal is for controlled processes whereas an external is a bit more free form.

@VoR0220

VoR0220 Feb 14, 2017

Contributor

Makes sense. Interfaces should mandate strict external usage and be the player of "no mans land". Internal is for controlled processes whereas an external is a bit more free form.

@chriseth

Please add tests for private functions and clarify that there can be information loss from solidity public interface to ABI.

Show outdated Hide outdated libsolidity/parsing/Parser.cpp Outdated
Show outdated Hide outdated test/libsolidity/SolidityNameAndTypeResolution.cpp Outdated
Show outdated Hide outdated test/libsolidity/SolidityNameAndTypeResolution.cpp Outdated
@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Feb 15, 2017

Member

that there can be information loss from solidity public interface to ABI.

Can you clarify please? You mean that enums and structs cannot be properly conveyed?

Member

axic commented Feb 15, 2017

that there can be information loss from solidity public interface to ABI.

Can you clarify please? You mean that enums and structs cannot be properly conveyed?

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Feb 15, 2017

Member

Need to decide what to do with enums. Since it is not supported in the ABI yet, I'd just reject them now and enable them once the ABI support is added.

Member

axic commented Feb 15, 2017

Need to decide what to do with enums. Since it is not supported in the ABI yet, I'd just reject them now and enable them once the ABI support is added.

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Feb 15, 2017

Contributor

Contract types for example are converted to plain address in the ABI. Basically any named type is deleted in the conversion. Yes, please disallow enums.

Contributor

chriseth commented Feb 15, 2017

Contract types for example are converted to plain address in the ABI. Basically any named type is deleted in the conversion. Yes, please disallow enums.

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Mar 15, 2017

Member

@chriseth is there anything else needed in this PR? (apart from documentation wording)

Member

axic commented Mar 15, 2017

@chriseth is there anything else needed in this PR? (apart from documentation wording)

default:
fatalParserError("Unsupported contract type.");
}
// FIXME: fatalParserError is not considered as throwing here

This comment has been minimized.

@axic

axic Mar 15, 2017

Member

This is needed here because the compiler doesn't detect the throw of fatalParserError. Any suggestion to make this nicer?

__attribute__(noreturn) on fatalParserError should do it.

@axic

axic Mar 15, 2017

Member

This is needed here because the compiler doesn't detect the throw of fatalParserError. Any suggestion to make this nicer?

__attribute__(noreturn) on fatalParserError should do it.

This comment has been minimized.

@chriseth

chriseth Mar 17, 2017

Contributor

We have that in multiple locations. If __attributen__(noreturn) is portable, then let's add that!

@chriseth

chriseth Mar 17, 2017

Contributor

We have that in multiple locations. If __attributen__(noreturn) is portable, then let's add that!

@VoR0220

This comment has been minimized.

Show comment
Hide comment
@VoR0220

VoR0220 Mar 15, 2017

Contributor

@axic question. What is the plan for libraries WRT to this feature?

Suppose I want to create an interface from my contract to a far off library that operates externally. Could I do something like this?

library L {
     function f() {
          //do stuff
     }
}

interface I {
    function f();
}

contract C is I {
     function C() {
         f();
     }
}

Furthermore, could we extend it to maybe include internal functions in the future? I get the strict part for now and I suppose we could have a loose interface down the road.

Contributor

VoR0220 commented Mar 15, 2017

@axic question. What is the plan for libraries WRT to this feature?

Suppose I want to create an interface from my contract to a far off library that operates externally. Could I do something like this?

library L {
     function f() {
          //do stuff
     }
}

interface I {
    function f();
}

contract C is I {
     function C() {
         f();
     }
}

Furthermore, could we extend it to maybe include internal functions in the future? I get the strict part for now and I suppose we could have a loose interface down the road.

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Mar 15, 2017

Member

How is L connected to anything in your example?

Furthermore, could we extend it to maybe include internal functions in the future?

I'm up for extending it later, once we know all the bits and details. Much easier to extend then to restrict.

Member

axic commented Mar 15, 2017

How is L connected to anything in your example?

Furthermore, could we extend it to maybe include internal functions in the future?

I'm up for extending it later, once we know all the bits and details. Much easier to extend then to restrict.

@VoR0220

This comment has been minimized.

Show comment
Hide comment
@VoR0220

VoR0220 Mar 15, 2017

Contributor

@axic what this needs is a couple of endToEnd tests. Can never be too careful.

Contributor

VoR0220 commented Mar 15, 2017

@axic what this needs is a couple of endToEnd tests. Can never be too careful.

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Mar 15, 2017

Member

For some reason the StructDefinition visitor in TypeChecker is called twice:

This is still a problem and results in test failure.

Member

axic commented Mar 15, 2017

For some reason the StructDefinition visitor in TypeChecker is called twice:

This is still a problem and results in test failure.

@VoR0220

This comment has been minimized.

Show comment
Hide comment
@VoR0220

VoR0220 Mar 15, 2017

Contributor

How is L connected to anything in your example?

That's kind of what I'm getting at...would there be a way to "link" based upon the interface? Could this feature be fleshed out further to be the placeholder for all linking?

Perhaps I'm blowing it out of scope for right now, but that's something that could be incredibly useful for us at Monax atm, especially as we push libraries to their limits in Solidity.

Contributor

VoR0220 commented Mar 15, 2017

How is L connected to anything in your example?

That's kind of what I'm getting at...would there be a way to "link" based upon the interface? Could this feature be fleshed out further to be the placeholder for all linking?

Perhaps I'm blowing it out of scope for right now, but that's something that could be incredibly useful for us at Monax atm, especially as we push libraries to their limits in Solidity.

@chriseth

Please add a test where you actually inherit from an interface (the intended use-case).
And specifying a constructor for an interface should be an error - it is not possible to override a constructor, so there is no point in specifying it.

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Mar 17, 2017

Member

@VoR0220 this is how you can use an interface:

interface I {
    function f();
}

contract C {
    function f() {
        I i = I(0x1234);
        i.f();
    }
}
Member

axic commented Mar 17, 2017

@VoR0220 this is how you can use an interface:

interface I {
    function f();
}

contract C {
    function f() {
        I i = I(0x1234);
        i.f();
    }
}
@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Mar 17, 2017

Member

@chriseth also @VoR0220 was concerned that libraries cannot inherit and whether it would make sense to allow them inherit interfaces.

Member

axic commented Mar 17, 2017

@chriseth also @VoR0220 was concerned that libraries cannot inherit and whether it would make sense to allow them inherit interfaces.

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Mar 17, 2017

Member

I guess if we model the interface to be the external interface, it should disallow declaring internal functions.

Member

axic commented Mar 17, 2017

I guess if we model the interface to be the external interface, it should disallow declaring internal functions.

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Mar 20, 2017

Contributor

@axic I don't think we should limit interfaces to the external interface. Disallowing private functions would make sense, though. Oh and note that internal has a different meaning for libraries.

Contributor

chriseth commented Mar 20, 2017

@axic I don't think we should limit interfaces to the external interface. Disallowing private functions would make sense, though. Oh and note that internal has a different meaning for libraries.

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Mar 21, 2017

Member

Oh and note that internal has a different meaning for libraries.

That is a thing I've never quite agreed with :)

I don't think we should limit interfaces to the external interface.

Why would we need an internal method in an interface though? To have an interface for libraries?

Member

axic commented Mar 21, 2017

Oh and note that internal has a different meaning for libraries.

That is a thing I've never quite agreed with :)

I don't think we should limit interfaces to the external interface.

Why would we need an internal method in an interface though? To have an interface for libraries?

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Mar 21, 2017

Contributor

Ok, you are right, interfaces are only concerned with the external interface, because they are only meaningful when you use "pointers" to contracts. Interfaces for libraries do not work except when you only access external functions of those libraries, but even then, interfaces do not yet work like that because the interface keyword itself makes it a contract and not a library.

Contributor

chriseth commented Mar 21, 2017

Ok, you are right, interfaces are only concerned with the external interface, because they are only meaningful when you use "pointers" to contracts. Interfaces for libraries do not work except when you only access external functions of those libraries, but even then, interfaces do not yet work like that because the interface keyword itself makes it a contract and not a library.

@chriseth chriseth self-assigned this Mar 21, 2017

@chriseth chriseth assigned axic and unassigned chriseth Mar 21, 2017

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Mar 21, 2017

Contributor

Test failure should be fixed now.

Contributor

chriseth commented Mar 21, 2017

Test failure should be fixed now.

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Mar 21, 2017

Contributor

Just mining error on windows.

Contributor

chriseth commented Mar 21, 2017

Just mining error on windows.

@chriseth chriseth merged commit 6fb27de into develop Mar 21, 2017

1 of 2 checks passed

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

@chriseth chriseth removed the in progress label Mar 21, 2017

@axic axic deleted the interface-keyword branch Mar 22, 2017

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