New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggest alternatives when identifier not found. Closes #3058. #3147

Merged
merged 8 commits into from Feb 13, 2018

Conversation

Projects
None yet
5 participants
@Balajiganapathi
Contributor

Balajiganapathi commented Oct 29, 2017

We can use the following method to compute alternatives:

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

For #3058.

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

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Oct 29, 2017

Member

@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.

Member

axic commented Oct 29, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@axic

axic Oct 29, 2017

Member

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

Member

axic commented Oct 29, 2017

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

@federicobond

This comment has been minimized.

Show comment
Hide comment
@federicobond

federicobond Oct 30, 2017

Contributor

Looks really good! Great work @Balajiganapathi

Contributor

federicobond commented Oct 30, 2017

Looks really good! Great work @Balajiganapathi

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Oct 30, 2017

Member

@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.

Member

axic commented Oct 30, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@Balajiganapathi

Balajiganapathi Nov 11, 2017

Contributor

@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 :)

Contributor

Balajiganapathi commented Nov 11, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@axic

axic Nov 17, 2017

Member

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

Member

axic commented Nov 17, 2017

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

@Balajiganapathi

This comment has been minimized.

Show comment
Hide comment
@Balajiganapathi

Balajiganapathi Nov 17, 2017

Contributor

@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.

Contributor

Balajiganapathi commented Nov 17, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@Balajiganapathi

Balajiganapathi Nov 17, 2017

Contributor

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

Contributor

Balajiganapathi commented Nov 17, 2017

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

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Nov 17, 2017

Member

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).

Member

axic commented Nov 17, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Balajiganapathi
Contributor

Balajiganapathi commented Nov 18, 2017

Done @axic

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Nov 21, 2017

Member

Rebased and made some tiny updates to the branch.

Member

axic commented Nov 21, 2017

Rebased and made some tiny updates to the branch.

@@ -425,6 +425,23 @@ vector<_T const*> NameAndTypeResolver::cThreeMerge(list<list<_T const*>>& _toMer
return result;
}
string NameAndTypeResolver::similarNameSuggestions(ASTString const& _name) const

This comment has been minimized.

@chriseth

chriseth Nov 21, 2017

Contributor

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

@chriseth

chriseth Nov 21, 2017

Contributor

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

This comment has been minimized.

@axic

axic Dec 12, 2017

Member

Done.

@axic

axic Dec 12, 2017

Member

Done.

BOOST_CHECK_EQUAL(stringWithinDistance("hello", "hllllo", 1), false);
BOOST_CHECK_EQUAL(stringWithinDistance("hello", "hllllo", 2), true);
BOOST_CHECK_EQUAL(stringWithinDistance("hello", "hlllo", 2), true);
BOOST_CHECK_EQUAL(stringWithinDistance("a", "", 2), false);

This comment has been minimized.

@chriseth

chriseth Nov 21, 2017

Contributor

Shouldn't this be distance 1?

@chriseth

chriseth Nov 21, 2017

Contributor

Shouldn't this be distance 1?

BOOST_CHECK_EQUAL(stringWithinDistance("abc", "ba", 2), false);
BOOST_CHECK_EQUAL(stringWithinDistance("abc", "abcdef", 2), false);
BOOST_CHECK_EQUAL(stringWithinDistance("abcd", "wxyz", 2), false);
BOOST_CHECK_EQUAL(stringWithinDistance("", "", 2), true);

This comment has been minimized.

@chriseth

chriseth Nov 21, 2017

Contributor

Shouldn't this be distance 0?

@chriseth

chriseth Nov 21, 2017

Contributor

Shouldn't this be distance 0?

This comment has been minimized.

@axic

axic Nov 22, 2017

Member

This method checks if it is distance <= n: 0 <= 2.

@axic

axic Nov 22, 2017

Member

This method checks if it is distance <= n: 0 <= 2.

BOOST_AUTO_TEST_CASE(test_similarity)
{
BOOST_CHECK_EQUAL(stringWithinDistance("hello", "hello", 0), true);

This comment has been minimized.

@chriseth

chriseth Nov 21, 2017

Contributor

I think the tests here would also profit greatly from a separate distance function.

@chriseth

chriseth Nov 21, 2017

Contributor

I think the tests here would also profit greatly from a separate distance function.

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

This comment has been minimized.

@chriseth

chriseth Nov 21, 2017

Contributor

Ah, it's so nice seeing this :-)

@chriseth

chriseth Nov 21, 2017

Contributor

Ah, it's so nice seeing this :-)

This comment has been minimized.

@Balajiganapathi

Balajiganapathi Nov 21, 2017

Contributor

I feel this error message might be even better?
Undeclared identifier "fun". Did you mean "func"?

@Balajiganapathi

Balajiganapathi Nov 21, 2017

Contributor

I feel this error message might be even better?
Undeclared identifier "fun". Did you mean "func"?

This comment has been minimized.

@axic

axic Nov 21, 2017

Member

We should have the source location highlighting the actual culprit (the identifier), but I think it can always help in clarity.

@axic

axic Nov 21, 2017

Member

We should have the source location highlighting the actual culprit (the identifier), but I think it can always help in clarity.

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Nov 23, 2017

Contributor

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

Contributor

chriseth commented Nov 23, 2017

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

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Dec 11, 2017

Member

Rebased for review.

Member

axic commented Dec 11, 2017

Rebased for review.

@axic axic added the nextrelease label Dec 12, 2017

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Feb 9, 2018

Member

@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.

Member

axic commented Feb 9, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Feb 9, 2018

Contributor

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

Contributor

chriseth commented Feb 9, 2018

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

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Feb 9, 2018

Contributor

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

Contributor

chriseth commented Feb 9, 2018

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

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Feb 13, 2018

Contributor

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

Contributor

chriseth commented Feb 13, 2018

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

@axic

axic approved these changes Feb 13, 2018

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Feb 13, 2018

Member

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

Member

axic commented Feb 13, 2018

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

@chriseth chriseth merged commit f84b2c4 into ethereum:develop Feb 13, 2018

2 of 4 checks passed

ci/circleci: build_emscripten Your tests failed on CircleCI
Details
ci/circleci: build_x86 Your tests failed on CircleCI
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@LianaHus LianaHus removed the nextrelease label Feb 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment