[WIP] Replace the constant keyword with pure and view for functions by axic · Pull Request #1049 · argotorg/solidity

axic · 2016-09-08T01:38:43Z

Further things to be sorted: selfdestruct, log (events), this.balance, address.balance, tx.*, new

Related: #992.

axic · 2016-09-08T01:40:34Z

I was wondering how much effort is it to change constant to view / pure. This PR can be closed if we think it is too early (as the design is not finalised yet).

chriseth · 2016-09-09T15:08:33Z

We should wait a bit until we introduce the next breaking changes. We can keep constant as an alias for view for now.

axic · 2016-10-05T12:30:40Z

 		Json::Value method;
 		method["type"] = "function";
 		method["name"] = it.second->declaration().name();
-		method["constant"] = it.second->isConstant();


Will leave this in as a deprecated field which we can remove in the breaking release. (So it will have both constant and view set)

axic · 2016-10-05T12:31:12Z

 		Token::Value token = m_scanner->currentToken();
-		if (token == Token::Const)
+		// FIXME: constant should be removed
+		if (token == Token::Const || token == Token::View)


This already leaves the constant keyword as a backwards compatibility change.

axic · 2016-10-05T12:32:21Z

@chriseth actually the code was like that, supports the constant keyword as an alias for view. I'll add back the JSON ABI field too.

Internally though I think it makes sense just moving to view / pure.

axic · 2016-10-05T12:36:52Z

 	addSourceFromDocStrings(m_currentContract.contract->annotation());

-	if (_function.isDeclaredConst())
+	if (_function.isPure() || _function.isView())


@pirapira shouldn't this be only isPure?

No. If it isView(), the function should not change the state, so it's reasonable to assert that the new state is equal to the original state.

chriseth · 2016-10-10T19:07:59Z

Please reduce the number of items to be done for this issue - they are a hell of a lot of work. There already is an issue about "enforcing the constant keyword", so please move some of this stuff there, potentially create new issues and interlink them.

chriseth · 2016-10-10T19:09:28Z

-		bool _isDeclaredConst,
 		std::vector<ASTPointer<ModifierInvocation>> const& _modifiers,
 		ASTPointer<ParameterList> const& _returnParameters,
+		bool _isView,


If they all contradict each other, perhaps we should create an enum?

Only view & pure does. (And payable.)

I feel it would hinder future development, because no one would like to get rid of such an enum (pure, view, regular, payable) if it is added.

If view, pure and payable are all mutually exclusive, the danger of having an inconsistent function type at any point is far worse than potentially hindering development.

Furthermore, we should always actively strive against any cargo cult thinking, so that these kinds of arguments are non-issues.

The name of the enum should match the name we use in the ABI below.

