[BREAKING] Disallow legacy constructor by erak · Pull Request #4215 · argotorg/solidity

erak · 2018-06-01T23:14:16Z

Closes #4106.

axic · 2018-06-02T03:34:30Z

Sorry I forgot to assign the issue to myself, but did mention the last call I started this work. Anyway it is not a big amount of duplicate effort :)

However we should only merge this after #4208

erak · 2018-06-02T14:43:32Z

@axic Oh sorry, did not get this in the last call. We should try to keep the project board up to date ;)
So, should we use with this branch / PR?

I agree, let's merge the other PR first.

axic · 2018-06-02T17:06:03Z

@bit-shift I will have a look at the differences between the two PRs. Do you want to review #4208 ?

axic · 2018-06-03T23:38:58Z

My local version of these changes are all included int this PR.

erak · 2018-06-05T17:06:52Z

@christianparpart I removed the expected shadowing warning of override_constructor.sol and constructor_old.sol. The old version of override_constructor.sol was basically testing that you can have a function that is called like the constructor of a contracts base contract. Since this construct is not possible anymore, we also should not warn anymore.
And getting the warning if contract A { function A() public {} } feels a little bit like clutter.

chriseth

Is there a test for

contract C {
function constructor() { ... }
}

?

chriseth · 2018-06-05T17:12:34Z

libsolidity/analysis/SyntaxChecker.cpp

