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

Adds no_spaces rules to selector strings in asm.pest #62

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mrferris
Copy link

@mrferris mrferris commented May 29, 2021

This pr addresses issue #47's "descriptive error for space in selector" item.

test.etk (note spaces in between parameters):

push4 selector("_burn(address, bytes32, uint256)")

eas ./test.etk output:

Caused by: lexing failed
Caused by:  --> 1:31
  |
1 | push4 selector("_burn(address, bytes32, uint256)")
  |                               ^---
  |
  = expected no_spaces_in_selector

I've validated that this solution works with spaces at any position in the selector string.

The only downside of this approach is asm.pest could potentially get bloated with every little custom parsing error that we want going forward.

Alternatively, another approach I tested was creating a new SelectorSpace error type in parse/error.rs, allow selector strings to pass through the parser with spaces in them, and then programmatically check for spaces and throw an error. However, taking this approach left me with an error message that didn't show nearly as much information:

Rule::selector => {
            let raw = operand.into_inner().next().unwrap().as_str();
            ensure!(!raw.contains(" "), error::SelectorSpaces);
           ...
}

prints:

Error: parsing failed
Caused by: selector strings may not contain any spaces

@SamWilsn
Copy link
Contributor

SamWilsn commented Jun 8, 2021

Sorry for taking so long to get back to you! I've been giving this a bit of thought, and I'm really not sure what the best approach is going forward.

My current preferred solution is combining your two approaches. Something like creating a special rule (like no_spaces_in_selector), and handling it with a new error variant in the parser (like Error::SelectorSpaces.) That way we can preserve the original error context information in source, but control the error message.

I'm not sure how feasible catching an error for a particular rule is though. Thoughts?

@mrferris
Copy link
Author

mrferris commented Jun 9, 2021

I agree with your preferred solution, and I am going to convert this to a draft while I work out catching and working with specific errors.

I've realized that this original commit has an edge case where someone accidentally includes a special character in the selector string (!@#$) and gets the same "expected no_spaces_in_selector" error message, which isn't a correct error message.

So I updated it to say "expected no_spaces_no_special_chars", (and "no_spaces_no_special_chars_no_numbers" for the first char), which is more accurate but less specific, and highlights how inflexible this approach alone is for making quality error messages. Catching this error in Rust will allow us to have a bad_char_in_selector rule, and then programmatically make the error as descriptive as possible.

@mrferris mrferris marked this pull request as draft June 9, 2021 16:24
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

2 participants