Change default function visibility to internal (or private?)

[axic]

The benefit is that making something externally accessible is a concious decision by the programmer (need to supply public or external modifiers).

Note: fallback functions (#1048) and functions in interfaces (#2330) will be forced to be external.

[rmerom]

+1, this likely allowed the Parity multisig wallet bug not to be noticed:
https://twitter.com/maraoz/status/887751004971831296

[flux627]

It saddens me that this was only considered now

[ethers]

Maybe worth considering going a step further, and requiring that visibilities are always specified (no defaults)?
https://github.com/ConsenSys/smart-contract-best-practices#explicitly-mark-visibility-in-functions-and-state-variables

[vrde]

Maybe worth considering going a step further, and requiring that visibilities are always specified (no defaults)?

IMO this is the best option. The programming language should force the developer in making a decision, and throw a syntax error if no visibility is specified.

[MrTibbles]

Hindsight is a funny thing.

[AusIV]

+1 for no default values. Having visibility be explicitly defined every time means that the developer has to expressly make the decision, and anyone reading the code (even if they're relatively new to the language) knows the visibility.

[ethernian]

A "no defaults" paradigm leads to excessive and confusing code.
A good programming language should set reasonable defaults. Reasonable for solidity should mean "defaults, introducing no hidden risk".

+1 for "private" per default because it adds no further risks and force a developer to make an explicit decision only if necessary.

[flux627]

Excessive could be debatable, but never have I experienced explicit code being more confusing than implicit code. It's usually the other way around. Isn't that the whole reason this issue is open?

[AusIV]

Another argument for no defaults is that switching the default at this point in the game could lead to unexpected behaviors. Going to no default means anyone who is relying on the current default behavior will get a syntax error when they upgrade compilers, and will be forced to fix it before they can deploy their contract. Going to private by default means people could have functions they expected to be public suddenly become private when they upgrade their compiler. Hopefully that would be caught by tests, but I can imagine how it could lead to some nasty surprises.

[veox]

Concur with "no defaults" "ERROR is the new default", mainly for reason outlined by @AusIV.

Changing the default is one thing. Checking all code ported to the newer compiler version is another thing. It will be done by developers who will likely not have seen this issue.

They may also be coming to Solidity much later, after the issue has been settled, and trying to reuse "old" code (that assumed different defaults) without knowing it's "old".

---

EDIT: The safe way to port for "default changed" is to specify visibility for all functions in given code, and to remove the specifier for whatever is chosen as the new default, but... Why do that? It's just been brought to most-entropy-possible.

---

OT: This is also high time to adopt semantic versioning, and bump the MAJOR to 1 (as in v1.0.0).

[wyaeld]

While there is valid arguments to be made for implicit vs explicit for programming languages in general, I think that Visibility is such a critical concern for functions on a blockchain, that forcing an explicit declaration on the function isn't too much overhead.

+1 for no default and compiler error.

People can fix existing contracts they care about if given clear notice.

[axic]

Going to private by default means people could have functions they expected to be public suddenly become private when they upgrade their compiler. Hopefully that would be caught by tests, but I can imagine how it could lead to some nasty surprises.

The compiler could also output a warning/error. If an old version pragma (or no pragma) is in the code it can assume the old rules were intended and issue the warning.

OT: This is also high time to adopt semantic versioning, and bump the MAJOR to 1 (as in v1.0.0).

Solidity already uses semantic versioning. What benefit would be changing major to 1?

[veox]

Solidity already uses semantic versioning.

Ah, OK.

What benefit would be changing major to 1?

The MAJOR is intended to indicate that something major has changed. I was assuming that if an error was to be emitted (as described above), then a major version bump like that would make it expected (when porting "old" code).

It's definitely not required if there'd be a deprecation warning first.

[axic]

This is slightly off topic, but semantic versioning is fully adhered to in Solidity (http://solidity.readthedocs.io/en/develop/installing-solidity.html#important-information-about-versioning). For major 0, semantic versioning dictates that bumps in minor indicate breaking changes. Once major is >0, a bump of major indicates breaking changes.

[axic]

Also since the version pragma checker supports all the rules of npm's semantic versioning it would make sense to suggest using the ~ modifier, e.g.:

pragma solidity ~0.4.13;

This means that compiler versions 0.4.13 <= n < 0.5.0 are required.

Any visibility modifier change we discuss here would very likely end up in the 0.5.0 (breaking) release and thus would render code using the above pragma not to compile until the programmer consciously updates it.

[5chdn]

Someone just pointed out that the docs are already stating internal is default.

By default, function types are internal, so the internal keyword can be omitted.

https://solidity.readthedocs.io/en/develop/types.html#function-types

[axic]

@5chdn let's keep the documentation discussion in the other issue.

[phra]

definitely needed. this language should focus first on security by design.

[chriseth]

I'm also slightly leaning towards the "no defaults" camp, but what about the following compromise (syntax could still be improved):

contract C {
  public {
    function f() { ... }
    function g() { ... }
  }
  internal {
     function h() { ... }
     function r() { ... }
  }
}

Of course the compiler will warn / generate an error if a function is declared without an explicit visibility specifier outside of such a "visibility area".

This can also be combined with modifier areas to something like this:

contract C {
  public {
    address owner;

    function f() { ... }
    apply onlyOwner {
      function changeOwner(address _new) { ... }
      function destroy() { ... }
    }
  }

  internal {
     function h() { ... }
     function r() { ... }
  }
  modifier onlyOwner { require(msg.sender == owner); _; }
}

[veox]

Personally, I'd much prefer leaving the visibility on the same line as the fuction declaration. For the pragmatic reason of PRs changing function visibility (e.g.public->external) looking like one-liners, with (probably) just those keywords highlighted, rather than the function's body jumping around from one section to another.

[ethernian]

... rather than the function's body jumping around from one section to another.
agree. I woud also prefer to leave visibility keywords in the function declaration.

another reason: sometimes it is reasonable to group functions with different visibility together.
For example there could be two similar versions of the same function: one public and one internal.

mapping (uint => Foo) foos;

//would like to grope these two functions together: 
function isValid(uint fooId) public returns(bool) { return isValid(foos[fooId]); } 
function isValid(Foo foo) internal returns(bool) { ... } 

[rmerom]

While the modifier areas may be more pleasing aesthetically, I think having the qualifier right next to the function makes the visibility more explicit and reduces the likelihood of a visibility bug going through a code review.
Locality is a good quality when it comes to security I believe.

[chriseth]

Oh wow, I did not expect such negative reactions :-)

While I see your points, just trying again with another argument:

In my opinion, the problem with the Parity wallet was not that the default visibility was public, the problem was that it was too easy to overlook that such a "powerful" function did not have any access protection.
Modifier areas allow you to group such functions so that it is immediately obvious that something is wrong. As an analogy, consider a power plant, where an important override button is placed in the corridor instead of the control room. Basically, I think it is much safer to restrict a function by its position in the code instead of just a small label.

[ethernian]

@chriseth,
I suppose, the real areas will go over multiple screens. Area header will be out of current vewport.
What will give me a hint about visibility then?

[ethers]

Hinted in the OP, but once visibilities are required, how can we avoid the potential issue of everyone overusing public when (if in most cases) external would be more appropriate? (Do we also need to reconsider non-inheritable contracts #463 ?)

[chriseth]

Just to clarify, because I heard that some people misunderstood this: The idea would be to require a visibility being specified explicitly for each function, but the programmer can choose whether this is done at the function itself or through a modifier area.

[chriseth]

@dip239 this is the same for contracts and functions - if the function or contract is long, their name will be out of the current viewport. All of these problems are solved by keeping such contexts small by re-grouping and modularizing.

[veox]

the programmer can choose whether this is done at the function itself or through a modifier area.

Ah, OK.

@chriseth To clarify - either at the function, or through a modifier area, but not both. Correct?

[ethers]

Expanding on my last comment: I would like to avoid the unnecessary proliferation and misuse of public. public functions need the most scrutiny and if everything is public, people aren't going to pay as much attention to them. public reminds contract writers to consider both the external and internal case (they may fix a customer issue that externally called foo(), but introduce a bug in their contract since they, contract writer, call foo() internally. Or vice-versa, their contract has an issue so they adjust foo() and forget about the external calls).

[federicobond]

While I see the value of these modifier areas, I am not a big fan of them either. With long contract implementations, they force you to reduce the cohesiveness of the code by moving related functions far away from each other.

I know that making a backwards-incompatible change to the default visibility is delicate, but I think Solidity will benefit from it in the long run.

[Ornataweaver]

https://www.bitdegree.org/learn/solidity-visibility-and-getters/