+bool SyntaxChecker::visit(ContractDefinition const& _contract)
+{
+	ASTString const& contractName = _contract.name();
+	for (const FunctionDefinition* const function : _contract.definedFunctions())


Should be FunctionDefinition const* const function, but I think the last const does not really help readability and thus could be dropped.

chriseth · 2018-06-05T17:13:55Z

libsolidity/analysis/SyntaxChecker.cpp

+	for (const FunctionDefinition* const function : _contract.definedFunctions())
+		if (function->name() == contractName)
+			m_errorReporter.syntaxError(function->location(),
+			"Defining constructors as functions with the same name as the contract is not allowed. "


Please use the original v050 message and not the message of the pre-050 warning. Also, the indentation and line breaks are not according to coding style.

chriseth · 2018-06-05T17:15:07Z

libsolidity/analysis/SyntaxChecker.cpp

 			m_errorReporter.warning(_function.location(), "Modifiers of functions without implementation are ignored." );
 	}
-	if (_function.name() == "constructor")
+	if (_function.name() == "constructor" && !_function.isConstructor())


Please don't add !_function.isConstructor(). Currently, it looks like this is a good thing to add, but it might hide errors that are introduced when we change the logic of isConstructor.

chriseth · 2018-06-05T17:17:06Z

libsolidity/parsing/Parser.cpp


-	if (result.isConstructor || _forceEmptyName || m_scanner->currentToken() == Token::LParen)
+	if (result.isConstructor)
+		result.name = make_shared<ASTString>("constructor");


I think the name should still be empty.

chriseth · 2018-06-05T17:21:21Z

libsolidity/parsing/Parser.cpp

+		result.name = make_shared<ASTString>("constructor");
+	else if (_forceEmptyName || m_scanner->currentToken() == Token::LParen)
 		result.name = make_shared<ASTString>();
+	else if (m_scanner->currentToken() == Token::Constructor && !result.isConstructor)


Is this to still allow function constructor(...) { ..}? I don't think we have to support that - constructor is now a real keyword and thus functions cannot be named constructor.

Yes, the idea was to still allow it, but warn about it's usage. But you're right, since this is keyword now, we should not allow it anymore.

!result.isConstructor can be removed (it is an else if).

chriseth · 2018-06-05T17:23:06Z

test/libsolidity/syntaxTests/constructor/overriding_constructor.sol

-// It is fine to "override" constructor of a base class since it is invisible
-contract A { function A() public { } }
+contract A { constructor() public {} }
 contract B is A { function A() public pure returns (uint8) {} }


Yep, that is exactly what we should test here!

chriseth · 2018-06-07T10:06:30Z

test/libsolidity/SolidityEndToEndTest.cpp

-			function Main() payable {
-				h = (new Helper).value(10)("abc", true);
+			constructor() payable {
+				h = (new Helper).value(10)('abc', true);


Any reason for this change?

Oh, this was an unconscious change due to broken code highlighting in my editor. We can revert to "abc"

chriseth · 2018-06-07T10:08:00Z

test/libsolidity/SolidityParser.cpp

 		"after",
 		"case",
 		"catch",
+		"constructor"


constructor should not be reserved, it should be a keyword "in use".

axic · 2018-06-11T11:31:40Z

Depends on #4206.

axic · 2018-06-11T11:32:25Z

This could be fixed without the external tests:

ASSERTION FAILURE:
- file   : SolidityABIJSON.cpp
- line   : 49
- message: Parsing contract failed

Also syntax test expectations need to be updated.

axic · 2018-06-12T00:27:18Z

libsolidity/parsing/Parser.cpp

 		result.name = make_shared<ASTString>();
+	else if (_forceEmptyName || m_scanner->currentToken() == Token::LParen)
+		result.name = make_shared<ASTString>();
+	else if (m_scanner->currentToken() == Token::Constructor && !result.isConstructor)


I don't think this can happen since expectIdentifierToken shouldn't allow a non-identifier, such as Token::Constructor. Is there a test for this which needs this extra condition here?

@axic This check should have been removed already. Thanks!

erak · 2018-06-12T13:12:21Z

@axic I updated the broken ABI tests.

erak · 2018-06-12T13:21:11Z

@chriseth The file function_named_constructor.sol contains a test for

contract C { function constructor() { ... }}

chriseth · 2018-06-13T10:15:19Z

Please rebase to trigger the tests.

chriseth · 2018-06-27T13:18:20Z

Tests are failing.

erak · 2018-06-27T13:44:48Z

We need to merge #4354 first. After merging, this PR needs to be updated such that it removes the old syntax tests that expect a warning instead of an error.

erak · 2018-06-29T15:08:05Z

Updated this PR again after #4354 got merged. Should be ready for final review now.

erak · 2018-06-29T16:01:01Z

I missed the docs here, but will add these changes.

erak · 2018-07-02T14:29:55Z

I've split out the required changes to the documentation: #4402. After that PR got merged, we can merge this one.

ekpyron · 2018-07-04T13:34:03Z

libsolidity/analysis/NameAndTypeResolver.cpp

+				dynamic_cast<FunctionDefinition const*>(&_declaration) &&
+				name == decl->name())
+			)
+				shadowedDeclaration = decl;


So only something that is a function can be shadowed by something that's not a contract with the same name?
What about contract C { uint msg; }? Currently this warns: This declaration shadows a builtin symbol.. And I think this comes from below in this function (I may be wrong) - does that still work? Is there a test for it?

We have a test for the case you've mentioned: 452_shadowing_builtins_with_storage_variables.sol

After we talked about that: I've improved the formatting to make the condition easier to understand 😄

ekpyron

Looks fine to me now.

chriseth · 2018-07-10T20:48:25Z

libsolidity/analysis/NameAndTypeResolver.cpp

@@ -476,7 +476,15 @@ bool DeclarationRegistrationHelper::registerDeclaration(
 	Declaration const* shadowedDeclaration = nullptr;
 	if (_warnOnShadow && !name.empty() && _container.enclosingContainer())
 		for (auto const* decl: _container.enclosingContainer()->resolveName(name, true, true))


Unrelated to this PR, but why do we have a for loop here?

chriseth · 2018-07-10T20:49:05Z

libsolidity/analysis/NameAndTypeResolver.cpp

 	if (_warnOnShadow && !name.empty() && _container.enclosingContainer())
 		for (auto const* decl: _container.enclosingContainer()->resolveName(name, true, true))
-			shadowedDeclaration = decl;
+			// Do not warn about functions shadowing a contract.


Can you explain the reason for this?

@chriseth Without this change, the following code would result in a warning about function A() shadowing contract A:

contract A { constructor() public {} } contract B is A { function A() public pure returns (uint8) {} }

Since functions can now have the same name as a previously declared contract, this should not warn. We also have a test for that: overriding_constructor.sol.

I would say the warning is correct, since it does shadow A. Note the following does not work because the function A shadows the contract A:

contract A { function f() public {} } contract B is A { function A() public pure returns (uint8) {} function g() public { A.f(); } }

chriseth · 2018-07-10T20:49:39Z

libsolidity/analysis/SyntaxChecker.cpp

+bool SyntaxChecker::visit(ContractDefinition const& _contract)
+{
+	ASTString const& contractName = _contract.name();
+	for (FunctionDefinition const* function : _contract.definedFunctions())


No space in front of :.

erak · 2018-07-17T22:08:13Z

Rebased and squashed some commits. @chriseth I've also addressed your comment about a function shadowing a contract: #4215 (comment).

chriseth · 2018-07-18T10:05:40Z

test/libsolidity/syntaxTests/constructor/constructor.sol

@@ -0,0 +1,9 @@
+contract A { 


Please move the positive case into its own test.

erak · 2018-07-18T12:44:45Z

@chriseth I reverted merging both, the new and the old constructor syntax, into one file. I also rebased the branch.

erak force-pushed the constructorWorkaround branch 5 times, most recently from 8dc5a06 to df89722 Compare June 2, 2018 00:22

axic mentioned this pull request Jun 5, 2018

[WIP] Constructor keyword for version 0.5.0 #4230

Closed

erak force-pushed the constructorWorkaround branch from 870a0eb to ee59eec Compare June 5, 2018 14:40

chriseth suggested changes Jun 5, 2018

View reviewed changes

chriseth reviewed Jun 7, 2018

View reviewed changes

erak force-pushed the constructorWorkaround branch from 10a63bb to ef49d2c Compare June 11, 2018 10:25

axic reviewed Jun 12, 2018

View reviewed changes

axic changed the title ~~[WIP] Disallow legacy constructor~~ [WIP] [BRAKING] Disallow legacy constructor Jun 12, 2018

axic changed the title ~~[WIP] [BRAKING] Disallow legacy constructor~~ [WIP] [BREAKING] Disallow legacy constructor Jun 12, 2018

erak force-pushed the constructorWorkaround branch from df180f5 to 024e897 Compare June 13, 2018 10:06

erak force-pushed the constructorWorkaround branch 5 times, most recently from 91ad649 to b4aaa30 Compare June 13, 2018 15:17

erak force-pushed the constructorWorkaround branch from 85ca746 to 5b3e08b Compare June 29, 2018 15:05

erak force-pushed the constructorWorkaround branch 2 times, most recently from 96d449c to bc5f671 Compare July 2, 2018 14:24

erak mentioned this pull request Jul 2, 2018

Update docs to new constructor syntax #4402

Merged

leonardoalt mentioned this pull request Jul 2, 2018

050 Breaking Features TODO list #4401

Closed

29 tasks

erak force-pushed the constructorWorkaround branch from bc5f671 to 7510875 Compare July 4, 2018 09:59

ekpyron reviewed Jul 4, 2018

View reviewed changes

ekpyron previously approved these changes Jul 4, 2018

View reviewed changes

erak self-assigned this Jul 6, 2018

chriseth reviewed Jul 10, 2018

View reviewed changes

erak dismissed ekpyron’s stale review via 06db389 July 17, 2018 20:03

erak force-pushed the constructorWorkaround branch 2 times, most recently from 015c823 to 2c01950 Compare July 17, 2018 21:50

chriseth reviewed Jul 18, 2018

View reviewed changes

erak added 3 commits July 18, 2018 14:29

Disallows old constructor syntax.

182a0a9

Adjusts syntax tests to new constructor syntax.

de6cd24

Adds warning if function is shadowing a contract.

b0b35e1

erak force-pushed the constructorWorkaround branch from 2c01950 to b0b35e1 Compare July 18, 2018 12:42

chriseth approved these changes Jul 18, 2018

View reviewed changes

erak merged commit ccb5fcc into develop Jul 18, 2018

chriseth deleted the constructorWorkaround branch August 16, 2018 12:58

Conversation

erak commented Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

axic commented Jun 2, 2018

Uh oh!

erak commented Jun 2, 2018

Uh oh!

axic commented Jun 2, 2018

Uh oh!

axic commented Jun 3, 2018

Uh oh!

erak commented Jun 5, 2018

Uh oh!

chriseth 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

Uh oh!

axic commented Jun 11, 2018

Uh oh!

axic commented Jun 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erak commented Jun 12, 2018

Uh oh!

erak commented Jun 12, 2018

Uh oh!

chriseth commented Jun 13, 2018

Uh oh!

chriseth commented Jun 27, 2018

Uh oh!

erak commented Jun 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erak commented Jun 29, 2018

Uh oh!

erak commented Jun 29, 2018

Uh oh!

erak commented Jul 2, 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!

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!

erak commented Jun 1, 2018 •

edited

Loading

erak commented Jun 27, 2018 •

edited

Loading

erak commented Jul 2, 2018 •

edited

Loading