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

[Clarity] Escaped backslash in strings may fail parsing #3123

Closed
njordhov opened this issue May 1, 2022 · 7 comments
Closed

[Clarity] Escaped backslash in strings may fail parsing #3123

njordhov opened this issue May 1, 2022 · 7 comments

Comments

@njordhov
Copy link
Contributor

njordhov commented May 1, 2022

Describe the bug

Parsing a string with escaped backslash \\ sometimes fails with error.

Steps To Reproduce

Evaluate in the Clarity repl:
(list "\\" "x")
=> error: Expected whitespace or a close parens. Found: 'x'

A contract with the code above also fails to deploy on testnet.

Expected behavior

Strings with an escaped backslash should parse correctly.

Note that the expression parses if the "x" is removed. Evaluate in the Clarity repl:

(list "\\")
=> ["\\"]

Environment (please complete the following information):

Confirmed in the latest Clarity REPL and by deploying to Testnet.

@obycode
Copy link
Contributor

obycode commented May 2, 2022

I can dig into this one. Upon first inspection, it looks like the v2 parser in use by the REPL handles it, but the v1 parser (the canonical parser) does not, which would mean it would need to be fixed along with other consensus breaking changes in 2.1. I will take a closer look and verify that later today.

@obycode
Copy link
Contributor

obycode commented May 2, 2022

The problem is in this regular expression from vm/ast/parser/mod.rs:140:

        LexMatcher::new(
            r##""(?P<value>((\\")|([[ -~]&&[^"]]))*)""##,
            TokenType::StringASCIILiteral,
        ),

For the ascii string in the example, (list "\\" "x"), the first token in the list from the lexer is the string "\\\" ".

We have the improved parser v2 in the REPL which handles this correctly. 2.1 seems like a good time to switch the canonical VM over to use this new parser. Should I begin putting together a PR for that?

obycode added a commit that referenced this issue May 25, 2022
Use of the new parser is gated on epoch 2.1 and the old parser is used
prior to then.

Resolves: #3124, #3123, #3128
obycode added a commit that referenced this issue May 25, 2022
Use of the new parser is gated on epoch 2.1 and the old parser is used
prior to then.

Resolves: #3124, #3123, #3128
obycode added a commit that referenced this issue May 25, 2022
Use of the new parser is gated on epoch 2.1 and the old parser is used
prior to then.

Resolves: #3124, #3123, #3128
obycode added a commit that referenced this issue May 25, 2022
Use of the new parser is gated on epoch 2.1 and the old parser is used
prior to then.

Resolves: #3124, #3123, #3128
obycode added a commit that referenced this issue May 27, 2022
Use of the new parser is gated on epoch 2.1 and the old parser is used
prior to then.

Resolves: #3124, #3123, #3128
obycode added a commit that referenced this issue May 27, 2022
Use of the new parser is gated on epoch 2.1 and the old parser is used
prior to then.

Resolves: #3124, #3123, #3128
obycode added a commit that referenced this issue May 27, 2022
Use of the new parser is gated on epoch 2.1 and the old parser is used
prior to then.

Resolves: #3124, #3123, #3128
obycode added a commit that referenced this issue May 31, 2022
Use of the new parser is gated on epoch 2.1 and the old parser is used
prior to then.

Resolves: #3124, #3123, #3128
obycode added a commit that referenced this issue Jun 13, 2022
Use of the new parser is gated on epoch 2.1 and the old parser is used
prior to then.

Resolves: #3124, #3123, #3128
@jcnelson
Copy link
Member

jcnelson commented Aug 3, 2022

Closing as complete if this was fixed in the new v2 parser. Please reopen if not.

@jcnelson jcnelson closed this as completed Aug 3, 2022
@njordhov
Copy link
Contributor Author

njordhov commented Aug 4, 2022

@jcnelson Is there a unit test to validate it has been fixed for the v2 parser?

@obycode
Copy link
Contributor

obycode commented Aug 4, 2022

No, I don't think there is. You're right, we should add tests for these.

@njordhov
Copy link
Contributor Author

njordhov commented Aug 4, 2022

There should also be tests for related cases that parses but shouldn't, such as:

(list "\\" x")

@blockstack-devops
Copy link
Contributor

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Nov 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants