[BREAKING] Throw error if function parameters (return) use default data location. #4014

chase1745 · 2018-04-28T22:43:41Z

Only warns if the contract is a library, but can be changed to all contracts if needed.
Throws Type Error for 0.5.0.

chriseth · 2018-05-02T14:06:42Z

libsolidity/analysis/ReferencesResolver.cpp

-					if (_variable.isCallableParameter())
-						typeLoc = DataLocation::Memory;
+					if (_variable.isCallableParameter()) {
+                        typeLoc = DataLocation::Memory;


Please indent using tabs only.
Please put { on a line of its own.

Coding style: https://github.com/ethereum/solidity/blob/develop/CODING_STYLE.md

My apologies on this! Should be fixed now.

chriseth · 2018-05-02T14:13:13Z

test/libsolidity/syntaxTests/functionTypes/warn_function_return_parameters_no_data_location.sol

+    function f() private pure returns(uint[]) {}
+}
+// ----
+// Warning: (50-56): Parameter is declared as memory. Use an explicit "memory" keyword to silence this warning.


Please add a test that shows that this warning can actually be removed by adding memory.

Please also add testst for external and internal functions and functions using storage parameters, both in contracts and in libraries.

I have added these tests for all types of function visibility as well as for contracts and libraries. I also added tests showing the warning is removed by adding a storage location.

chriseth · 2018-05-02T14:17:22Z

libsolidity/analysis/ReferencesResolver.cpp

+                        auto const& contract = dynamic_cast<ContractDefinition const&>(
+                                *dynamic_cast<Declaration const&>(*_variable.scope()).scope()
+                        );
+                        if (contract.isLibrary()) {


Why should this be restricted to libraries only?

Note that for external functions, we have a default data location of calldata which is not possible to express in source code, so we cannot warn about it.

In the original issue you mentioned "We should do the same at least for the case where the contract is a library.", so I only warned for libraries, but I have changed it to warn for contracts as well.

chase1745 · 2018-05-12T02:13:04Z

I believe the only issue left is with events. It is warning for no default data location for event parameters, which isn't correct since they are not allowed.
Is there a way to check if the parameter is in an event? @chriseth

chriseth · 2018-05-14T19:30:18Z

Please add a function isEventParameter in VariableDeclaration similar to isReturnParameter(). It should be enough to dynamic_cast the scope() to EventDeclaration.

chriseth · 2018-05-14T19:31:22Z

libsolidity/analysis/ReferencesResolver.cpp

+					if (varLoc == Location::Default)
+					{
+						// Warn users (error in 0.5.0) for using default data location in (return) parameters.
+						if (_variable.sourceUnit().annotation().experimentalFeatures.count(


Please create a bool const v050 variable that holds this condition at the beginning of the function.

chriseth · 2018-05-14T19:32:20Z

libsolidity/analysis/ReferencesResolver.cpp

 					);
 				if (varLoc == Location::Default || !contract.isLibrary())
+				{
+					if (varLoc == Location::Default)


Does this have to be inside the other condition in line 352?

Yes, because of the or condition, a parameter using memory in a public function would be passed to this if statement, so we need to check if it's using the default location. This is a little confusing, but I didn't want to refactor too much of the existing code.

axic · 2018-05-17T07:20:58Z

libsolidity/analysis/ReferencesResolver.cpp

 		using Location = VariableDeclaration::Location;
 		Location varLoc = _variable.referenceLocation();
 		DataLocation typeLoc = DataLocation::Memory;
+		const bool v050 = bool(


No need for an explicit typecast here. Also usually we write this as bool const v050.

axic · 2018-05-17T07:21:49Z

libsolidity/analysis/ReferencesResolver.cpp

+				{
+					if (varLoc == Location::Default && !_variable.isEventParameter())
+					{
+						// Warn users (error in 0.5.0) for using default data location in (return) parameters.


No need for this comment.

axic · 2018-05-17T07:22:42Z

libsolidity/analysis/ReferencesResolver.cpp

+						else
+							m_errorReporter.warning(
+									_variable.location(),
+									"Parameter is declared as memory. "


Please enclose memory in quotes. Perhaps reword it as Parameter location is assumed as "memory".

axic · 2018-05-17T07:23:05Z

libsolidity/analysis/ReferencesResolver.cpp

+						else
+							m_errorReporter.warning(
+									_variable.location(),
+									"Parameter is declared as memory. "


The same comments apply to this block as to the above block.

axic · 2018-05-17T07:23:32Z

libsolidity/ast/AST.cpp


+bool VariableDeclaration::isEventParameter() const
+{
+	return (bool)dynamic_cast<EventDefinition const*>(scope());


Please use != nullptr instead of the typecasting.

axic · 2018-05-17T07:25:19Z

Please add a change log entry, rebase and squash some of the commits (which belong together, i.e. the test changes).

chase1745 · 2018-05-21T05:45:42Z

Just pushed a squashed version of commits.
Where should this go in the change log since 0.4.24 was just released? 0.5.0?

chriseth · 2018-05-28T19:59:10Z

@chase1745 yes, please put this inside the 0.5.0 section.

chase1745 · 2018-06-06T04:29:06Z

Added entry to changelog.

axic · 2018-06-12T16:16:26Z

@chase1745 sorry for not responding earlier. This is failing test, mostly in syntaxTests, which means you need to isoltest to regenerate the expectations (since you've added the warning).

axic · 2018-06-12T16:16:58Z

libsolidity/analysis/ReferencesResolver.cpp

+									"for parameters in publicly visible functions."
+							);
+						else
+							m_errorReporter.warning(


Since this is listed as a breaking change, there is no need to have the non-0.5.0 mode here.

axic · 2018-06-12T16:17:07Z

libsolidity/analysis/ReferencesResolver.cpp

+									"\"memory\" or \"storage\" for parameters."
+							);
+						else
+							m_errorReporter.warning(


chase1745 · 2018-06-15T05:05:32Z

I removed checking for non 0.5.0 mode since this is now a breaking change. I also fixed the new tests to represent this and fixed other non-related tests to not use default data location so they could run as intended without receiving this new error. All isoltests are passing for me now.

chase1745 · 2018-06-15T05:16:14Z

Quite a few non-syntax tests are failing now since its a pretty big change. I'll be going through and fixing what I can find over the weekend.

chase1745 · 2018-06-19T03:30:31Z

Still working on them, there's a good number of failures because of this. There also seems to be quite a few external tests throwing errors because of this, not sure what to do about those?

axic · 2018-06-19T16:34:16Z

@chase1745 regarding the external tests:

please fix solc-js and create a PR
try to fix zeppelin and create a PR against https://github.com/axic/openzeppelin-solidity (against the solidity-050 branch)
ignore gnosis failures for the moment

chase1745 · 2018-07-08T21:18:05Z

@bit-shift thanks, yeah must've just missed that one in my OpenZeppelin PR, I'll fix that.

…t data location.

…ts to use explicit storage parameters to silence warnings.

…vent.

…rrors.

…parameters.

chase1745 · 2018-07-09T00:42:34Z

I rebased from develop and also moved tests to more appropriate locations. All tests should be passing once axic/openzeppelin-solidity#8 is merged.

christianparpart · 2018-07-09T08:06:18Z

libsolidity/analysis/ReferencesResolver.cpp

 		using Location = VariableDeclaration::Location;
 		Location varLoc = _variable.referenceLocation();
 		DataLocation typeLoc = DataLocation::Memory;
+		bool const v050 = _variable.sourceUnit().annotation().experimentalFeatures.count(ExperimentalFeature::V050);


The next release will be version 0.5.0, so there's no need [anymore] to optionally check for this, is it?

Yes, since this is a breaking change, the warning should be turned into an error (and therefor this check can be removed.)

erak

Please remove the check for pragma experimental "v0.5.0" and turn the warning into an error in ReferencesResolver::endVisit()

chase1745 · 2018-07-09T14:03:10Z

@bit-shift That is the the error for local variables, not related to this PR, so I didn’t change it, thought it may be better in a separate PR. Happy to change it though if you all think it’s fine for this PR.

erak · 2018-07-09T14:15:51Z

@chase1745 Ah yeah, right :) Let's do this in another PR. This one is already pretty large.

chase1745 · 2018-07-09T14:42:22Z

@bit-shift Sounds good, I’ll try to submit one this afternoon. Can you re-trigger the tests now that @axic merged in the changes to OpenZeppelin?

erak · 2018-07-09T15:23:24Z

@chase1745 The external test is still failing (SignatureBouncer.sol). Here's you can run them locally:

Download soljson.js, built on Circle in the build_emscripten step: https://21670-40892817-gh.circle-artifacts.com/0/soljson.js
Place the file under [your build directory]/libsolc/soljson.js
Run test/externalTests.sh [your build directory]/libsolc/soljson.js in the root of Solidity

Doing this will help to find breaking contracts :)

chase1745 · 2018-07-10T01:04:44Z

🙄I'll find them all this time! Thanks for the tip!

chase1745 · 2018-07-10T02:01:12Z

@bit-shift I found 2 parameters in that file to fix, and am almost positive that is all of them in OpenZeppelin, but running the tests the way you said, the current solidity-050 branch passes all tests before adding in these changes. Any ideas why? I want to make sure I get all of them this time.

chriseth · 2018-07-11T22:29:19Z

Sorry, this needs to be rebased now. Perhaps it would make sense to split this PR into multiple PRs:

one that updates the syntax tests
one that updates the end to end tests
one that updates the external tests
one that enforces the logic

chase1745 · 2018-07-11T23:53:58Z

@chriseth Yeah that makes sense I think. I'll work on that and submit some PR's.

chase1745 · 2018-07-14T22:00:55Z

@chriseth Submitted PR's for syntax tests (merged), external tests (#4512), and end to end tests (#4489). Once all 3 have been merged I'll submit a PR for the actual logic so it passes tests and I don't have to rebase a bunch of times.

vs77bb · 2018-07-20T06:15:38Z

@chase1745 Great work here. We're happy to tip for all the extra effort you put in - excited to pay out soon 👍

axic · 2018-07-24T22:47:32Z

@chase1745 can this PR be closed? I think #4518 is the last in the row superseding this one.

gitcoinbot mentioned this pull request Apr 30, 2018

Do not use default data location and warn users #3402

Closed

chriseth suggested changes May 2, 2018

View reviewed changes

chriseth reviewed May 14, 2018

View reviewed changes

axic reviewed May 17, 2018

View reviewed changes

chase1745 force-pushed the no-default-loc-params branch from 801b941 to 36f9b1e Compare May 21, 2018 05:53

chase1745 force-pushed the no-default-loc-params branch 2 times, most recently from fc94f0a to 9d9a9a3 Compare June 6, 2018 04:28

axic reviewed Jun 12, 2018

View reviewed changes

axic changed the title ~~Warn if function parameter (return) uses default data location.~~ [BREAKING] Warn if function parameter (return) uses default data location. Jun 12, 2018

chase1745 force-pushed the no-default-loc-params branch 2 times, most recently from 91c9c1e to b91c11b Compare June 15, 2018 05:02

chase1745 changed the title ~~[BREAKING] Warn if function parameter (return) uses default data location.~~ [BREAKING] Throw error if function parameters (return) use default data location. Jun 21, 2018

chase1745 mentioned this pull request Jun 21, 2018

Added data locations for function parameters and return parameters. argotorg/solc-js#228

Merged

Warn (error - 0.5) if function parameter (return as well) uses defaul…

8f8f79d

…t data location.

chase1745 force-pushed the no-default-loc-params branch from 84eee29 to b2300ac Compare July 8, 2018 22:51

chase1745 added 2 commits July 8, 2018 18:08

Added tests for default data location for parameters. Fixed older tes…

dbd3cbc

…ts to use explicit storage parameters to silence warnings.

Added isEventParameter to tell if parameter is being called from an e…

022b0d7

…vent.

chase1745 force-pushed the no-default-loc-params branch from b2300ac to 0c25155 Compare July 8, 2018 23:22

chase1745 added 2 commits July 8, 2018 18:31

Fixed older tests to use explicit "memory" data location to silence e…

c1623e6

…rrors.

Added documentation for removing default data locations for function …

3cb1bc1

…parameters.

chase1745 force-pushed the no-default-loc-params branch from 0c25155 to 3cb1bc1 Compare July 8, 2018 23:34

christianparpart reviewed Jul 9, 2018

View reviewed changes

erak requested changes Jul 9, 2018

View reviewed changes

leonardoalt mentioned this pull request Jul 9, 2018

050 Breaking Features TODO list #4401

Closed

29 tasks

chase1745 mentioned this pull request Jul 17, 2018

[BREAKING] Throw error for no data location for function params. #4518

Closed

chase1745 closed this Jul 25, 2018

[BREAKING] Throw error if function parameters (return) use default data location. #4014

[BREAKING] Throw error if function parameters (return) use default data location. #4014

Uh oh!

Conversation

chase1745 commented Apr 28, 2018 • edited by axic Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

chase1745 commented May 12, 2018

Uh oh!

chriseth commented May 14, 2018

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!

Choose a reason for hiding this comment

Uh oh!

axic commented May 17, 2018

Uh oh!

chase1745 commented May 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chriseth commented May 28, 2018

Uh oh!

chase1745 commented Jun 6, 2018

Uh oh!

axic commented Jun 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chase1745 commented Jun 15, 2018

Uh oh!

chase1745 commented Jun 15, 2018

Uh oh!

chase1745 commented Jun 19, 2018

Uh oh!

axic commented Jun 19, 2018

Uh oh!

chase1745 commented Jul 8, 2018

Uh oh!

chase1745 commented Jul 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erak Jul 9, 2018 • 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!

erak left a comment

Choose a reason for hiding this comment

Uh oh!

chase1745 commented Jul 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erak commented Jul 9, 2018

Uh oh!

chase1745 commented Apr 28, 2018 •

edited by axic

Loading

chase1745 commented May 21, 2018 •

edited

Loading

erak Jul 9, 2018 •

edited

Loading

chase1745 commented Jul 9, 2018 •

edited

Loading

erak commented Jul 9, 2018 •

edited

Loading

chase1745 commented Jul 10, 2018 •

edited

Loading