chriseth · 2016-10-10T19:11:20Z

 	{
 		"name": "f",
 		"constant": false,
+		"view": false,


This also looks like having an enum at the API level would make sense.

Enum of constant, view, pure?

Constant == view, it is just left in the ABI JSON for backwards compatibility.

What about something like "promise": "view" or "constant": "view"?

If we want a single field, we should come up with a better name. I think constant is definitely a no go and promise might be ok.

We have to consider the three states we have: regular (might find a better name), view, pure. They differ in their state changing ability.

mutability, mutates, mutate?

vp-qualifiers (no, I just looked up how C does it)

side-effects, purity, information-flow?

From yesterday's call:

We have the following types (from most restricted / smallest subset of instructions): pure, view, non-payable, regular/payable.

It can be represented as state-access with levels of none, read-only, non-payable, full.

It may be beneficial to make separation of state and memory access. Think about internal pure functions which work on a memory reference - can they change data?

Memory-avoiding: contained, isolated, confined

I'm starting to think that we may be better with having a syntax similar to modifiers for these: memory(changes), memory(intact), or may be alone memory for those which change the memory after finishing.

Just like C inline assembly has its clobber list.

Did we consider a variant where state-changes are an attribute of the function but memory-changes are attributes of the function arguments? I guess creating new objects in newly allocated memory should be fine for functions that do not alter memory, because I think altering memory should essentially not be about memory as a whole, but only about pre-existing objects in memory.
So we can just apply a constant keyword to function arguments of reference type (it does not make sense for return values) and for complex variables, this constant keyword is pulled all the way through.
If we do it like that, is there a way to get more fine-grained access in the future, i.e. you can pass an array where it is fine for the function to add new elements but it is not allowed to alter existing elements? We could add a 'mutable' keyword that negates the effect of 'constant' starting from that level.

First iteration allows:

function f(uint[][] constant x) { ... }

Second iteration allows:

function f(uint[]constant[] x) { ... }

I'd prefer the keyword immutable instead of constant.

We've have clarified in an offline meeting that view and pure refer to state and make sense in the case of external calls. They do not care about altering memory, because for external calls an individual instance is spinned up and there's no way to access/alter memory after it.

Therefore, we declare that parameters, if pointing to memory, are always mutable and the above is introducing a keyword to mark them immutable.

The only case not handled by this is deliberate memory changes via inline assembly.

axic · 2016-10-10T20:13:02Z

@chriseth:

Please reduce the number of items to be done for this issue - they are a hell of a lot of work.

I will, the plan for this PR here is only be the non-breaking change.

Re: enforcing, please review this: #992 (comment)

axic · 2017-06-13T15:08:27Z

 		make_pair("payable", _node.isPayable()),
 		make_pair("visibility", visibility(_node.visibility())),
+		// FIXME: remove with next breaking release
 		make_pair(m_legacy ? "constant" : "isDeclaredConst", _node.isDeclaredConst()),


Need to change to isView().

axic · 2017-06-13T15:12:05Z

I think once tests are fixed, this could be merged as is (it just introduces the keywords).

We could change it to an enum as agreed, though we never could settle on a naming:

Mutability: pure, view, regular/nonpayable, payable.
StateAccess: none, read-only, non-payable, full/payable.

axic · 2017-07-01T00:00:24Z

+		// FIXME: constant should be removed at the next breaking release
+		method["constant"] = it.second->isView();
+		method["view"] = it.second->isView();
+		method["pure"] = it.second->isPure();


Once we decide on a naming, the ABI should be:

constant (legacy field, to be removed in the future)

payable (legacy field, to be removed in the future)

mutability with values pure, view, write, payable (this is the part to be decided)

axic · 2017-08-09T14:37:50Z

Split out first step as #2722.

axic · 2017-08-16T20:42:51Z

Closing this in favour of the new smaller PRs: #2745 and #2762.

This change makes information in ENR compatible with the existing discovery protocol: - The default identity scheme is now called "v4" to make the name shorter. - Node addresses are derived as the hash of the uncompressed public key. - The "ip4" and "ip6" keys are merged into a single "ip" key which can hold a IPv4 or IPv6 address. - The "discv5" key is now called "udp". - The new "tcp" key holds the TCP port.

LianaHus added the in progress label Sep 8, 2016

axic commented Oct 5, 2016

View reviewed changes

axic force-pushed the view-pure branch 3 times, most recently from 71b7e03 to 036b292 Compare October 8, 2016 01:36

chriseth changed the title ~~Replace the constant keyword with pure and view for functions [WIP]~~ Breaing: Replace the constant keyword with pure and view for functions [WIP] Oct 10, 2016

chriseth changed the title ~~Breaing: Replace the constant keyword with pure and view for functions [WIP]~~ Breaking: Replace the constant keyword with pure and view for functions [WIP] Oct 10, 2016

chriseth suggested changes Oct 10, 2016

View reviewed changes

chriseth changed the title ~~Breaking: Replace the constant keyword with pure and view for functions [WIP]~~ Replace the constant keyword with pure and view for functions [WIP] Oct 10, 2016

axic force-pushed the view-pure branch from 036b292 to f8bbe4f Compare October 12, 2016 19:55

axic force-pushed the view-pure branch from f8bbe4f to 47b7871 Compare October 19, 2016 23:06

chriseth added soon and removed in progress labels Oct 28, 2016

axic force-pushed the view-pure branch 2 times, most recently from 5156e84 to f0f6b59 Compare November 15, 2016 02:20

axic force-pushed the view-pure branch 5 times, most recently from 9fdbd99 to 188a0f2 Compare November 21, 2016 11:29

axic force-pushed the view-pure branch from b76cba9 to e0351ee Compare November 28, 2016 14:46

pirapira assigned axic Dec 6, 2016

axic changed the title ~~Replace the constant keyword with pure and view for functions [WIP]~~ [WIP] Replace the constant keyword with pure and view for functions Feb 6, 2017

axic force-pushed the view-pure branch 2 times, most recently from 5d956a5 to ce5f252 Compare March 22, 2017 11:13

axic mentioned this pull request Apr 26, 2017

mutable variables (immutable as default) #715

Closed

5 tasks

axic force-pushed the view-pure branch 2 times, most recently from 47ea26f to e791805 Compare May 5, 2017 22:08

axic force-pushed the view-pure branch from e791805 to 3d477e0 Compare May 19, 2017 19:49

axic force-pushed the view-pure branch from 3d477e0 to ce81b01 Compare May 30, 2017 11:02

chriseth added the Focus label May 31, 2017

axic commented Jun 13, 2017

View reviewed changes

axic force-pushed the view-pure branch 2 times, most recently from 711d672 to dba969a Compare June 30, 2017 23:55

axic commented Jul 1, 2017

View reviewed changes

axic force-pushed the view-pure branch 3 times, most recently from 5e050a2 to 7de262d Compare July 1, 2017 00:21

chriseth added the requires discussion label Jul 12, 2017

axic force-pushed the view-pure branch from 7de262d to a540740 Compare July 19, 2017 01:44

axic force-pushed the view-pure branch from a540740 to cfe3122 Compare July 27, 2017 11:46

axic force-pushed the view-pure branch from cfe3122 to 642a2d6 Compare August 8, 2017 22:21

axic added 3 commits August 15, 2017 01:49

Replace the constant keyword with pure and view for functions

22dbf48

Update grammar with view/pure

a05fd68

Change tests to use view/pure

744eca3

axic force-pushed the view-pure branch from 642a2d6 to 744eca3 Compare August 15, 2017 00:50

axic closed this Aug 16, 2017

axic deleted the view-pure branch August 16, 2017 21:08

axic pushed a commit to ipsilon/solidity that referenced this pull request Apr 22, 2025

♻️ Improve Brutalizer (argotorg#1049)

2e34c71

Conversation

axic commented Sep 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

axic commented Sep 8, 2016

Uh oh!

chriseth commented Sep 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic commented Oct 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriseth commented Oct 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic Nov 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic Apr 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic commented Oct 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic commented Jun 13, 2017

Uh oh!

axic Jul 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic commented Aug 9, 2017

Uh oh!

axic commented Aug 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

axic commented Sep 8, 2016 •

edited

Loading

axic Nov 18, 2016 •

edited

Loading

axic Apr 26, 2017 •

edited

Loading

axic Jul 1, 2017 •

edited

Loading