Warn if shadowing built-ins #1637

axic · 2017-02-02T13:24:41Z

axic · 2017-02-02T13:25:07Z

libsolidity/analysis/StaticAnalyzer.cpp

+
+	for (Declaration const* declaration: m_globals)
+		if (declaration->name() == _function.name())
+			warning(_function.location(), "Shadowing global function \"" + _function.name() + "\".");


Perhaps it is better to say "Shadowing builtin function".

It does not have to be a function. What about builtin symbol?

This is within visit(FunctionDefinition), but using builtin symbol for both cases is fine.

axic · 2017-02-02T13:25:13Z

libsolidity/analysis/StaticAnalyzer.cpp

+	for (Declaration const* declaration: m_globals)
+		if (declaration->name() == _variable.name())
+			warning(_variable.location(), "Shadowing global variable \"" + _variable.name() + "\".");
+


Perhaps it is better to say "Shadowing builtin variable".

axic · 2017-02-02T13:25:38Z

libsolidity/analysis/StaticAnalyzer.cpp

@@ -49,6 +50,11 @@ void StaticAnalyzer::endVisit(ContractDefinition const&)
 bool StaticAnalyzer::visit(FunctionDefinition const& _function)
 {
 	m_nonPayablePublic = _function.isPublic() && !_function.isPayable();
+
+	for (Declaration const* declaration: m_globals)


@chriseth any idea why here m_globals.size() == 0, while in the constructor it is still 18?

Maybe it was only a problem on my computer, it seems to work on AppVeyor.

federicobond · 2017-02-02T15:52:36Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

@@ -93,7 +93,7 @@ parseAnalyseAndReturnError(string const& _source, bool _reportWarnings = false,
 				}
 		if (success)
 		{
-			StaticAnalyzer staticAnalyzer(errors);
+			StaticAnalyzer staticAnalyzer(globalContext->declartions(), errors);


declarations

Thanks, there's still that other issue, at least on my computer.

axic · 2017-02-02T23:27:12Z

Status: tests fail on Linux, but succeed on Windows.

axic · 2017-02-06T15:07:36Z

No idea why the original vector<Declarations> didn't work on linux/mac, replaced with a vector<string> and now including this and super. Note: we could use the currentThis() and currentSuper() but that seems like an overkill.

chriseth · 2017-02-07T15:00:54Z

Did you consider adding this code into NameAndTypeResolver.cpp? While perhaps conceptually less clean, it is much less error prone to add it there. We have the scopes, see what is visible where and so on. If we keep it as it is now and add a new declaration, we have to think about also adding it to the visitor here. As an example, you forgot to check for names of events :-)
Oh and what about import x as msg from "y.sol";?

axic · 2017-02-07T15:48:37Z

Fixed the event and import case.

While perhaps conceptually less clean, it is much less error prone to add it there.

@chriseth Why should StaticAnalyzer exists in that case? :)

chriseth · 2017-02-07T16:59:09Z

@axic it is not about static analyzer. The main design problem is that the scopes are not persisted in the AST annotations.

axic · 2017-02-08T14:14:04Z

I'm not sure what is going on, for me it works on mac and linux (ubuntu), but travis fails at without any message:

Compiling ./scripts/../test/../std/StandardToken.sol...

axic · 2017-02-08T14:35:17Z

The main design problem is that the scopes are not persisted in the AST annotations.

I think the scope issue which could affect this is members of a struct.

axic · 2017-02-08T16:33:32Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

+			}
+		}
+	)";
+	CHECK_WARNING(text, "Shadowing builtin symbol");


Typo, this should be successful.

chriseth · 2017-02-08T16:34:42Z

I think it is better to add this change to NameAndTypeResolver.cpp, basically to anything that callsDeclarationContainer::registerDeclaration(). It would be good to perhaps add it to that function itself, but it does not have access to the error list. At that point, we could also check for any shadowing. One thing we have to check there is whether overriding a function is implemented as shadowing.

chriseth · 2017-02-08T16:36:11Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

