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.

Sign up for GitHub

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

Jump to bottom

Introduce pure specifier on functions #2745

Merged
merged 6 commits into from
Aug 24, 2017
Merged

Introduce pure specifier on functions #2745

merged 6 commits into from
Aug 24, 2017

Conversation

axic
Copy link
Member

@axic axic commented Aug 15, 2017 •
edited
Loading

Part of #992. Depends on #2754 and #2762.

axic
axic commented Aug 15, 2017
View reviewed changes
libsolidity/analysis/StaticAnalyzer.cpp
@@ -57,6 +57,8 @@ bool StaticAnalyzer::visit(FunctionDefinition const& _function)
solAssert(m_localVarUseCount.empty(), "");
m_nonPayablePublic = _function.isPublic() && !_function.isPayable();
m_constructor = _function.isConstructor();
if (_function.stateMutability() == StateMutability::Pure)
m_errorReporter.warning(_function.location(), "Function is marked pure. Be careful, pureness is not enforced yet.");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the safe way to introduce it unless the call graph analyser is finished.

@axic axic force-pushed the statemutability-pure branch 4 times, most recently from c986134 to 5ae7d0e Compare August 16, 2017 19:54
@axic
Copy link
Member Author

axic commented Aug 16, 2017

Need to add tests for pure functions and function types.

@axic axic mentioned this pull request Aug 16, 2017
Closed
13 tasks
VoR0220
VoR0220 reviewed Aug 17, 2017
View reviewed changes
Changelog.md
@@ -1,7 +1,8 @@
### 0.4.16 (unreleased)

Features:
* ABI JSON: Include new field ``statemutability`` with values ``view``, ``nonpayable`` and ``payable``.
* Introduce ``pure`` functions. The pureness is not enforced yet, use with care.
* ABI JSON: Include new field ``statemutability`` with values ``pure``, ``view``, ``nonpayable`` and ``payable``.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any chance of making statemutability: state_mutability? Pure aesthetics I know. But might make it easier to read. Atleast a mixedCase or - of some kind.

Copy link
Member Author

@axic axic Aug 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no prior example in the ABI so not sure which way to go. (However this was merged in a different PR so please open an issue/PR to solve it.)

@axic axic force-pushed the statemutability-pure branch from 5ae7d0e to 6959fe5 Compare August 21, 2017 14:53
axic
axic commented Aug 21, 2017
View reviewed changes
libsolidity/ast/ASTJsonConverter.cpp Outdated
make_pair(
m_legacy ? "constant" : "isDeclaredConst",
_node.stateMutability() == StateMutability::View ||
_node.stateMutability() == StateMutability::Pure
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I don' think this should contain pure as it is only for the AST. By using this AST one couldn't reliable rebuild the source.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand. constant / isDeclaredConst is used by static analyzers and I would like to break them rather later than sooner. We have an additional field that provides the state mutability, so all the information is there.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I still don't agree it belongs to isDeclaredConstant, because that refers to the constant specifier, I can understand the reasoning for static analyzers not yet aware of the statemutability field. Ultimately, removing the obsolete field will solve this.

axic
axic commented Aug 21, 2017
View reviewed changes
libsolidity/ast/ASTPrinter.cpp Outdated
(_node.isPublic() ? " - public" : "") +
(
_node.stateMutability() == StateMutability::View ||
_node.stateMutability() == StateMutability::Pure ?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same applies here as for the AST JSON.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the AST printer should at least contain the information about state mutability.

@axic axic force-pushed the statemutability-pure branch 3 times, most recently from 4ba6558 to 5cf2997 Compare August 22, 2017 18:20
@axic axic changed the title [WIP] Introduce pure specifier on functions Introduce pure specifier on functions Aug 22, 2017
chriseth
chriseth reviewed Aug 24, 2017
View reviewed changes
docs/contracts.rst
Pure Functions
**************

Functions can be declared ``pure`` in which case they promise not to read from or modify the state.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should explain in more detail what reading from or writing to the state means.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also postpone this explanation so that we do not delay the release.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move it to #2792? Do you want to list all the disallowed things?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can move it to #2792. We should list as much as people need to understand.

@axic axic force-pushed the statemutability-pure branch from 5cf2997 to 6c0f21e Compare August 24, 2017 09:34
@axic axic mentioned this pull request Aug 24, 2017
Merged
@chriseth
Copy link
Contributor

I think there are some places where == StateMutability::View has to be changed to <= StateMutability::View, for example ASTJsonConverter and ABI.cpp.

Also ASTPrinter sholud be updated for completeness.

@axic
Copy link
Member Author

axic commented Aug 24, 2017 •
edited
Loading

I've thought I've changed the ABI (seems like it was gone through some wrong conflict resolution in rebasing), but explained above why I don't think it applies to ASTJsonConverter and ASTPrinter in #2745 (comment).

@axic axic force-pushed the statemutability-pure branch from 6c0f21e to 75d35ab Compare August 24, 2017 09:51
@axic axic force-pushed the statemutability-pure branch from 75d35ab to f646247 Compare August 24, 2017 13:14
@axic
Copy link
Member Author

axic commented Aug 24, 2017

@chriseth should be ready

@axic axic merged commit d3fd6a8 into develop Aug 24, 2017
@axic axic deleted the statemutability-pure branch August 24, 2017 13:53
@HaleTom HaleTom mentioned this pull request Oct 1, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chriseth chriseth chriseth approved these changes

@VoR0220 VoR0220 VoR0220 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@axic @chriseth @VoR0220