Skip to content

Parse Infinity and NaN as numbers #257

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

Closed

Conversation

guillaumebrunerie
Copy link
Contributor

This change parses the tokens Infinity and NaN as numbers.
Technically, they are read-only global variables, but just like for undefined, I think it makes sense to parse and highlight Infinity and NaN as numbers instead of as identifiers.

Checklist:

  • All tests pass in CI.
  • The script/parse-examples passes without issues.
  • There are sufficient tests for the new fix/feature.
  • Grammar rules have not been renamed unless absolutely necessary.
  • The conflicts section hasn't grown too much.
  • The parser size hasn't grown too much (check the value of STATE_COUNT in src/parser.c).

@amaanq amaanq requested review from dcreager and aryx July 11, 2023 23:27
@maxbrunsfeld
Copy link
Contributor

maxbrunsfeld commented Jul 11, 2023

Let's not add these to the grammar. They aren't special syntax in JavaScript; they're just built-in global variables. There is a cost to every special keyword added to the grammar. For syntax highlighting, these can be recognized via string-matching predicates.

@guillaumebrunerie
Copy link
Contributor Author

Should the special rule for undefined be removed, then?

// 'undefined' is syntactically a regular identifier in JavaScript.
// However, its main use is as the read-only global variable whose
// value is [undefined], for which there's no literal representation
// unlike 'null'. We gave it its own rule so it's easy to
// highlight in text editors and other applications.
_identifier: $ => choice(
$.undefined,
$.identifier
),

@maxbrunsfeld
Copy link
Contributor

I'd be fine with that, except that a lot of existing queries may need to be updated.

@aryx aryx closed this Jul 17, 2023
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.

4 participants