Correctly handle carriage return characters according to the spec #12661
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Vexu
requested changes
Aug 28, 2022
This was referenced Aug 29, 2022
moosichu
force-pushed
the
fix/carriage-return-crash
branch
from
b289c7b to
fd960f9
Compare
moosichu
changed the title
moosichu
force-pushed
the
fix/carriage-return-crash
branch
3 times, most recently
from
582b344 to
4af4034
Compare
|
I've added a couple of tests that should hopefully do the trick, one to check the case that should work and the other to test the case that should error. |
moosichu
force-pushed
the
fix/carriage-return-crash
branch
2 times, most recently
from
cd43521 to
8fc2537
Compare
moosichu
force-pushed
the
fix/carriage-return-crash
branch
2 times, most recently
from
65f116d to
965779a
Compare
andrewrk
force-pushed
the
fix/carriage-return-crash
branch
from
965779a to
4ee22ca
Compare
moosichu
force-pushed
the
fix/carriage-return-crash
branch
2 times, most recently
from
7889cfc to
18dade0
Compare
moosichu
force-pushed
the
fix/carriage-return-crash
branch
from
18dade0 to
2bc3a48
Compare
andrewrk
force-pushed
the
fix/carriage-return-crash
branch
from
2bc3a48 to
eb3fc7d
Compare
|
CI checks are failing |
|
Yeah - I haven't had the time to address those test issues for the the past while, but should hopefully be able to at the end of this week if that's ok :) Sorry for the delay on this one! |
moosichu
force-pushed
the
fix/carriage-return-crash
branch
from
eb3fc7d to
601398d
Compare
|
Things should be fixed now! Ended up being something really simple, but only just had the time to get to it now. |
|
Are there any further changes desired here? Just double checking as I might have some time available to act on feedback this month if needed. |
moosichu
force-pushed
the
fix/carriage-return-crash
branch
from
601398d to
4e09283
Compare
|
Just resolved the merge conflicts that I saw had cropped-up (hopefully 🤞) |
Vexu
approved these changes
Jan 3, 2023
andrewrk
force-pushed
the
fix/carriage-return-crash
branch
from
4e09283 to
2ed3ce2
Compare
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.
Follow the guidance of ziglang#38: > However CR directly before NL is interpreted as only a newline and not part of the multiline string. zig fmt will delete the CR. Zig fmt already had code for deleting carriage returns, but would still crash - now it no longer does so. Carriage returns encountered before line-feeds are now appropriately removed on program compilation as well.
Previous commit was much less strict about this, this more closely matches the desired spec of only allow CR characters in a CRLF pair, but not otherwise.
Missed this comment from ziglang/zig-spec#83: > CR used as whitespace, whether directly preceding NL or stray, is still unambiguously whitespace. It is accepted by the grammar and replaced by the canonical whitespace by zig fmt.
moosichu
force-pushed
the
fix/carriage-return-crash
branch
from
2ed3ce2 to
219d4e5
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Taking guidance from: ziglang/zig-spec#38, carriage returns are now allowed preceding line-feeds when found in doc-comments or multiline strings. The CRLF is also only interpreted as a newline in this case. However - they are otherwise rejected when found in any other context except when used as plain whitespace.
This resolves #11414 and resolves #12674. It replaces #12692 which is a part of this PR.
Ideally the error reporting should be improved by #12449 sometime in the future, but I think I might take a look at that once this has been merged.