-
Notifications
You must be signed in to change notification settings - Fork 27
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
Remove three unwraps from escaped_char #73
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks!
Before this commit, escape sequences above std::char::MAX ('\u{10ffff}') would overflow and cause a panic. This commit replaces an `unwrap` with `ok_or` and a clearer error message. This message will likely change in the future in order to better conform to the `dart-sass` implementation which currently also fails to cleanly handle this overflow. See kaj/rsass#73
@divergentdave you may also want to upstream this issue to With the same input,
The relevant code for this panic is here, https://github.com/sass/dart-sass/blob/master/lib/src/parse/parser.dart#L462 It may be clearer to use |
Progress: 2234 of 5510 tests passed in dart-sass compatiblilty mode. * PR #76 and #78: Target dart-sass rather than libsass compatibilty. This sets the target when testing and changes some different behaviour, including how strings are parsed and handled. * PR #75 from @divergentdave: Parse and add `Value` variants for BigInt numbers * PR #77: Support `rgba(r g b / a)` and `hsla(h s l / a)` functions, i.e. the `channels` parameter with div-separated alpha channel. * Fix `@import` indention. * Improve function default argument parsing and dont panic in `parse_value_data`. * Handle dictionary-ellipsis style call-by-varargs for functions. * PR #73 from @divergentdave: Remove three unwraps from escaped_char * PR #71 from @connorskees: Further optimize number printing * Update travis url in README. * PR #70 from @svenstaro: Mention grass rather than sassers, as sassers appears to be dead while grass is in rather active development. * Add a CHANGELOG.md * Update spec to 2020-09-17. * Update num-rational to 0.3.0. Tested with rustc 1.46.0 (04488afe3 2020-08-24), 1.44.1 (c7087fe00 2020-06-17), 1.42.0 (b8cedc004 2020-03-09), 1.40.0 (73528e339 2019-12-16), 1.38.0 (625451e37 2019-09-23), 1.47.0-beta.7 (e28d2bd09 2020-10-01), and 1.48.0-nightly (8876ffc92 2020-10-02)
I found through fuzzing that the minimized input
\d00000
panics upon unwrapping None. This PR adds that input as a test, and refactors escaped_char() to use additional combinators instead of unwrap calls.