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

Add true and false as reserved words #656

Merged
merged 1 commit into from
Oct 18, 2021
Merged

Conversation

pushkine
Copy link
Contributor

Did not test it, but based on the discussion on #392 and #405 this should fix #655

@codecov
Copy link

codecov bot commented Oct 17, 2021

Codecov Report

Merging #656 (d4ac363) into main (01ed0ce) will decrease coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #656      +/-   ##
==========================================
- Coverage   82.65%   82.58%   -0.08%     
==========================================
  Files          56       56              
  Lines        5928     5690     -238     
  Branches     1344     1344              
==========================================
- Hits         4900     4699     -201     
+ Misses        745      708      -37     
  Partials      283      283              
Impacted Files Coverage Δ
src/util/isIdentifier.ts 100.00% <ø> (ø)
src/Options-gen-types.ts 75.00% <0.00%> (-25.00%) ⬇️
src/parser/index.ts 77.77% <0.00%> (-7.94%) ⬇️
src/computeSourceMap.ts 83.33% <0.00%> (-5.56%) ⬇️
src/util/getNonTypeIdentifiers.ts 94.44% <0.00%> (-5.56%) ⬇️
src/parser/plugins/types.ts 91.66% <0.00%> (-3.34%) ⬇️
src/parser/tokenizer/readWord.ts 82.35% <0.00%> (-3.02%) ⬇️
src/parser/traverser/util.ts 83.33% <0.00%> (-2.95%) ⬇️
src/parser/traverser/base.ts 95.45% <0.00%> (-1.69%) ⬇️
src/parser/traverser/statement.ts 82.16% <0.00%> (-0.44%) ⬇️
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01ed0ce...d4ac363. Read the comment docs.

@github-actions
Copy link

Benchmark results

Before this PR: 283.7 thousand lines per second
After this PR: 283.8 thousand lines per second

Measured change: 0.04% faster (2.04% slower to 4.49% faster)
Summary: Likely no significant difference

Copy link
Owner

@alangpierce alangpierce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the bug report and the fix! Agreed that this fixes the issue, and I just tested to confirm.

I was trying to figure out how I originally missed this case, and I see from MDN https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Lexical_grammar
this detail:

Additionally, the literals null, true, and false cannot be used as identifiers in ECMAScript.

So I guess those are not technically "keywords" but they're not allowed as identifiers either, so they should be in the list here.

There are a few more things that would be nice here:

  • Add a test
  • Add null as well
  • Document these three in a new section of the RESERVED_WORDS declaration
  • Maybe add a jsdoc comment for the isIdentifier function

But I'd be happy to do those as a quick follow-up PR.

@alangpierce alangpierce merged commit 6aba006 into alangpierce:main Oct 18, 2021
alangpierce added a commit that referenced this pull request Oct 18, 2021
Follow-up to #656:
* Add `null` as a new case that should not have a variable declared in enum
  emit.
* Rearrange the declaration to group these special non-identifier literals.
* Add a test, also confirming that `undefined` is *not* reserved here.
* Add a jsdoc comment for `isIdentifier`.
alangpierce added a commit that referenced this pull request Oct 18, 2021
Follow-up to #656:
* Add `null` as a new case that should not have a variable declared in enum
  emit.
* Rearrange the declaration to group these special non-identifier literals.
* Add a test, also confirming that `undefined` is *not* reserved here.
* Add a jsdoc comment for `isIdentifier`.
@alangpierce
Copy link
Owner

Published as 3.20.3.

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.

const enum doesn't allow true and false keywords
2 participants