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 ability to disable lexical dep #7752

Closed
Manishearth opened this issue Aug 3, 2023 · 6 comments · Fixed by #7758
Closed

Add ability to disable lexical dep #7752

Manishearth opened this issue Aug 3, 2023 · 6 comments · Fixed by #7758
Milestone

Comments

@Manishearth
Copy link
Contributor

swc uses the lexical family of crates for parsing numbers. It's a fast crate, but unfortunately it has a number of soundness issues, and so far there has been no movement from upstream on this (or accepting any of the patches opened so far):

It would be nice for users of swc to be able to choose whether lexical is used. I would recommend making lexical an optional dependency, enabled by default, so that people can use swc with no-default-features if they wish to avoid the dep. The num family of crates is well established and has alternate code that does the same stuff as lexical, though perhaps not as optimized.

We have a patch we can submit to support this.

@kdy1 kdy1 added this to the Planned milestone Aug 3, 2023
@kdy1
Copy link
Member

kdy1 commented Aug 3, 2023

It's fine to switch to num completely, but I don't want it to be optional dep. It's too much for the maintainers

@Manishearth
Copy link
Contributor Author

It's a relatively small patch. How about we provide both options (remove the dep completely vs make it optional) and you can choose?

@kdy1
Copy link
Member

kdy1 commented Aug 3, 2023

Who choose? Removing dep completely is fine, but I'm not sure what you are saying

@Manishearth
Copy link
Contributor Author

Manishearth commented Aug 3, 2023

Oh, I was saying that the patch to make it optional is not much more complicated and you can choose which PR to land after looking at them: whether to land the one making it optional or the one removing it completely. But if you strongly prefer removing the dep we can just do that.

@gmcsorley-work
Copy link
Contributor

I've sent two pull requests:

You can decide which approach you prefer.

@kdy1 kdy1 closed this as completed in #7758 Aug 6, 2023
kdy1 pushed a commit that referenced this issue Aug 6, 2023
This PR replaces the current usage of lexical within the swc_ecma_parser
crate with equivalent parsing of large numbers using BigInt.

**Description:**

As discussed in
#7752, lexical contains a
number of soundness issues but doesn't appear to be actively supported.
Given the relatively low integration surface it seems reasonable to
replace the usage of lexical with another package to avoid this issue.

**Related issue:**

- Closes #7752
@kdy1 kdy1 modified the milestones: Planned, v1.3.75 Aug 8, 2023
kdy1 pushed a commit to kdy1/swc that referenced this issue Aug 15, 2023
This PR replaces the current usage of lexical within the swc_ecma_parser
crate with equivalent parsing of large numbers using BigInt.

**Description:**

As discussed in
swc-project#7752, lexical contains a
number of soundness issues but doesn't appear to be actively supported.
Given the relatively low integration surface it seems reasonable to
replace the usage of lexical with another package to avoid this issue.

**Related issue:**

- Closes swc-project#7752
@swc-bot
Copy link
Collaborator

swc-bot commented Sep 7, 2023

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@swc-project swc-project locked as resolved and limited conversation to collaborators Sep 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging a pull request may close this issue.

4 participants