Skip to content
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

Use levenshtein distance for better error messages #2806

Merged
merged 11 commits into from
Apr 17, 2022

Conversation

tserg
Copy link
Collaborator

@tserg tserg commented Apr 17, 2022

What I did

Fix for #2767

How I did it

Refer to Brownie and docopt-ng. Helper function is extracted from the source at docopt-ng.

How to verify it

Commit message

Use Levenshtein distance for better error messages

Description for the changelog

Use Levenshtein distance for better error messages

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@codecov-commenter
Copy link

codecov-commenter commented Apr 17, 2022

Codecov Report

Merging #2806 (7e8f9ff) into master (47cbc8e) will decrease coverage by 30.37%.
The diff coverage is 23.80%.

@@             Coverage Diff             @@
##           master    #2806       +/-   ##
===========================================
- Coverage   87.46%   57.09%   -30.38%     
===========================================
  Files          93       94        +1     
  Lines        9960     9994       +34     
  Branches     2471     2431       -40     
===========================================
- Hits         8712     5706     -3006     
- Misses        782     3679     +2897     
- Partials      466      609      +143     
Impacted Files Coverage Δ
vyper/semantics/validation/levenshtein_utils.py 16.00% <16.00%> (ø)
vyper/semantics/namespace.py 71.21% <33.33%> (-25.67%) ⬇️
vyper/semantics/types/bases.py 60.82% <33.33%> (-23.03%) ⬇️
vyper/semantics/types/user/struct.py 42.85% <33.33%> (-39.81%) ⬇️
vyper/semantics/types/utils.py 43.33% <33.33%> (-44.81%) ⬇️
vyper/semantics/validation/module.py 46.63% <33.33%> (-40.35%) ⬇️
vyper/semantics/validation/utils.py 50.86% <50.00%> (-40.36%) ⬇️
vyper/builtin_interfaces/ERC165.py 0.00% <0.00%> (-100.00%) ⬇️
vyper/builtin_interfaces/ERC20Detailed.py 0.00% <0.00%> (-100.00%) ⬇️
vyper/ir/s_expressions.py 5.88% <0.00%> (-88.24%) ⬇️
... and 63 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 47cbc8e...7e8f9ff. Read the comment docs.

@@ -7,6 +7,7 @@
StructureException,
UndeclaredDefinition,
)
from vyper.utils import get_levenshtein_string
Copy link
Member

@charles-cooper charles-cooper Apr 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should a) rename this to get_levenshtein_error_suggestions and b) move all the levenshtein utils to their own module. i think vyper/semantics/validation/levenshtein_utils.py makes sense.

vyper/utils.py Outdated

distances = sorted([(i, levenshtein_norm(key, i)) for i in namespace], key=lambda k: k[1])
if len(distances) > 0 and distances[0][1] <= threshold:
return f" Did you mean '{distances[0][0]}'?"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think the initial space is required -- the caller should be responsible for this.

@@ -151,8 +152,11 @@ def types_from_Attribute(self, node):
f"'{name}' is not a storage variable, it should not be prepended with self",
node,
) from None

levenshtein_string = get_levenshtein_string(name, var.members, 0.4)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think we need to name the variable levenshtein_string, which is a bit arcane. how about suggestions_str?

Suggested change
levenshtein_string = get_levenshtein_string(name, var.members, 0.4)
suggestions_str = get_levenshtein_error_string(name, var.members, 0.4)

Copy link
Member

@charles-cooper charles-cooper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

functionality looks good, just a few nits

@charles-cooper charles-cooper marked this pull request as ready for review April 17, 2022 07:55
distances = sorted([(i, levenshtein_norm(key, i)) for i in namespace], key=lambda k: k[1])
min_levenshtein_val = distances[0][1]
if len(distances) > 0 and min_levenshtein_val <= threshold:
if distances[1][1] == min_levenshtein_val:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't work if there is only one possible suggestion. e.g.

(vyper) ~/vyper $ cat tmp/foo.vy
interface Foo:
    def foo(x: uint256, y: uint256 = 1): nonpayable

@external
def foo():
    Foo(msg.sender).fooo(1)


distances = sorted([(i, levenshtein_norm(key, i)) for i in namespace], key=lambda k: k[1])
if len(distances) > 0 and distances[0][1] <= threshold:
if len(distances) >= 2 and distances[0][1] == distances[1][1]:
Copy link
Member

@charles-cooper charles-cooper Apr 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we do not need to check that the top two distances are equal, instead we only require that distances[1][1] is also <= threshold.


distances = sorted([(i, levenshtein_norm(key, i)) for i in namespace], key=lambda k: k[1])
if len(distances) > 0 and distances[0][1] <= threshold:
if len(distances) >= 2 and distances[0][1] == distances[1][1]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also a nit -- for consistency with the above branch, use >

Suggested change
if len(distances) >= 2 and distances[0][1] == distances[1][1]:
if len(distances) > 1 and distances[0][1] == distances[1][1]:

Co-authored-by: Charles Cooper <cooper.charles.m@gmail.com>
@charles-cooper charles-cooper enabled auto-merge (squash) April 17, 2022 09:07
@charles-cooper charles-cooper merged commit 857bc86 into vyperlang:master Apr 17, 2022
@tserg tserg deleted the feat/better_error_msg branch April 17, 2022 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants