Suggest alternatives when identifier not found. Closes #3058. by Balajiganapathi · Pull Request #3147 · argotorg/solidity

Balajiganapathi · 2017-10-29T08:12:06Z

We can use the following method to compute alternatives:

When an identifier is not found, look through current and enclosing scopes and compare each declaration name with the identifier name
To decide whether two names are similar, calculate the Damerau–Levenshtein distance. This metric considers insertion, deletion, substitution and transposition as single operations
If the DL distance is <= 2 and it is less than the lengths of the name they can be considered similar.

axic · 2017-10-29T14:46:32Z

@Balajiganapathi thanks these are great! Will do an individual review, but two small mentions (applies to all PRs): no need to use namespace specifier std as it is imported and please add an entry to the changelog in every case a bug is fixed (no need to include bug issue number) or a feature is added.

axic · 2017-10-29T14:51:04Z

Please include a test for inheritance (suggested name in the inheritance tree).

axic · 2017-10-29T14:51:53Z

libsolidity/analysis/DeclarationContainer.cpp

+	if (m_enclosingContainer)
+	{
+		std::vector<ASTString> enclosingSimilar = m_enclosingContainer->similarNames(_name);
+		similar.insert(similar.end(), enclosingSimilar.begin(), enclosingSimilar.end());


I wonder which is faster. The current way or doing the parent first and the push_front to extend that vector?

I think this is reasonably fast. push_front on vector is quite slow (though we can use list/deque to get around that). Inserting at end like here is efficient.

It can be made slightly faster by avoiding creation of multiple vectors. We can create a single vector and pass it by reference as a parameter to the function. Or we can use a list and use splice to append at end. But I am not sure it will result in any substantial gain.

PS: Thanks for the tips :) will remove std:: and add change log to all my PRs.

axic · 2017-10-29T15:29:28Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

+	CHECK_ERROR(sourceCode, DeclarationError, "Undeclared identifier.");
+
+	sourceCode = R"(
+			contract c {


Indentation on these looks a bit off (one extra level of indentation).

federicobond · 2017-10-30T03:25:40Z

Looks really good! Great work @Balajiganapathi

axic · 2017-10-30T12:09:53Z

@Balajiganapathi just a heads up: we'll be busy with devcon this week and it is likely that your changes won't be properly addressed this week.

Balajiganapathi · 2017-11-11T10:25:11Z

@axic I have made the suggested changes to this (and my 3 other) PRs. Waiting for these PRs to be reviewed and merged before taking up more tasks as I don't want to keep repeating the same mistakes :)

axic · 2017-11-11T12:31:04Z

libsolidity/analysis/DeclarationContainer.cpp

+	return similar;
+}
+
+bool DeclarationContainer::areSimilarNames(ASTString const& name1, ASTString const& name2)


I would move this to libdevcore and name it something like stringWithinDistance(name1, name2, distance). It can be later reused for showing potential names for visibility specifiers, etc.

axic · 2017-11-11T12:31:47Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

+			function g() public { fun(); }
+		}
+	)";
+	CHECK_ERROR(sourceCode, DeclarationError, "Undeclared identifier. Did you mean func?");


Can you enclose the suggestions in quotes - that is how other messages are? eg "func", "func1", "func2" or "func3"

axic · 2017-11-17T15:35:36Z

@Balajiganapathi will you have some time before next week to check these changes out or should I push changes to this branch?

Balajiganapathi · 2017-11-17T15:54:09Z

@axic had a busy week so could not work on it. Will make these changes over this weekend.
Also, can you tell me where exactly to put the stringWithinDistance in libdevcore? I am not able to decide.

Balajiganapathi · 2017-11-17T16:22:37Z

None of the existing files in libdevcore seems to fit. Shall I make a new StringUtils source file for this?

axic · 2017-11-17T23:55:00Z

Shall I make a new StringUtils source file for this?

I think it sounds fine, could be more specific too if you find a good name (something along the lines of string similarity).

Please add a specific test for the distance module in test/libdevcore (just name the file the same as the implementation file name).

Balajiganapathi · 2017-11-18T07:16:42Z

Done @axic

axic · 2017-11-21T11:02:21Z

libsolidity/analysis/DeclarationContainer.h

 	std::map<ASTString, std::vector<Declaration const*>> m_declarations;
 	std::map<ASTString, std::vector<Declaration const*>> m_invisibleDeclarations;
+
+	static size_t const MAXIMUM_DISTANCE = 2;


Not fully sold we should have this constant here, but if we do the name should reflect what it does.

I agree, it should just be a constant inside the similarNames function.

axic · 2017-11-21T11:05:24Z

Rebased and made some tiny updates to the branch.

chriseth · 2017-11-21T11:07:47Z

libdevcore/StringUtils.cpp

+using namespace std;
+using namespace dev;
+
+namespace dev


Style: Please don't use namespace regions in cpp files.

chriseth · 2017-11-21T11:11:55Z

libdevcore/StringUtils.cpp

+	size_t n1 = _name1.size(), n2 = _name2.size();
+	vector<vector<size_t>> dp(n1 + 1, vector<size_t>(n2 + 1));
+
+	// In this dp formulation of Damerau–Levenshtein distance we are assuming that the strings are 1-based to make base case storage easier.


This seems rather complicated compared to other implementations: https://en.wikibooks.org/wiki/Algorithm_Implementation/Strings/Levenshtein_distance#C.2B.2B

Especially the two-dimensional vector could be rather heap-heavy, especially given that this routine will be called on every identifier that is in scope.

What is the license on the wikibook examples?

@chriseth the linked example does not consider transpositions. e.g. for "test", "tset" would be considered as distance 2 (i.e. 2 substitution). I think this is a common typo so we should consider these as 1 distance.

"Text is available under the Creative Commons Attribution-ShareAlike License.; additional terms may apply. By using this site, you agree to the Terms of Use and Privacy Policy."

but I'm not sure copyright and licensing actually applies to such small snippets.

@chriseth @axic

About the implementation, the implementation currently uses O(n1.n2) space. I will optimize it to O(n1) (or O(n2)) space. It will be different from the one in the wiki link as we will be accessing previous 2 entries. So will need a vector of size (3 * n).

I can also think of another (but ugly) optimization to avoid creating a vector of 3.n1 everytime. Fix the unidentified identifier as s1, so n1 will be same over all calls to the function. So, we can create a 3 * n1 vector once and pass it to the function.
I think we should do the 1st optimization but avoid the 2nd as it would affect generalization of the string distance function.

PS: I will do this and all the other small changes suggested here in a few hours.

chriseth · 2017-11-21T11:16:08Z

libdevcore/StringUtils.cpp

+		}
+	}
+
+	size_t distance = dp[n1][n2];


Could you factor out the distance calculation into its own function?

Yes that is a good idea. I will:

Only keep a distance function in StringUtils and remove this one.

Compare the distance with MIN_EDIT_DISTANCE constant that will be declared in similarNames function.

chriseth · 2017-11-21T11:19:45Z

libsolidity/analysis/DeclarationContainer.cpp

+
+	if (m_enclosingContainer)
+	{
+		vector<ASTString> enclosingSimilar = m_enclosingContainer->similarNames(_name);


We have an operator overload that allows similar += m_enclosingContainer->similarNames(_name);

chriseth · 2017-11-21T11:22:49Z

libsolidity/analysis/NameAndTypeResolver.cpp

 	return result;
 }

+string NameAndTypeResolver::similarNameSuggestions(ASTString const& _name) const


This could be a function in StringUtils called something like quotedAlternativesList.

axic · 2017-11-22T04:18:09Z

libsolidity/analysis/NameAndTypeResolver.cpp


