rust-bakery / nom

Rust parser combinator framework
MIT License
9.18k stars 792 forks source link

fix: parse "infinity" literal fully in `float` #1673

Closed boxdot closed 8 months ago

boxdot commented 1 year ago

float first parsed the "inf" literal and then "infinity", therefore even though the latter was parsed correctly, the suffix "inity" was returned as remaining input, which is not correct.

epage commented 1 year ago

Not the maintainer but I'm assuming the intent here is to parse the keywords inf, similar to nan. infinity isn't generally a keyword. The reason inity is being left is because nom-style parsers leave all unrecognized content for the next parser to identify or error on.

For a language when an invalid keyword is used often enough, it can make sense to special case the error situation into the grammar (match on infinity but error).

boxdot commented 1 year ago

infinity isn't generally a keyword

I forgot to mention that nom 6 parsed infinity as float infinity without remaining input. The current code also contains a case and tests for parsing infinity completely. So, I assume it is a regression in nom 7.

My motivation is actually this upgrade: https://github.com/Garvys/rustfst/pull/240

epage commented 1 year ago

Huh, didn't realize that FromStr accepts infinity. FromStr and lexical_core were the nom v6 implementations of float. In nom v7, an attempt was made to switch to minimal_lexical (baf5e01) but that had problems (f8633f9). Whats confusing is I'm not seeing how the minimal_lexical version was meant to handle nan, inf, and infinity.

Geal commented 8 months ago

right, the order was wrong here, nice catch

coveralls commented 8 months ago

Pull Request Test Coverage Report for Build 6597520943


Changes Missing Coverage Covered Lines Changed/Added Lines %
src/number/mod.rs 0 2 0.0%
<!-- Total: 4 6 66.67% -->
Totals Coverage Status
Change from base Build 6597500402: 0.0%
Covered Lines: 1828
Relevant Lines: 2929

💛 - Coveralls
Geal commented 8 months ago

thank you!