Fix compact_script fusing adjacent operators into an invalid token (< - -> <-)#1106
Open
yuvalrakavy wants to merge 2 commits into
Open
Fix compact_script fusing adjacent operators into an invalid token (< - -> <-)#1106yuvalrakavy wants to merge 2 commits into
compact_script fusing adjacent operators into an invalid token (< - -> <-)#1106yuvalrakavy wants to merge 2 commits into
Conversation
`Engine::compact_script`'s whitespace removal only inserted a separating
space between two identifier characters (so `let x` doesn't become `letx`).
It had no equivalent guard for operator characters, so two adjacent operator
tokens could fuse into a different token: `x < -1` compacted to `x<-1`, and
the lexer then reads `<-` as an invalid operator ("This is not Go!"), so the
compacted script no longer compiles — breaking compact_script's documented
"semantically identical to the input" contract.
Also insert a separating space when the previous and current characters are
both operator-lead characters. Brackets and delimiters are unaffected, so
ordinary compaction (`let x=1;`) is unchanged.
Adds a regression test.
7cac2b2 to
af59b33
Compare
`#[lang = "panic_impl"]` on a function is now a hard error on nightly
("attribute cannot be used on functions / can only be applied to foreign
functions"). `#[panic_handler]` already installs the `panic_impl` lang item,
so the explicit `#[lang = "panic_impl"]` is redundant. Remove it (and the
now-unused `lang_items` feature) so `NoStdBuild` builds on current nightly.
Collaborator
|
Would it be better to actually just merge the two tokens and check, via This way we don't have to hardcode anything in. |
This file contains hidden or 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
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.
Problem
Engine::compact_script's doc comment promises output "semantically identical to the input script, except smaller in size." That breaks for operator adjacency:Removing the space in
x < -1yieldsx<-1; the lexer then reads<-as an (invalid) operator, so the compacted script fails to compile.Cause
In
TokenIterator::next, the compressed-buffer builder inserts a separating space only between two identifier characters (to avoid merginglet xintoletx). There is no equivalent guard for operator characters, so<followed by-(two separate tokens) is emitted as<-.Fix
Also insert a separating space when the previous and current characters are both operator-lead characters (
= ! < > & | ^ + - * / % ~ . : ?). Brackets and delimiters are not in that set, so ordinary compaction (let x=1;) is unchanged; single tokens such as?./??/?[are emitted whole, so they are unaffected. It also prevents accidentally forming reserved tokens such as->,++and--.Test
tests/tokens.rs::test_compact_script_operator_adjacencyround-trips severaloperator + negative literalscripts throughcompact_scriptand asserts the result recompiles (and that<-is never produced). The test fails onmainand passes with this change; the full test suite is green.Note — a second, unrelated commit is included (
Fix no_std_test panic handler for current nightly). Current nightly makes#[lang = "panic_impl"]on a function a hard error, which was failing theNoStdBuildjob on this (and any current) PR — not related to the tokenizer change.#[panic_handler]already installs that lang item, so the explicit#[lang = "panic_impl"](and the now-unusedlang_itemsfeature) are removed. Happy to split it into its own PR if you'd prefer to keep this one focused.