+string NameAndTypeResolver::similarNameSuggestions(ASTString const& _name) const
+{
+    return quotedAlternativesList(m_currentScope->similarNames(_name));


Since this has been split into utils I think similarNameSuggestions should only return a vector and the formatting of the contents should be in ReferencesResolver.

Please move the quotedAlternativeList part to the error message in ReferencesResolver. This method could be also placed into the header since it is a single line (though it is not necessary).

Indentation.

chriseth · 2017-11-23T18:05:00Z

libsolidity/analysis/ReferencesResolver.cpp

 	if (declarations.empty())
 	{
-		string const& suggestions = m_resolver.similarNameSuggestions(_identifier.name());
+		string const& suggestions = quotedAlternativesList(m_resolver.similarNameSuggestions(_identifier.name()));


You cannot store this as a const&. I actually wonder why the compiler does not complain here...

chriseth · 2017-11-23T18:05:22Z

Please use rebasing instead of merging, since it is impossible to review a merge commit.

axic · 2017-12-11T12:58:15Z

Rebased for review.

axic · 2018-02-09T15:12:27Z

@chriseth oh I think actually this was waiting for you to review. Can you review the updates? It shouldn't be too hard to finish this one.

chriseth · 2018-02-09T15:23:37Z

libdevcore/StringUtils.cpp

+
+size_t dev::stringDistance(string const& _str1, string const& _str2)
+{
+	size_t n1 = _str1.size(), n2 = _str2.size();


only one declaration per line.

chriseth · 2018-02-09T15:24:28Z

Fine part from minor things. I will rebase and fix those.

chriseth · 2018-02-09T17:10:24Z

@axic if tests are green, you can merge at will

chriseth · 2018-02-13T12:39:20Z

Circle somehow fails since it cannot fetch the commit. Since travis is green, I would ignore it.

axic · 2018-02-13T15:07:29Z

Circle somehow fails since it cannot fetch the commit. Since travis is green, I would ignore it.

I guess building from external pull requests doesn't properly work with circleci

Balajiganapathi changed the title ~~Suggest alternatives when identifier not found. For #3058.~~ Suggest alternatives when identifier not found. Closes #3058. Oct 29, 2017

axic reviewed Oct 29, 2017

View reviewed changes

Balajiganapathi force-pushed the alternative_scope branch from 798084b to f97a754 Compare November 11, 2017 10:21

axic reviewed Nov 11, 2017

View reviewed changes

Balajiganapathi force-pushed the alternative_scope branch from 2c2c7b0 to 8b020c1 Compare November 17, 2017 17:25

axic force-pushed the alternative_scope branch 4 times, most recently from 960e853 to 22c7b6c Compare November 21, 2017 11:01

axic reviewed Nov 21, 2017

View reviewed changes

axic force-pushed the alternative_scope branch from 22c7b6c to a32ad20 Compare November 21, 2017 11:04

axic force-pushed the alternative_scope branch from a32ad20 to bc799b1 Compare November 21, 2017 11:06

chriseth reviewed Nov 21, 2017

View reviewed changes

axic reviewed Nov 22, 2017

View reviewed changes

chriseth reviewed Nov 23, 2017

View reviewed changes

Balajiganapathi force-pushed the alternative_scope branch from bded3e6 to 619c5e2 Compare November 28, 2017 03:16

axic force-pushed the alternative_scope branch from 619c5e2 to e3ac2dd Compare December 11, 2017 12:52

axic added the nextrelease label Dec 12, 2017

chriseth reviewed Feb 9, 2018

View reviewed changes

axic assigned chriseth Feb 9, 2018

chriseth force-pushed the alternative_scope branch from e3ac2dd to 4ecdc51 Compare February 9, 2018 17:00

chriseth force-pushed the alternative_scope branch 2 times, most recently from a2d93ce to 8df768d Compare February 13, 2018 11:50

Balajiganapathi added 4 commits February 13, 2018 14:54

Suggest alternatives when identifier not found.

2859834

Move string distance function to utils and format error message

b1417b3

Add tests for similarity routine

d123e77

Restructure code for alternative identifier suggestions

8a491c7

axic force-pushed the alternative_scope branch from 8df768d to 31c3dd1 Compare February 13, 2018 15:01

chriseth added 4 commits February 13, 2018 15:04

Minor changes.

dc0a25f

Suggestion to improve readability.

12c3eb8

Use one-dimensional vector.

5747985

Fix: remove reference.

1dcd7c5

axic force-pushed the alternative_scope branch from 31c3dd1 to 1dcd7c5 Compare February 13, 2018 15:04

axic approved these changes Feb 13, 2018

View reviewed changes

chriseth merged commit f84b2c4 into argotorg:develop Feb 13, 2018

LianaHus removed the nextrelease label Feb 13, 2018

Conversation

Balajiganapathi commented Oct 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

axic commented Oct 29, 2017

Uh oh!

axic commented Oct 29, 2017

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!

federicobond commented Oct 30, 2017

Uh oh!

axic commented Oct 30, 2017

Uh oh!

Balajiganapathi commented Nov 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic commented Nov 17, 2017

Uh oh!

Balajiganapathi commented Nov 17, 2017

Uh oh!

Balajiganapathi commented Nov 17, 2017

Uh oh!

axic commented Nov 17, 2017

Uh oh!

Balajiganapathi commented Nov 18, 2017

Uh oh!

axic Nov 21, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

axic commented Nov 21, 2017

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!

chriseth commented Nov 23, 2017

Uh oh!

axic commented Dec 11, 2017

Uh oh!

axic commented Feb 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Balajiganapathi commented Oct 29, 2017 •

edited

Loading

axic Nov 21, 2017 •

edited

Loading