+BOOST_AUTO_TEST_CASE(shadowing_builtins_with_imports)
+{
+	char const* text = R"(
+	        import * as msg from "B.sol";


Perhaps also add something like import {msg, block} as X from "B.sol" - that should also be fine.

Seems like that notation is broken currently:

Error: Expected "from". import {msg, block} as X from "B.sol"

Parser has this comment:

// import "abc" [as x]; // import * as x from "abc"; // import {a as b, c} from "abc";

import {msg, block} from "B.sol"; works.

Ah right. I meant import * as X from "B.sol".

axic · 2017-03-15T22:45:38Z

Rebased for good measure.

axic · 2017-05-03T18:45:03Z

@roadriverrail would you be interested in looking into this?

roadriverrail · 2017-05-03T19:39:46Z

@axic Yes, I'll look into this soon.

roadriverrail · 2017-06-03T12:13:45Z

Similarly, I've gone ahead and done a rebase of this one, but I can't push to the repository. Once we get that sorted, I can take on the comments @chriseth left.

roadriverrail · 2017-06-28T14:17:30Z

Just a quick ping before the weekly meeting...I had a rebase of this set up a couple weeks ago so that I could finish the work on it, but I can't push to the main solidity repo on this branch. If someone can please help me out with this, I can get this done. @chriseth @axic

chriseth · 2017-06-30T14:42:15Z

@roadriverrail what is your progress on this? I would like to include this feature in the next version which can hopefully happen on monday.

chriseth · 2017-07-12T19:04:13Z

We cannot really test imports inside this testing framework.

axic · 2017-07-18T21:00:22Z

@chriseth is this ready? Needs to be rebased.

chriseth · 2017-07-25T14:33:10Z

Yes, this is ready.

axic · 2017-07-26T14:01:45Z

@chriseth you need to approve and merge

axic added the in progress label Feb 2, 2017

axic commented Feb 2, 2017

View reviewed changes

axic requested a review from chriseth February 2, 2017 13:27

axic force-pushed the warn-shadowing-globals branch from cc0f4e0 to bdd5614 Compare February 2, 2017 13:43

federicobond reviewed Feb 2, 2017

View reviewed changes

axic force-pushed the warn-shadowing-globals branch from bdd5614 to d287927 Compare February 2, 2017 20:08

axic force-pushed the warn-shadowing-globals branch from befa226 to 14a69d7 Compare February 6, 2017 15:07

axic force-pushed the warn-shadowing-globals branch from 14a69d7 to 9942714 Compare February 7, 2017 15:47

axic force-pushed the warn-shadowing-globals branch from 429d303 to e0dba9d Compare February 7, 2017 16:50

axic force-pushed the warn-shadowing-globals branch from fd13d00 to 21e2cdb Compare February 8, 2017 14:28

axic commented Feb 8, 2017

View reviewed changes

chriseth reviewed Feb 8, 2017

View reviewed changes

axic force-pushed the warn-shadowing-globals branch 3 times, most recently from ac5e613 to 72fea3b Compare February 11, 2017 02:54

axic changed the title ~~Warn if shadowing built-ins~~ [WIP] Warn if shadowing built-ins Feb 12, 2017

axic force-pushed the warn-shadowing-globals branch from 72fea3b to 9c39439 Compare February 14, 2017 12:53

axic force-pushed the warn-shadowing-globals branch from 9c39439 to 4456984 Compare March 15, 2017 22:45

axic force-pushed the warn-shadowing-globals branch from 4456984 to 5d199cf Compare May 3, 2017 19:11

axic force-pushed the warn-shadowing-globals branch from 5d199cf to ef47139 Compare May 23, 2017 09:52

chriseth added the nextrelease label Jun 30, 2017

chriseth force-pushed the warn-shadowing-globals branch from ef47139 to d273111 Compare July 12, 2017 19:03

chriseth force-pushed the warn-shadowing-globals branch from aec796f to 225768b Compare July 13, 2017 12:55

axic changed the title ~~[WIP] Warn if shadowing built-ins~~ Warn if shadowing built-ins Jul 18, 2017

Warn about shadowing variables.

e0dc74b

chriseth force-pushed the warn-shadowing-globals branch from 225768b to e0dc74b Compare July 25, 2017 14:32

chriseth merged commit 925569b into develop Jul 26, 2017

chriseth removed in progress labels Jul 26, 2017

axic deleted the warn-shadowing-globals branch July 26, 2017 15:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn if shadowing built-ins #1637

Warn if shadowing built-ins #1637

axic commented Feb 2, 2017 •

edited

Loading

axic Feb 2, 2017

chriseth Feb 2, 2017

axic Feb 2, 2017 •

edited

Loading

axic Feb 2, 2017

axic Feb 2, 2017

axic Feb 2, 2017

federicobond Feb 2, 2017

axic Feb 2, 2017

axic commented Feb 2, 2017

axic commented Feb 6, 2017

chriseth commented Feb 7, 2017

axic commented Feb 7, 2017 •

edited

Loading

chriseth commented Feb 7, 2017

axic commented Feb 8, 2017 •

edited

Loading

axic commented Feb 8, 2017

axic Feb 8, 2017

chriseth commented Feb 8, 2017

chriseth Feb 8, 2017

axic Feb 10, 2017

axic Feb 10, 2017

chriseth Feb 16, 2017

axic commented Mar 15, 2017

axic commented May 3, 2017

roadriverrail commented May 3, 2017

roadriverrail commented Jun 3, 2017

roadriverrail commented Jun 28, 2017

chriseth commented Jun 30, 2017

chriseth commented Jul 12, 2017

axic commented Jul 18, 2017

chriseth commented Jul 25, 2017

axic commented Jul 26, 2017

Warn if shadowing built-ins #1637

Warn if shadowing built-ins #1637

Conversation

axic commented Feb 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic Feb 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic commented Feb 2, 2017

axic commented Feb 6, 2017

chriseth commented Feb 7, 2017

axic commented Feb 7, 2017 • edited Loading

chriseth commented Feb 7, 2017

axic commented Feb 8, 2017 • edited Loading

axic commented Feb 8, 2017

Choose a reason for hiding this comment

chriseth commented Feb 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic commented Mar 15, 2017

axic commented May 3, 2017

roadriverrail commented May 3, 2017

roadriverrail commented Jun 3, 2017

roadriverrail commented Jun 28, 2017

chriseth commented Jun 30, 2017

chriseth commented Jul 12, 2017

axic commented Jul 18, 2017

chriseth commented Jul 25, 2017

axic commented Jul 26, 2017

axic commented Feb 2, 2017 •

edited

Loading

axic Feb 2, 2017 •

edited

Loading

axic commented Feb 7, 2017 •

edited

Loading

axic commented Feb 8, 2017 •

edited

Loading