Solidity: Do not use default data location and warn users

[chriseth]

We should warn users about default data location for function parameters and return parameters.

In void ReferencesResolver::endVisit(VariableDeclaration const& _variable), ReferencesResolver.cpp:221, the case where varLoc == Location::Default produces warnings (or errors starting from version 0.5.0) only for local variables (line 301).

We should do the same at least for the case where the contract is a library.

[chriseth]

This is a good first task for external contributors.

[GNSPS]

I think this is super important! Many entries to the underhanded Solidity contest would have been avoided. 😄

[federicobond]

Willing to mentor someone who wants to make their first contribution to Solidity 🙋🏻‍♂️

[chriseth]

@GNSPS note that we already fixed it for local variables, I don't think function arguments were used in the underhanded contest, but of course you are right :)

[makoto]

Never done any cpp (only js, ruby and solidity) but sort of interested.

Is it a bit like this?

diff --git a/libsolidity/analysis/ReferencesResolver.cpp b/libsolidity/analysis/ReferencesResolver.cpp
index 540ffaf..59c343a 100644
--- a/libsolidity/analysis/ReferencesResolver.cpp
+++ b/libsolidity/analysis/ReferencesResolver.cpp
@@ -300,7 +300,12 @@ void ReferencesResolver::endVisit(VariableDeclaration const& _variable)
                                                typeLoc = DataLocation::Storage;
                                                if (_variable.isLocalVariable())
                                                {
-                                                       if (_variable.sourceUnit().annotation().experimentalFeatures.count(ExperimentalFeature::V050))
+                                                       if (varLoc == Location::Default)
+                                                               m_errorReporter.warning(
+                                                                       _variable.location(),
+                                                                       "Do not use default data location"
+                                                               );
+                                                       else if (_variable.sourceUnit().annotation().experimentalFeatures.count(ExperimentalFeature::V050))
                                                                typeError(
                                                                        _variable.location(),
                                                                        "Storage location must be specified as either \"memory\" or \"storage\"."

Which test should I run to see if I haven't broken anything? https://solidity.readthedocs.io/en/latest/contributing.html#running-the-compiler-tests shows several ways to test but not sure which one is related to this file.

Also do you have any sample solidity code to reproduce this condition?

[chriseth]

@makoto You added it to the case where we indeed have a local variable. The task would be to take roughly the block underneath of what you inserted and put it at some place which is about 50 lines further up. Please try to understand the code, though :)

[makoto]

Yes, I had no idea what I was doing. I now read the code and hopefully, this is getting a bit closer to what you asked for.

https://github.com/ethereum/solidity/compare/develop...makoto:no_default_on_parameter?expand=1

I am still not able to run the test locally so not sure if it's syntactically correct or not.

[makoto]

And I would very much appreciate mentorship of @federicobond . In your YouTube video you said you started from no experience of c++. Would be great if you can point to some noob introduction of c++, boost testing enough to be able to compile and run the test.

[federicobond]

Hi @makoto, sorry for the delay. Let me find some time tomorrow to review and give you some pointers on C++ (no pun intended). I had to take my laptop to repair and lost my Solidity setup.

Have you looked into this page to perform an initial build? http://solidity.readthedocs.io/en/develop/contributing.html

There is a section there on running the tests (use the --no-ipc option).

[makoto]

Thanks for the help, @federicobond . I am actually stuck at http://solidity.readthedocs.io/en/develop/installing-solidity.html#external-dependencies even before running test. Installing the build took a whole night (4 years old MBA) and looks like it screwed my node environment completely :-( Maybe time to buy a new machine and start from scratch. Is it easy to build on Mac or should I get Linux (or Chromebook if it's doable).

[federicobond]

The build on Mac should work fine. Can you paste which errors are you
getting?
El El sáb, 20 ene. 2018 a las 11:48, Makoto Inoue [e-mail]
escribió:

Thanks for the help, @federicobond https://github.com/federicobond . I
am actually stuck at
http://solidity.readthedocs.io/en/develop/installing-solidity.html#external-dependencies
even before running test. Installing the build took a whole night (4 years
old MBA) and looks like it screwed my node environment completely :-( Maybe
time to buy a new machine and start from scratch. Is it easy to build on
Mac or should I get Linux (or Chromebook if it's doable).

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/ethereum/solidity/issues/3402#issuecomment-359176633,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAIcuiZ6lBQhw0JVyibNq5CbdqMwSqCTks5tMfzWgaJpZM4RgTvn
.

[makoto]

Actually now build works on develop branch (took 11min), but running test throws 14 failures. Is this expected? https://gist.github.com/makoto/3e9aea718f8f57eaacff39e3724fab9e

[federicobond]

Your build looks fine. Try brew install z3 and rerunning the build script to fix the errors related to the SMT solver.

[makoto]

Thanks. I installed z3 and ran the build script, but now it got worse :-( Looks like it's failing to build now.

https://gist.github.com/makoto/fd904d33ad9813dfefa1608cb14f6f52

Also, are there any other ways to build quicker apart from ./scripts/build .sh? Waiting over 10 min just so that I can run the test seems quit unproductive or is it how usually work in compiling language world (as opposed to scripting language world like js or ruby)?

[federicobond]

I think those errors are related to homebrew updating to an incompatible
jsoncpp version. Check which versions you have installed and see if you can
revert back to the old one (usually uninstalling the latest manually works
for this case).

As for the time it takes to build, unless you touch header files (.h
extension) newer builds should only recompile the affected compilation
units. Headers are usually included in more files so any changes in them
end up propagating far to the rest of the codebase. In summary, if you run
scripts/build.sh twice it should be much faster the second time.
El El lun, 22 ene. 2018 a las 19:32, Makoto Inoue [e-mail]
escribió:

Thanks. I installed z3 and ran the build script, but now it got worse :-(
Looks like it's failing to build now.

https://gist.github.com/makoto/fd904d33ad9813dfefa1608cb14f6f52

Also, are there any other ways to build quicker apart from ./scripts/build
.sh? Waiting over 10 min just so that I can run the test seems quit
unproductive or is it how usually work in compiling language world (as
opposed to scripting language world like js or ruby)?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/ethereum/solidity/issues/3402#issuecomment-359593414,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAIcutcJD152rqD8wrpHctS3DuL5JfDpks5tNQyGgaJpZM4RgTvn
.

[kevinaud]

Are you still looking for someone to work on this? If you are I can work on it.

[makoto]

@kevinaud I am still doing this but very slowly more for a learning purpose. If you can provide the solution quicker, go for it. I probably can learn more from your PR anyway.

[gitcoinbot]

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

---

__This issue now has a funding of 250.0 DAI (250.0 USD @ $1.0/DAI) attached to it.__

• If you would like to work on this issue you can 'start work' on the Gitcoin Issue Details page.
• Questions? Checkout Gitcoin Help or the Gitcoin Slack
• $4,362.21 more funded OSS Work available on the Gitcoin Issue Explorer

[chase1745]

I can work on this if needed. It'd be my first issue in solidity, but I've been looking through the code quite a bit and think I understand it pretty well and know what needs to be done for this issue. Might need a little help with writing the tests though.

[gitcoinbot]

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

---

__Work has been started__.

These users each claimed they can complete the work by 2 months, 1 week from now.
Please review their action plans below:

1. chase1745 has started work.

Learn more on the Gitcoin Issue Details page.

[chriseth]

@chase1745 the tests for these things are pretty easy now. You write some code that you think should have a warning, then run the isoltest script and it adds the correct expectation.

[chriseth]

Here is the documentation: https://solidity.readthedocs.io/en/develop/contributing.html#writing-and-running-syntax-tests

[chase1745]

@chriseth so I wouldn't add the tests for this to test/libsolidity/SolidityNameAndTypeResolution.cpp (where the tests for warning about default storage for local variables are)?

[chase1745]

Also, should we warn users about the default data locations for parameters/returns when it's external, public, and else? Or just non public/external?

[chriseth]

Please create a new file in the syntaxTests directory. The warning should only be given if the user did not explicitly specify a storage location.

[gitcoinbot]

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

---

__Work for 250.0 DAI (250.0 USD @ $1.0/DAI) has been submitted by__:

1. @chase1745
2. @chase1745

If you are the bounty funder, please take a look at the submitted work:

• PR by @chase1745
• PR by @chase1745

---

• Learn more on the Gitcoin Issue Details page
• Want to chip in? Add your own contribution here.
• Questions? Checkout Gitcoin Help or the Gitcoin Slack
• $22,814.16 more funded OSS Work available on the Gitcoin Issue Explorer

[chriseth]

@axic this is being implemented. Do you agree on the semantics? Should we force data location for every function parameter and return parameter?

[vs77bb]

@chriseth Is this one good for payout?

[chriseth]

The pull request is still under review: https://github.com/ethereum/solidity/pull/4014

[chriseth]

Has this been paid out?