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

Scan from line start when finding tag in tokenizer #12692

Closed

Conversation

moosichu
Copy link
Sponsor Contributor

This resolves a crash that can occur for invalid bytes like carriage
returns that are valid characters when not parsed from within literals.

There are potentially other edge cases this could resolve as well, as
the calling code for this function didn't account for any potential
'pending_invalid_tokens' that could be queued up by the tokenizer from
within another state.

This PR resolves #12674 and will unblock #12661.

#12449 could later simplify this when implemented, however this was a super simple change and fixes a known crash.

@Vexu
Copy link
Member

Vexu commented Aug 31, 2022

tokenSlice is quite heavily used so if this only affects .invalid tokens then it should only be called for them.

@moosichu
Copy link
Sponsor Contributor Author

moosichu commented Sep 5, 2022

Only just found some time to get around to this! Good shout, that change has been pushed.

This resolves a crash that can occur for invalid bytes like carriage
returns that are valid characters when not parsed from within literals.

There are potentially other edge cases this could resolve as well, as
the calling code for this function didn't account for any potential
'pending_invalid_tokens' that could be queued up by the tokenizer from
within another state.
Copy link
Member

@andrewrk andrewrk 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 this improvement. Needs a unit test before being merged - especially because there is already discussion about wanting to remove the workaround, in which case we would want to make sure it doesn't break!

@moosichu
Copy link
Sponsor Contributor Author

moosichu commented Oct 30, 2022

#12661 Already supercedes this anyway, so if you are happy with that (especially because it has tests) I don't think this needs to be merged. This was mainly intended as a quick low-risk workaround for a known crash, whereas #12661 actually fixes the issue by adhering to the spec.

Will close this for now! Mainly becuase any tests for this would be made redundant very quickly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants