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

fix(parser): remove erroneous debug assertion #2356

Merged
merged 1 commit into from
Feb 9, 2024

Conversation

overlookmotel
Copy link
Collaborator

@overlookmotel overlookmotel commented Feb 9, 2024

This was a bit of a whoopsie in last batch of PRs. This assertion shouldn't be there, because all reads are now via source.position().read(), so this assertion says "you can only read some byte values".

Only reason it didn't blow up conformance tests is that they run in release mode.

Sorry. Please merge soon as you can and cover my shame!

@overlookmotel
Copy link
Collaborator Author

overlookmotel commented Feb 9, 2024

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@github-actions github-actions bot added the A-parser Area - Parser label Feb 9, 2024
@overlookmotel overlookmotel marked this pull request as draft February 9, 2024 04:14
Copy link

codspeed-hq bot commented Feb 9, 2024

CodSpeed Performance Report

Merging #2356 will not alter performance

Comparing 02-09-fix_parser_remove_erroneous_debug_assertion (ed961b0) with main (8376f15)

Summary

✅ 27 untouched benchmarks

@overlookmotel overlookmotel marked this pull request as ready for review February 9, 2024 04:18
@overlookmotel
Copy link
Collaborator Author

Just to say, please don't release crates until this has been merged. It's quite a bad bug. Will cause parser to panic on any file containing irregular whitespace when run in debug mode.

Again, really sorry I allowed this bug to slip in in the first place.

Copy link
Member

Boshen commented Feb 9, 2024

Merge activity

  • Feb 9, 7:55 AM EST: @Boshen started a stack merge that includes this pull request via Graphite.
  • Feb 9, 7:55 AM EST: @Boshen merged this pull request with Graphite.

@Boshen Boshen merged commit 2f6cf73 into main Feb 9, 2024
22 checks passed
@Boshen Boshen deleted the 02-09-fix_parser_remove_erroneous_debug_assertion branch February 9, 2024 12:55
IWANABETHATGUY pushed a commit to IWANABETHATGUY/oxc that referenced this pull request May 29, 2024
This was a bit of a whoopsie in last batch of PRs. This assertion shouldn't be there, because all reads are now via `source.position().read()`, so this assertion says "you can only read some byte values".

Only reason it didn't blow up conformance tests is that they run in release mode.

Sorry. Please merge soon as you can and cover my shame!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-parser Area - Parser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants