Enforce data location. #4738

chriseth · 2018-08-07T17:14:10Z

chriseth · 2018-08-07T17:15:30Z

libsolidity/analysis/NameAndTypeResolver.cpp

 			if (!resolveNamesAndTypes(*node, false))
 			{
 				success = false;
-				break;


I'm not sure why this was here. I will run the whole test suite once the expectations are all updated, if it does not crash, I would propose to keep it like this.

axic · 2018-08-07T20:07:56Z

Needs rebasing.

codecov · 2018-08-07T21:11:26Z

Codecov Report

Merging #4738 into develop will decrease coverage by 0.01%.
The diff coverage is 85.6%.

@@             Coverage Diff             @@
##           develop    #4738      +/-   ##
===========================================
- Coverage    87.79%   87.77%   -0.02%     
===========================================
  Files          314      314              
  Lines        31107    31159      +52     
  Branches      3690     3688       -2     
===========================================
+ Hits         27310    27351      +41     
- Misses        2548     2552       +4     
- Partials      1249     1256       +7

Flag	Coverage Δ
#all	`87.77% <85.6%> (-0.02%)`	⬇️
#syntax	`28.41% <84%> (+0.08%)`	⬆️

axic · 2018-08-07T21:20:04Z

libsolidity/analysis/ReferencesResolver.cpp

 		return;

+	if (_variable.isConstant() && !_variable.isStateVariable())
+		m_errorReporter.declarationError(_variable.location(), "The \"constant\" keyword can only be used for state variables.");


Is this actually part of this PR?

Previously, the checks were much less restrictive, so the checks would pass here and then the type checker did the rest, but I actually think it is better to have strict tests here and then also move this check here, which I would say is the appropriate place.

Ok perhaps to summarize differently: Other ways to solve this would result in weird error messages.

Actually, to me it seems this check should even be moved up to the SyntaxChecker instead. Using the constant keyword in the wrong place feels like a syntax error to me.

If you agree, I would still like to keep it here. While it can be checked earlier, I think it still "belongs" here, especially when we also add things like mutable and immutable.

I'm fine with keeping it here.

chriseth · 2018-08-07T21:48:33Z

libsolidity/analysis/TypeChecker.cpp

 		expectType(*_variable.value(), *varType);
 	if (_variable.isConstant())
 	{
-		if (!_variable.isStateVariable())


This is the place where the constant check was previously.

chriseth · 2018-08-08T12:12:08Z

libsolidity/analysis/ReferencesResolver.cpp

 	if (_typeName.isPayable() && _typeName.visibility() != VariableDeclaration::Visibility::External)
 	{
-		typeError(_typeName.location(), "Only external function types can be payable.");
+		fatalTypeError(_typeName.location(), "Only external function types can be payable.");


These have to be fatal because otherwise the constructor of FunctionType will assert.

chriseth · 2018-08-08T12:21:21Z

libsolidity/analysis/ReferencesResolver.cpp

-						"Location has to be calldata for external functions "
-						"(remove the \"memory\" or \"storage\" keyword)."
-					);
+				case Location::Memory:


Could be pulled up onto the same line and unindented once.

chriseth · 2018-08-08T12:27:09Z

libsolidity/analysis/ReferencesResolver.cpp

 	m_errorReporter.fatalDeclarationError(_location, _description);
 }
+
+DataLocation ReferencesResolver::variableLocationToDataLocation(VariableDeclaration::Location _varLoc)


This is unused, should either be used above or removed.

chriseth · 2018-08-08T12:29:09Z

libsolidity/ast/AST.cpp

+	if (!hasReferenceOrMappingType() || isStateVariable())
+		return set<Location>{ Location::Default };
+
+	if (isEventParameter())


change to else if

chriseth · 2018-08-08T12:29:57Z

libsolidity/ast/AST.cpp

+	}
+	else if (isLocalVariable())
+	{
+		// TODO where was this checked in the previous version?


remove - it was in reference resolver visit.

chriseth · 2018-08-08T12:30:52Z

test/libsolidity/ASTJSON/function_type_legacy.json

 															{
 																"constant" : false,
 																"name" : "",
-																"scope" : 16,


Thanks for adding this to isoltest, @ekpyron ;)

Although it's still a bit hacky, e.g. it's adding the // ---- delimiter above, although it's not needed for ASTJSON tests - but I'll fix all that in 0.5.1.

chriseth · 2018-08-08T12:32:40Z

test/libsolidity/syntaxTests/dataLocations/data_location_in_function_type.sol

@@ -0,0 +1,11 @@
+library L {
+    struct Nested { uint y; }
+    // data location specifier in function signature should be optional even if there are multiple choices


Remove comment and also extract the working case.

chriseth · 2018-08-08T12:33:36Z

test/libsolidity/syntaxTests/dataLocations/function_parameters_with_data_location_fine.sol

@@ -0,0 +1,9 @@
+contract C {
+    // Warning for no data location provided can be silenced with storage or memory.


chriseth · 2018-08-08T12:33:42Z

...libsolidity/syntaxTests/dataLocations/function_return_parameters_with_data_location_fine.sol

@@ -0,0 +1,7 @@
+contract C {
+    // Shows that the warning for no data location provided can be silenced with storage or memory.


ekpyron

Only had a very quick look so far, so for now only two very minor comments.

ekpyron · 2018-08-08T16:25:40Z

libsolidity/ast/AST.cpp

+	if (isReturnParameter())
+		return true;
+
+	std::vector<ASTPointer<VariableDeclaration>> const* parameters = nullptr;


No need for std::.

ekpyron · 2018-08-08T16:31:40Z

libsolidity/ast/AST.cpp

-		return false;
-	if (callable->returnParameterList())
-		for (auto const& variable: callable->returnParameterList()->parameters())
+	std::vector<ASTPointer<VariableDeclaration>> const* returnParameters = nullptr;


std:: again

ekpyron

This would probably have been easier to review, if it was split up into several commits/PRs, but I think it should be fine. I added some minor comments.

ekpyron · 2018-08-13T10:54:49Z

libsolidity/analysis/ReferencesResolver.cpp

 		return;

+	if (_variable.isConstant() && !_variable.isStateVariable())
+		m_errorReporter.declarationError(_variable.location(), "The \"constant\" keyword can only be used for state variables.");


Actually, to me it seems this check should even be moved up to the SyntaxChecker instead. Using the constant keyword in the wrong place feels like a syntax error to me.

ekpyron · 2018-08-13T11:12:15Z

...sts/dataLocations/libraries/library_internal_function_return_parameters_no_data_location.sol

@@ -0,0 +1,6 @@
+library L {
+    function g(uint[]) internal pure returns (uint[2]) {}


Maybe better to remove the function argument here?

ekpyron · 2018-08-13T11:12:32Z

...syntaxTests/dataLocations/libraries/library_private_function_parameters_no_data_location.sol

@@ -0,0 +1,6 @@
+library L {
+    function h(uint[]) private pure returns (uint[]) {}


And remove the return value here?

ekpyron · 2018-08-13T11:13:32Z

...Tests/dataLocations/libraries/library_public_function_return_parameters_no_data_location.sol

@@ -0,0 +1,6 @@
+library L {
+    function h(uint[]) public pure returns (uint[]) {}


Probably better to rename the test or split it into two (an return parameter and function parameter version; at the moment I don't see a function parameter version for this, but there is one for private functions).

I actually don't see much point in splitting, but it should probably be two different functions.

ekpyron · 2018-08-13T11:15:44Z

test/libsolidity/syntaxTests/nameAndTypeResolution/224_string_bytes_conversion.sol

-// Warning: (215-260): Function state mutability can be restricted to pure
-// Warning: (265-317): Function state mutability can be restricted to pure
-// Warning: (322-358): Function state mutability can be restricted to view
+// Warning: (47-108): Function state mutability can be restricted to pure


Maybe we should get rid of these warnings altogether.

ekpyron · 2018-08-13T11:17:45Z

test/libsolidity/syntaxTests/parsing/location_specifiers_for_params.sol

 }
 // ----
-// TypeError: (30-55): Data location has to be "memory" (or unspecified) for constants.
+// DeclarationError: (30-55): The "constant" keyword can only be used for state variables.


This test looks a bit weird to me - what is it actually supposed to test?

Will remove it.

Hm, seems to be an old test. I think I will keep it for now.

It is one of the few tests that combine 'constant' with params

We could rename it (not sure to what, though), but I'm fine with just keeping it as it is.

ekpyron · 2018-08-13T11:20:09Z

test/libsolidity/syntaxTests/types/mapping/mapping_return_public_memory.sol

+    }
+}
+// ----
+// TypeError: (51-79): Type is required to live outside storage.


These expectations don't really match the intention of the test I guess - but it's probably good to keep the test - the expectations should properly align with/after #4798. [so nothing to be done here]

Yes, should be in conflict with #4798 so good to keep.

chriseth · 2018-08-13T20:35:13Z

OK, hopefully final update. Will squash after review.

ekpyron · 2018-08-14T08:50:01Z

Emscripten build fails on travis.
Either we need #include <iterator> in StringUtils.h or the older emscripten is not fully C++11 compatible - ~~which~~ the latter would be another reason for #4486, it seems to work on circleci.

chriseth · 2018-08-14T09:14:03Z

Actually I think that is an argument against using the same version.

ekpyron · 2018-08-14T09:15:31Z

That depends on whether it's the lack of a header include or the lack of C++11 support... If it's the former, then yes, if it's the latter, then bumping the minimum required version makes more sense, I would say.

chriseth · 2018-08-14T09:20:46Z

Only read half-way into this, but the problem may be that transformed only gets us a single-pass iterator, while std::next requires a forward iterator.

chriseth · 2018-08-14T09:25:09Z

Not sure why it works in the other cases, but interestingly, since C++17, there is also an input iterator version of std::next...

ekpyron · 2018-08-14T09:25:35Z

That's probably right - and I guess newer compiler (resp. STL) versions are forward compatible with C++17, for which std::next only requires an input iterator. In that sense maybe it is an argument against #4486.

chriseth · 2018-08-14T09:32:26Z

Updated, let's see what travis will do with it.

chriseth · 2018-08-14T10:18:00Z

Build passed.

ekpyron · 2018-08-14T11:21:24Z

Why are the circleci linux tests passing and there is a test failure for one of the appveyor builds? Not related to this PR, but weird.

ekpyron · 2018-08-14T12:35:29Z

It's actually very weird - the respective test case syntaxTests/conversion/conversion_to_bytes.sol is not actually in this branch - not sure how it ended up in the appveyor test run, but in any case, the test is in develop and will fail after this PR, if the expectations are not updated, so this PR needs to be rebased again.

chriseth · 2018-08-14T13:29:01Z

Yeah, it sometimes happens that appveyor picks the wrong commit.

chriseth · 2018-08-14T16:53:29Z

Another run...

chriseth commented Aug 7, 2018

View reviewed changes

This was referenced Aug 7, 2018

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

Closed

Rewrite/redesign data location checking #4425

Closed

ekpyron mentioned this pull request Aug 7, 2018

Do not stop after the first error in reference resolution. #4745

Merged

chriseth force-pushed the dataloc_merged branch from 0ac2a81 to 57293f7 Compare August 7, 2018 21:11

axic reviewed Aug 7, 2018

View reviewed changes

chriseth commented Aug 7, 2018

View reviewed changes

chriseth force-pushed the dataloc_merged branch from 57293f7 to 5a38b2d Compare August 7, 2018 21:49

nventuro mentioned this pull request Aug 8, 2018

Added explicit data locations for function parameters to comply with Solidity v050 changes. OpenZeppelin/openzeppelin-contracts#1068

Closed

4 tasks

chriseth changed the title ~~[WIP] Enforce data location.~~ Enforce data location. Aug 8, 2018

chriseth force-pushed the dataloc_merged branch from 07ff13c to 6281484 Compare August 8, 2018 10:53

chriseth commented Aug 8, 2018

View reviewed changes

chriseth force-pushed the dataloc_merged branch from 6281484 to 6b4837b Compare August 8, 2018 12:50

chriseth mentioned this pull request Aug 8, 2018

050 Breaking Features TODO list #4401

Closed

29 tasks

ekpyron reviewed Aug 8, 2018

View reviewed changes

chriseth mentioned this pull request Aug 8, 2018

External functions somehow require storage pointers #4771

Closed

chriseth force-pushed the dataloc_merged branch from 6b4837b to 87d4d87 Compare August 8, 2018 20:45

chriseth mentioned this pull request Aug 9, 2018

Fix data location of external reference parameters. #4790

Merged

ekpyron reviewed Aug 13, 2018

View reviewed changes

chriseth mentioned this pull request Aug 13, 2018

Grammar: Type names are expressions #4807

Closed

chriseth force-pushed the dataloc_merged branch from 0d3300f to 9ae0138 Compare August 14, 2018 13:51

chase1745 and others added 2 commits August 14, 2018 18:07

Refactor data location check.

683bce1

Update documentation.

e3b6c5a

chriseth force-pushed the dataloc_merged branch from 9ae0138 to 09bd3b1 Compare August 14, 2018 16:16

ekpyron previously approved these changes Aug 14, 2018

View reviewed changes

chase1745 and others added 5 commits August 14, 2018 18:53

Update tests

b000a02

New test cases.

16de7a0

Update tests.

13905a2

Update external tests.

b30da88

Make joinHumanReadable work for input iterators.

14e116c

chriseth dismissed ekpyron’s stale review via 14e116c August 14, 2018 16:53

chriseth force-pushed the dataloc_merged branch from 09bd3b1 to 14e116c Compare August 14, 2018 16:53

chriseth merged commit cc54f6c into develop Aug 14, 2018

chriseth deleted the dataloc_merged branch August 16, 2018 12:57

gitcoinbot mentioned this pull request Aug 17, 2018

Do not use default data location and warn users #3402

Closed

This was referenced Aug 27, 2018

Crash when declaring external function with array of struct that possesses arrays #4713

Closed

Crash when using struct as external function parameter using ABIEncoderV2 #4714

Closed

		@@ -0,0 +1,9 @@
		contract C {
		// Warning for no data location provided can be silenced with storage or memory.

		@@ -0,0 +1,7 @@
		contract C {
		// Shows that the warning for no data location provided can be silenced with storage or memory.

		@@ -0,0 +1,6 @@
		library L {
		function g(uint[]) internal pure returns (uint[2]) {}

		@@ -0,0 +1,6 @@
		library L {
		function h(uint[]) private pure returns (uint[]) {}

		@@ -0,0 +1,6 @@
		library L {
		function h(uint[]) public pure returns (uint[]) {}

Enforce data location. #4738

Enforce data location. #4738

Uh oh!

Conversation

chriseth commented Aug 7, 2018 • edited 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!

axic commented Aug 7, 2018

Uh oh!

codecov bot commented Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

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!

Choose a reason for hiding this comment

Uh oh!

ekpyron left a comment

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!

ekpyron left a comment

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!

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

chriseth commented Aug 7, 2018 •

edited

Loading

codecov bot commented Aug 7, 2018 •

edited

Loading

ekpyron commented Aug 14, 2018 •

edited

Loading

ekpyron commented Aug 14, 2018 •

edited

Loading