Fix silent output corruption in modern CSS and JavaScript, and build a regression safety net - #12
Merged
Merged
Conversation
The repository contains 71 source/expected fixture pairs under src/test/resources, and its README states that they are discovered and run automatically, but no such test existed. 66 of them compress with the default options and are now wired up; 62 pass and become regression tests. The five fixtures whose names begin with an underscore need non-default options and are left out. Four fixtures do not match current output and are quarantined in KNOWN_FAILURES so the rest can protect the upcoming changes: - zeros.css emits a zero time value with a unit where the golden expects 0 - issue86.js emits a bare number before .toString() where the golden parenthesises it - promise-catch-finally-issue203.js does not munge callback parameters - jquery-1.6.4.js is 137795 bytes against a golden of 101992 bytes
Add CliOptionTest to pin the externally visible behaviour of the compressor options (munge/nomunge, global name preservation, /*! ... */ license comment retention, ordinary comment stripping, CSS line-break handling) so later refactoring cannot change them silently. Add JsOutputSyntaxTest to feed the compressed output of every JS fixture to "node --check", catching output that is not valid JavaScript at all -- a class of defect an expected-value comparison can never catch for a case nobody thought of. Skips cleanly via @EnabledIf when Node is unavailable so `mvn test` stays Node-independent.
catch (Exception) around both parsing and code generation let a crash during the munging/output pass (NPE, index-out-of-bounds) be silently swallowed as if it were an unparsable-input case, letting the test pass with zero assertions. Split the two phases: only the JavaScriptCompressor constructor's EvaluatorException (unparsable input) is caught and excused; compress() runs outside the try so a codegen crash propagates as a real test error, as it should when the class of jquery-1.6.4.js's size and complexity are the fixtures this test exists to protect.
Update build plugins to latest stable 3.x versions: - maven-compiler-plugin: 3.11.0 → 3.16.0 - maven-shade-plugin: 3.2.1 → 3.6.2 - maven-jar-plugin: 3.1.0 → 3.5.1 - maven-javadoc-plugin: 3.11.2 → 3.12.0 All versions maintain Java 11 compatibility and have been verified to build successfully with no test failures (236 tests, 0 failures, 2 skipped). The shaded jar continues to work correctly for both CSS and JavaScript minification. Java 11 target version remains unchanged.
Remove obsolete build artifacts: - Delete .travis.yml (Travis CI config for old upstream) - Delete build.xml and ant.properties (Ant build configuration) - Remove Travis badge from README.md Update documentation to reflect current CI setup: - Update BUILDING.md to document GitHub Actions instead of Travis CI - Keep "Migration from Ant to Maven" section in README.md (explains project history) - Confirm GitHub Actions workflow uses v4 actions and Java 11/17/21 The project has been using Maven and GitHub Actions since the previous release. This cleanup removes misleading legacy build files and outdated CI documentation.
Removing the whitespace before '(' turns an identifier into a function token
per CSS Syntax Level 3, which produced invalid output for
'@container <name> (...)', '@supports ... not (...)' and '@scope ... to (...)'.
The narrow patch that used to restore this for the single keyword 'and' also
lowercased it, so it is replaced by an explicit keyword-case normalisation
that leaves whitespace untouched (inserting a space here would break
functional pseudo-classes like ':not(...)').
The colour optimiser rewrites values by plain text substitution without looking at the property name, so it turned a custom property holding a hex colour into a named colour and rewrote @Property initial-value descriptors. A custom property value is an arbitrary token stream and has to survive unchanged. - Preserve custom property declaration values (--name: ...;) as an opaque token, after string/URL/calc preservation so a quote or semicolon inside an already-preserved token cannot be mistaken for the end of the value. - Preserve every @Property at-rule block verbatim the same way, in the same layer, right before the custom-property scan. Running it there (rather than on raw source) means "@Property" mentioned inside a comment can never be matched, since the comment is already an opaque placeholder by then, and a brace inside a descriptor string (e.g. syntax: "}") can never be misread as the block's own closing brace, since the string is opaque too. It also means the stylesheet-global and/or/not keyword regex can't case-fold text like NOT(x) inside a custom property value, since by the time that regex runs the value is already an opaque placeholder. - Because both scanners run after string preservation, a captured value can itself contain a placeholder reference to an earlier-preserved token (e.g. --content: "a;b";, or an @Property syntax descriptor's quoted value). Resolve any such nested reference before storing the new token: the final restoration pass walks preserved tokens once in index order, so a later-added token that still embeds a reference to an earlier index would otherwise never get resolved and would leak into the output as a raw ___YUICSSMIN_PRESERVED_TOKEN_N___ placeholder.
An empty '@layer name {}' declares the layer and fixes its position in the cascade order, so removing it changes rendering; the empty-rule removal had no at-rule awareness at all. Excluding '@' from the removal regex's char class is not sufficient on its own: replaceAll takes the leftmost match at any position, so it just resumes matching right after the '@', corrupting "@layer utilities {}" into a stray "@" glued onto the next selector. The fix matches three explicit cases instead: an '@layer' prelude is kept verbatim, any other at-rule prelude (e.g. '@media', which does nothing when empty) is matched and deleted as one unit so its '@' is never left stranded, and a plain empty rule is deleted as before. Also extends the at-rule name lowercasing list with the modern at-rules this file already special-cases elsewhere: supports, container, layer, property, scope, starting-style. Adds a regression test asserting compressed output never contains the '___YUICSSMIN' placeholder prefix, run across inputs that exercise several preservation passes at once (string, data URI, calc(), preserved comment, ordinary comment, custom property, @Property block), to guard against a class of defect where an internal placeholder leaks into shipped output.
zeros.css.min expects "transition-delay:0" from "0.0ms". CSS Values and Units Level 3 allows omitting the unit only for a zero <length>; <time> has no such exemption, so that golden encodes invalid CSS. Current behaviour already keeps the unit, which is correct, so the golden stays quarantined with a written reason rather than being matched or edited. Adds tests locking in that zero <time> keeps its unit while zero <length> still drops it.
…hand methods
Rhino's AST keeps the optionality, but the generator had no case for it and fell
through to toSource(), which does not emit the operator either. Turning a?.b into
a.b changes a safe undefined into a TypeError.
An optional catch binding was emitted as 'catch()', which is a syntax error.
ES6 shorthand methods in object literals were expanded to 'm:function(){}',
losing the shorthand form.
Rhino marks every link of an optional chain with the same QUESTION_DOT node
type (and FunctionCall.isOptionalCall() over-reports the same way), not just
the one link that actually carries "?." in the source. The prior fix treated
QUESTION_DOT as "emit ?." unconditionally, which widened mixed chains such as
"a?.b.c" into "a?.b?.c": on a null "a", the original throws a TypeError while
the widened version silently evaluates to undefined. That is its own class of
silent behaviour change, symmetrical to the bug this feature was meant to fix.
MungedCodeGenerator now takes the original source text and, for each
QUESTION_DOT-typed property/element access and for each call, reads the actual
text between the target and the next token to decide whether that specific
link has its own "?." or is a plain "."/"["/"(" continuation of a chain that
started optional earlier. Comments are stripped from the compared text first
so a comment's literal characters can never be mistaken for the operator, and
out-of-range positions fall back to the conservative, non-optional form.
JavaScriptCompressor now keeps the source string it already reads and passes
it into MungedCodeGenerator; the old 2-arg constructor remains as a
backward-compatible overload for existing callers that build an AST without
an associated source string.
…ards The pipeline rendered the AST to a string and then ran ten blind regexes over it, while only single and double quoted strings were protected. Template literal and regex literal contents were therefore rewritten, and the line breaker sliced the final string at fixed offsets, cutting through identifiers. The generator now emits only the separators the syntax requires, so the post-processing stage is gone and with it this entire class of corruption. Line breaking is now driven by offsets the generator records between statements, so a break can never land inside a token either.
…erand merges
Four pre-existing MungedCodeGenerator defects, all exposed by Task 12's
removal of the "emit loosely, strip with regex" pipeline:
- yield* silently lost its star (Yield reports Token.YIELD_STAR, not
Token.YIELD, for the delegating form; the switch had no case for it and
fell back to Rhino's own toSource(), which drops the star).
- Any labeled statement crashed the compressor with a ClassCastException.
LabeledStatement.getType() reports Token.EXPR_VOID, not Token.LABEL, so
it was always miscast as a plain ExpressionStatement. The now-provably
dead `case Token.LABEL:` (Label, not LabeledStatement, is the only node
that ever reports that type, and it's never dispatched through
visitNode()) was removed rather than left as misleading dead code.
- "a + +b", "a - -b", and "start + +new Date()" rendered as "a++b",
"a--b", "start++new Date()" - all SyntaxErrors - because the operator's
trailing +/- ran directly into the operand's leading +/-.
- "x / /re/.test(...)" rendered as "x//re/.test(...)", turning the rest of
the statement into a line comment. This passes `node --check` (the
output still parses) while silently changing what the code does.
The last two share one fix: after an infix operator or a unary operator
renders, check whether the operand's first rendered character would
combine with the operator's last character into a different token
(+/+, -/-, //, /*), and insert a single separating space only then.
Also strengthened ModernJsTest's lineBreakNeverSplitsAnIdentifier: its
prior `contains("beta")` assertion did not discriminate against a
regression to the project's old fixed-column line slicer, since the
first "beta" occurrence happens to land exactly on a 20-column boundary
for this input while the second gets split - now counts whole-token
`\bbeta\b` matches and requires both to survive intact.
267 baseline tests + 7 new -> 274 tests, 0 failures, 2 skipped. Golden
fixtures (float.js byte-identical) and JsOutputSyntaxTest (jQuery
included) unaffected; jQuery 1.6.4's compressed output is byte-identical
before/after, since it doesn't exercise any of the four patterns.
team-lead's systematic adjacency sweep found four more operator/operand
merge cases beyond the two ("a + +b", "a - -b") in the original brief:
- -a -> --a (double unary minus silently becomes pre-decrement,
mutating the operand instead of leaving it alone)
+ +c -> ++c (same shape for unary plus / pre-increment)
a + ++b -> a+++b (reparses as "(a++) + b" - increments the wrong
variable and changes the result)
a - --b -> a---b (same shape for subtraction / pre-decrement)
Verified against 86326d4 (the existing fix) that all four already produce
correct output: the fix checks the operand's actual rendered first
character rather than special-casing specific node-type pairs, so it
already generalizes to nested unary and prefix-increment/decrement
operands. Confirmed two ways:
- String diff against a rebuild of the pre-86326d4 generator (from
11fc36b) shows the four cases regress to exactly the broken forms
above under the old code, and match the fixed forms under current
code.
- Ran both the original (uncompressed) source and the compressor's
output for all four cases (plus the two matching subtraction/postfix
variants) through node and diffed the resulting values/mutations -
byte-for-byte identical in every case.
Added five regression tests with exact (assertEquals) assertions per
team-lead's request, including one that asserts "a++ + b" and "a + ++b"
- which collide to the identical string "a+++b" under the old,
separator-free code - now render differently.
No production code change; MungedCodeGenerator.java is unchanged.
279 tests (274 + 5 new), 0 failures, 2 skipped.
…colons
Fix round 1 on defects (C) and (B), from a systematic adjacency sweep and
a re-check of the (B) fix's sibling logic.
CRITICAL: a bare "<" immediately before an operand rendering "!--" (e.g.
"!" applied to a prefix "--x") formed "<!--", the Annex B
SingleLineHTMLOpenComment:
a < !--b -> a<!--b
Since minified output is a single line, that "comment" swallowed the rest
of the entire FILE, not just the rest of the statement - worse than
defect (D), and like it, `node --check` reported the corrupted output as
valid (it parses; it just does nothing from that point on). insertSeparat
orIfMerging previously only ever looked one character ahead, which cannot
distinguish "<" from "<=" or "<<" (both safe - once consumed as their own
token, the next token scan starts past where "<!--" could ever form), so
this needed a dedicated 3-character lookahead scoped to the bare "<"
operator specifically.
IMPORTANT: needsSemicolon still excluded Token.LABEL - the same
misconception the (B) fix corrected in visitNode's switch.
LabeledStatement.getType() is always Token.EXPR_VOID, so that exclusion
never fired, and every labeled statement got an unconditional trailing
";", including ones that don't need one:
outer:for(var i=0;i<3;i++){f();} -> outer:for(...){{f();}};
outer:{ g(); } -> outer:{g();};
needsSemicolon now recurses into the labeled statement's wrapped
statement instead of special-casing labels directly, so "outer: x();"
(whose wrapped statement is an ExpressionStatement) still keeps its ";",
while a labeled for-loop or block no longer gains a needless one.
Verified as both unaffected (no regression, no needless bytes): "a << !--b"
(-> "a<<!--b"), "a <= b", "a < b", and "a-- > b" (-> "a-->b" - "-->" is
only special at the start of a line, which the generator never produces
it in, so a separator there would only cost bytes).
Tightened the existing yield*/plain-yield/labeled-statement tests
(originally contains()-based) to exact assertEquals, per review feedback
that a contains-only test is why the semicolon gap went undetected.
287 tests (279 + 8 new), 0 failures, 2 skipped. float.js golden
byte-identical, JsOutputSyntaxTest 4/4 (jQuery included), jQuery 1.6.4
compressed output unchanged (0 byte delta).
…building scopes
ScopeBuilder's generic child recursion walked Rhino's low-level
Node.getFirstChild()/getNext() chain, which is only populated for
list-style containers (Block, AstRoot, ...). AST nodes that keep their
children in typed fields instead - FunctionCall arguments, ObjectLiteral
property values, ArrayLiteral elements, IfStatement/WhileLoop/ForLoop
bodies, InfixExpression operands, and so on - reported zero children
through that chain, so the fallback silently skipped them entirely.
A function expression sitting in one of those positions never got a
scope, so its parameters were never munged:
p.then(function(longParamName){...}) // call argument
var o = { m: function(longParamName){...} } // object property value
var arr = [ function(longParamName){...} ] // array element
The same gap also meant a `var` declared inside an if/while/for/switch
body was never discovered at all, since ScopeBuilder could not descend
past the containing IfStatement/WhileLoop/... node either:
function outer(x) { if (x) { var innerVariableName = 1; } ... }
jQuery, in particular, is overwhelmingly fn(function(){...}) and
{key: function(){...}}, which is why it compressed to 137798 bytes
against a 101992-byte golden - 26% too large.
Replaced the broken low-level-chain fallback with one built on
AstNode.visit(NodeVisitor), which is what Rhino itself uses to enumerate
a node's children correctly regardless of how they are stored, so it
does not require hand-enumerating every AST node type the way the
low-level chain (or a from-scratch reimplementation of the per-type
traversal already in MungedCodeGenerator) would. Since ScopeBuilder
threads currentScope/braceNesting as method parameters rather than
mutable instance state, the replacement is scoped to enumerate exactly
the direct children of one node at a time (return true once for self,
false afterward), delegating each child straight back into visitNode(),
which continues the recursive descent with the correct scope.
jQuery 1.6.4 now compresses to 106970 bytes (was 137798) against the
101992-byte golden - the 26% gap is under 5%. The residual gap is not a
munging defect: 171 bytes are a second preserved "/*!" banner comment
(a bundled Sizzle engine license at line 3770) that this generation
preserves and the golden's generation did not, and the rest is different
(but equally valid) short-name choices from the free-symbol pool between
compressor generations. Documented in JsGoldenFileTest, which keeps
jquery-1.6.4.js quarantined for that reason.
Of the three previously-quarantined JS goldens:
- promise-catch-finally-issue203.js now matches its golden exactly (the
only prior difference was munging: function(res) vs function(a)) and
is removed from KNOWN_FAILURES.
- issue86.js still does not match, but for an unrelated, pre-existing
reason with nothing to do with scope or munging: the compressor emits
".0.toString()" (a leading-dot numeric literal, valid on its own) where
the golden has "(0).toString()" (parenthesized) - a MungedCodeGenerator
numeric-literal formatting difference, out of this task's scope.
KNOWN_FAILURES' comment is updated to describe this precisely.
- jquery-1.6.4.js stays quarantined per above.
Also closes a class of bug from two Criticals earlier this release
(division-before-regex forming "//", and "<" before "!--" forming
"<!--") for good: added a string-level comment-injection guard to
JsOutputSyntaxTest, run over the same fixtures the syntax check uses.
"node --check" only proves output parses, never that it means the same
thing - node's non-strict scripts still honor the legacy Annex B.1.3
HTML-comment-in-script grammar, so "<!--" anywhere, or "-->" at the
start of a line, silently opens a comment instead of raising a syntax
error, and so does a stray "//" or "/*" the generator never intended as
one. The guard is a small string scanner (not a full lexer) that skips
string/template/regex literal content and the interior of a preserved
"/*!" banner comment - a blind substring search was tried first and
immediately false-positived on jQuery's own legitimate output (regex
literals like /^\/\// and strings like "//" both occur in its compressed
form), so context-awareness is required, not optional. Covered by direct
unit tests of the scanner itself (catches each of the four sequences;
does not flag the legitimate look-alikes above) in addition to running
over all four JS fixtures.
311 tests (287 + 24 new), 0 failures, 2 skipped. JsOutputSyntaxTest
19/19 (4 syntax + 4 injection-guard-over-fixtures + 11 direct scanner
unit tests). float.js golden byte-identical. No .min, fixture, or CSS
file changed.
…guard
Two shapes let a "/" that is actually division get misclassified by
CommentInjectionScanner as opening a regex literal. Once misclassified,
skipRegex() treats everything up to the next unescaped "/" as inert
regex body without scanning it at all, so any of the four dangerous
sequences hiding in that span was a genuine false negative - exactly
what this guard exists to catch, going undetected by the guard itself.
1. "}" was in REGEX_PRECURSORS, but it is reachable immediately before a
genuine division, not just before a block statement's regex:
"var f = {} / a;" compresses to "var f={}/a;". Removed "}" from the
set, so the "/" after it is treated as division and its span is
scanned like ordinary code. The competing, also-reachable case - a
real regex literal right after a block statement, e.g.
"if(x){} /re/.test(y);" -> "if(x){{}}/re/.test(y);" - stays safe:
scanned as ordinary code, "re/.test(y);" contains none of the four
sequences, so no false positive.
2. Found by auditing the rest of REGEX_PRECURSORS for the same shape:
postfix "++"/"--" was folded into a plain "+"/"-" token by the
tokenizer's generic one-char fallback, which IS a regex precursor -
correct for a binary/unary "+"/"-" awaiting an operand, wrong for a
postfix operator that has already completed a value. "var b = a++ /
2;" compresses to "var b=a++/2;", also reachable. "++"/"--" now get
their own two-character token, deliberately kept out of
REGEX_PRECURSORS.
Both closed with a reachable counterexample turned into a regression
test first (confirmed failing against the prior code), then fixed, then
reconfirmed passing - plus a paired "still allows the legitimate case"
test for each, so neither fix quietly narrows the guard past what the
corpus needs.
Corrected two comments that overstated what the guard actually
guarantees: the class-level test comment no longer claims the sequence
class is "closed for good" outright, and CommentInjectionScanner's
javadoc no longer claims a misclassification "can only cause a missed
regex boundary, not a missed injection" - that was false, as this fix
demonstrates. It now states the real asymmetry (misreading a real regex
precursor as division is a safe false positive; misreading division as
a regex precursor is the dangerous direction) and is explicit that
REGEX_PRECURSORS is a curated allowlist against known-reachable cases,
not a derivation from the grammar - i.e. not a proof no third case
exists.
315 tests (311 + 4 new), 0 failures, 2 skipped. JsOutputSyntaxTest
23/23. compressedOutputHasNoCommentInjection still passes clean on all
four real fixtures, jQuery included. ScopeBuilder.java untouched.
…pted NUL bytes
Continues the R17 line of fixes to CommentInjectionScanner's
REGEX_PRECURSORS - each entry removed here follows the same shape:
a token that is reachable immediately before genuine division in real
compressor output, not just before its "textbook" regex-precursor use.
1. Removed "of" from REGEX_PRECURSORS. "of" is a contextual keyword,
not a reserved word - an ordinary identifier everywhere outside a
for-of head, and JavaScriptCompressor's own munged-name pool already
treats it that way (twos.remove("of") is filed separately as "ES6+
two-letter keywords" from the genuinely reserved words above it).
Confirmed reachable: "var of = 4; var x = of / 2;" compresses to
"var of=4;var x=of/2;". Through the scanner, "x=of/y<!--INJECT-->z/w;"
reported zero violations before this fix - the same failure shape as
"}" and postfix "++". Added the same RED (counterexample) -> GREEN
(fixed) -> paired-allow (competing case, "for(x of/re/g.exec(s)){}",
does not become a false positive) test triad used for the earlier two.
2. Pinned the accepted false-positive tradeoff from removing "}" with
an explicit, clearly-labelled regression test:
"if(x){{y=1;}}/foo\/\//.test(z);" - a genuine regex literal
containing escaped slashes, sitting right after a block statement -
is scanned as ordinary code once "}" stops being treated as a regex
precursor, and its own escaped "\/" pairs read as a live "//" once
they are no longer skipped whole as regex content. This is accepted
as the deliberate cost (a false positive costs one investigation; a
false negative in the other direction ships a Critical, and the
asymmetry is why "}" was removed from REGEX_PRECURSORS in the first
place) rather than special-cased away, which would add real scanner
complexity for a test-only guard. The test exists specifically so a
future contributor who sees this fire does not "fix" it by putting
"}" back and silently reopening the false negative.
3. Fixed three literal NUL bytes (0x00) that had ended up in the
`lastToken = " value";` sentinels in skipQuoted/skipTemplate/
skipRegex, present since 39de1e9. Not cosmetic: it made `grep`, and
other locale-aware tools, treat the whole file as binary and return
nothing, which cost real time during review. Replaced with real
space characters (byte-level, to be certain); `grep -c "lastToken"`
on the file now returns a count instead of nothing, and `file`
reports it as text.
4. Clarified the "++"/"--" tokenizer comment to also cover prefix use,
correcting a wrong assumption along the way: prefix "++"/"--"
immediately before a regex literal is NOT rejected as a syntax error
by node or by Rhino, and is real, reachable compressor output
("var a=1; ++/x/.test(a);" compresses to itself unchanged) - prefix
"++" only requires a syntactic LeftHandSideExpression operand, which
a regex-literal-then-call-expression satisfies; whether the result is
actually assignable is a separate, later check this compressor
reproduces rather than performs, same as node's own checker. The
comment now gives the correct reason the shared "++"/"--" token is
safe for both: excluding it from REGEX_PRECURSORS puts the following
"/" on the safe side of the documented misclassification asymmetry
either way - scanning a real regex as ordinary code costs nothing
when, as here, it holds none of the four dangerous sequences - so the
token not distinguishing prefix from postfix does not matter.
Added a regression test for this reachable prefix case too.
Class-level CommentInjectionScanner javadoc updated: three reachable
cases found so far ("}", postfix "++"/"--", "of"), not two; references
the pinned false-positive test; states plainly this is a curated
allowlist against cases found reachable, not a grammar-derived proof
of completeness.
320 tests (315 + 5 new), 0 failures, 2 skipped. JsOutputSyntaxTest
28/28; compressedOutputHasNoCommentInjection confirmed individually
green for all four real fixtures, jQuery included. ScopeBuilder.java
untouched - diff is JsOutputSyntaxTest.java only.
ScopeBuilder never called ScriptOrFnScope.preventMunging(), so a local
readable by a direct eval() call, or shadowable by a with statement,
could still be renamed - breaking eval("name") lookups and with's
dynamic property resolution. This regressed with the prior scope-tree
fix, which started giving scopes (and munging) to function expressions
in call arguments that eval could previously reach only by accident.
ScopeBuilder now marks the current scope and every enclosing scope as
unsafe to munge when it sees a bare reference to the identifier "eval"
(called or not; a property access like obj.eval is unaffected, since
that's always an indirect eval and can't see locals anyway) or a with
statement.
Also fixes ScriptOrFnScope.munge(), which returned before reaching its
own recursion into sub-scopes whenever the current scope was unsafe -
previously unreachable dead code, since preventMunging() had no
callers. Left as-is, protecting one scope would have silently disabled
munging for every function nested inside it, even ones with no eval or
with of their own.
- CHANGELOG.md: add the 2.4.11-SNAPSHOT entry covering the CSS and
JavaScript correctness fixes, the ScopeBuilder compression
improvement, and the JUnit 5 / golden-fixture / build test-tooling
work
- README.md: fix the stale "2.4.10-SNAPSHOT" version references (the
version line and the two example jar filenames) to match pom.xml's
2.4.11-SNAPSHOT
- docs/ES6_MIGRATION_PLAN.md: correct the completion checklist against
measured Rhino behaviour
- "ES6構文をパースしてもエラーにならない" was false: Rhino cannot
parse class, async/await, import/export, dynamic import(),
new.target, or for-of with const, at any version
- "for-in/for-ofループ" was partially false: for-of works with
var/let but not const
- "labeled文" was false when written (any labeled statement crashed
with a ClassCastException); true only as of this release
- checked off the two Phase 1.1 items that were actually already
done (VERSION_ES6, CompilerEnvirons review)
Review round 1 caught three overstatements in the 2.4.11-SNAPSHOT entry: - the CSS lowercasing bullet claimed at-rule, function, and pseudo-class names were all normalised; only the at-rule directive list changed in this release (commit a186768), the function-name and pseudo-class regexes are untouched. Narrowed the claim to at-rule names. - the optional-chaining bullet named the wrong variable as the source of the divergence between `a?.b.c` and `a?.b?.c`. Verified in Node: both forms return `undefined` (no throw) when `a` itself is null, since the chain short-circuits at the first `?.`. The divergence is when `a` is non-null but `a.b` is null: `a?.b.c` throws a TypeError, the widened `a?.b?.c` silently swallows it. Rewrote the explanation to match. - the comment-injection bullet implied `//` injection was less severe than `<!--` injection ("rest of the statement" vs "rest of the file"). Both are single-line comment openers and minified output is a single line, so both swallow everything after them. Made the descriptions symmetric.
`MungedCodeGenerator.visitNode`'s `default:` arm calls `node.toSource()`,
which re-prints the ORIGINAL source of the whole subtree. Every identifier
inside therefore keeps its pre-munge spelling while its declaration was
munged, silently turning locals into globals, and `?.` is dropped outright.
The output parses, so neither the goldens nor `node --check` caught it:
function f(alpha, beta) { var gamma = alpha ?? beta; return gamma; }
-> function f(c,b){var a=alpha ?? beta;return a;}
Two changes, plus one addition beyond the reported four operators.
1. Handle the operators that reached the fallback. Verified against the
Rhino 1.8.0 constant pool rather than assumed names:
`NULLISH_COALESCING` (an `InfixExpression`), and `ASSIGN_LOGICAL_OR`,
`ASSIGN_LOGICAL_AND`, `ASSIGN_NULLISH`, `ASSIGN_EXP` (all `Assignment`,
which extends `InfixExpression`). Routing them through
`visitInfixExpression` visits both operands, so they munge, and keeps
them in `insertSeparatorIfMerging`'s view. `**=` was not in the original
triage; it leaked the same way and is fixed with the rest. Parentheses
around `??` mixed with `||`/`&&` survive because Rhino records them as
`ParenthesizedExpression` nodes, which already have a case.
`Token.DEBUGGER` is also handled here now. It too reached the fallback,
where `toSource()` emitted `debugger;\n` - an embedded newline in output
that both `addLineBreaks` and the Annex B merge-hazard reasoning assume
is a single line - and then `needsSemicolon()` added a second `;`.
2. Make the fallback loud. `MungedCodeGenerator.STRICT_PROPERTY`
(`yuicompressor.strict`) turns `default:` into a thrown
`UnsupportedSyntaxException` naming the node type and class, alongside
the existing `yuicompressor.debug` warning hook. Off by default, so
production callers keep today's behaviour.
This matters more than the five operator cases: Release 2 upgrades
Rhino, which makes more syntax parse, which routes MORE node types into
the fallback. Fixing only the known operators would leave the trap armed
and better hidden.
Tests: `StrictNodeCoverageTest` runs every `.js` fixture plus a 25-row
modern-syntax table under strict mode (zero fallbacks), records the syntax
Rhino genuinely cannot parse as expected parse failures, and pins both
directions of the flag using array comprehensions - a node type Rhino still
parses and this generator has no case for. `ModernJsTest` gains munged-output
assertions for each operator, the `??` + `?.` end-to-end case, parenthesis
preservation, and the `debugger` newline.
`insertSeparatorIfMerging` inserts its separating space with a raw
`output.insert(mark, ' ')`, but the operand was already rendered by then, so
any safe-break offset recorded inside it (a nested function expression's
statement boundaries) was recorded at its pre-insertion position and never
adjusted. Every such offset therefore landed k characters early, k being the
number of separators inserted before it, and `addLineBreaks` cut inside the
preceding token.
At `--line-break 20`, with two nested insertions (k=2):
var q = a + + +function(){ abcdefghijklmnop; }();
-> var q=a+ + +function(){abcdefghijklmno
p
;}();
var q = a + + +function(){ var s = "hello"; }();
-> var q=a+ + +function(){var a="hello
"
;}();
Only the second is loud. The first still parses - as two statements naming
two different variables - which is why this survived the review rounds that
checked output with `node --check`.
Route both insert sites through a new `insertSeparator(int)` that shifts
every already-recorded offset at or after the insertion point. The list is
ascending, so the walk stops at the first offset below `mark`.
Tests: three cases at `--line-break 20` covering k=2 (identifier split and
string-literal split) and k=1, each asserting the exact output, that Rhino
re-parses it, and that every inserted newline follows a ";" or "}". The
existing `lineBreakNeverSplitsAnIdentifier` gains the same boundary check.
Parsing alone is deliberately not the only assertion: the identifier case
parses either way. Rhino is used rather than node so the check is
unconditional.
Four matchers in CssCompressor located their construct by its literal text,
with no regard for whether an at-rule or a declaration could actually begin
there. Two were reported; the audit asked for by the review found two more of
the same shape. All are fixed by one shared `startsAtBoundary` helper rather
than four separate guards.
1. `@property` (reported as I2). Located context-free, so the match preserved
everything up to the next balanced "}" verbatim - which meant the rule
AFTER it was emitted completely unminified:
a { background: url(/img/@property.png) } b { color: #ff0000; margin: 0px }
-> a{background:url(/img/@property.png) } b { color: #ff0000; margin: 0px }
2. Custom property declarations (reported as I3). The matcher accepted only
"{" or ";" as the preceding character, so a preserved comment between the
"{" and the declaration made the value ordinary again:
:root{/*! v1 */--brand:#ff0000} -> --brand:red
:root{/*! x */--pad:0px} -> --pad:0
The second is the damaging one: calc(var(--pad) + 1px) needs the unit.
3. The at-directive lowercasing pass. Also context-free, and URL paths are
case-sensitive on essentially every server:
url(/img/@MEDIA.png) -> url(/img/@media.png)
4. The `@charset` hoisting pass. The literal text inside an unpreserved url()
was hoisted out of the URL to the top of the stylesheet:
a { background: url(/x/@charset "y";) }
-> @charset "y";a{background:url(/x/)}
The second `@charset` pass is anchored at start-of-input with only whitespace
allowed before it, so it is already boundary-safe and is unchanged. The empty-
rule `@layer` matcher requires "{}" immediately after the prelude, which cannot
occur inside a URL or any other value, so it is not exposed either.
`startsAtBoundary` accepts the start of the stylesheet, one of a given set of
boundary characters, or a preserved-token placeholder. That last part is not
optional and is what I3 was missing: by the time these passes run, a leading
comment is already a placeholder. Note the placeholder keeps its delimiters -
a preserved comment reads "/*" + placeholder + "*/" and a preserved string
keeps its quotes - so the detector steps over the closing delimiter first.
Getting this wrong initially regressed `/*! keep */@media screen{...}`, which
now has its own test.
Tests: eight cases in ModernCssTest, covering each of the four defects and,
for each, the legitimate case the boundary check must not break - an at-rule
after a preserved comment, a real leading `@charset`, and `calc(1px --2px)`,
whose "--2px" must still not be read as a declaration.
…enerator methods
Five fixes in MungedCodeGenerator, plus one in ScopeBuilder that the first of
them made necessary.
1. Default and rest parameters were silently dropped. `visitParameterList`
emitted the bare binding name and ignored everything around it:
function f(a=1){ return a; } -> function f(a){return a;}
function f(...args){ return args.length; } -> function f(args){...}
The first changes `f()` from 1 to undefined; the second turns an array of
the trailing arguments into a single positional parameter and changes
`f.length`. Rhino spreads a parameter's syntax across three places -
`getParams()` (binding target only, a rest parameter appearing as a plain
`Name`), `hasRestParameter()` (a flag on the function), and
`getDefaultParams()` (a flat, alternating list of ORIGINAL parameter name
and default expression) - and this read only the first.
A destructuring pattern's default is recorded nowhere at all, so
`function f({b}={})` and `function f({b})` produce identical parameter
nodes. The source text is consulted to tell them apart: the plain pattern
is reproduced exactly, and the one with a default throws rather than emit a
parameter list quietly missing it.
The default expression is live code, so it is visited rather than printed.
That exposed two things Rhino's side-list storage hides:
- The expression has no parent link back to the function, so
`findScopeForVariable` walked straight off the top and resolved every
name against the GLOBAL scope: `function f(alpha, beta=alpha)` emitted
`function f(b,a=alpha)`. The missing link is restored before visiting.
- `ScopeBuilder` never traversed default expressions either, so a name read
there was not a recorded use and an `eval` there did not disable munging.
It now visits them with the function's own scope. This only became
reachable once defaults stopped being dropped.
The single-parameter arrow shortcut drops the parentheses, which would have
taken a `=1` with them; it is now only taken when there is no default.
2. Redundant double braces. Rhino wraps a loop, if- or do-body that declares
anything in a `Scope`, which does NOT extend `Block`, so the
`body instanceof Block` check missed it and wrapped an already-braced block
in a second pair: `for(...){f();}` came out as `for(...){{f();}}`. Checking
the node's TYPE catches `Block` and `Scope` alike, and correctly excludes
`AstRoot`/`FunctionNode`, which are `Scope` subclasses reporting
SCRIPT/FUNCTION. Measured on jQuery 1.6.4: 106,970 -> 104,770 bytes, `{{`
count 1,100 -> 0, `node --check` clean, goldens unchanged.
3. Generator object methods crashed. Rhino wraps a generator method's key in a
`GeneratorMethodDefinition` whose type is `Token.MUL`, so
`var o = { *gen(){ yield 1; } };` reached the infix path and died with a
`ClassCastException`. Handled in the object-literal path, computed keys
included.
4. The getter and setter branches failed open: they emitted `get `/`set ` and
the key, then the parameter list and body only `if (right instanceof
FunctionNode)`, so a non-FunctionNode right-hand side produced `{get x}`,
which is invalid. They now go to the fallback instead of emitting a
truncated property. The three method branches are one method now, since
they differ only in their prefix.
5. `stripComments` strips line comments before block comments, so a `//`
comment whose own text contains `/*` cannot open a block comment that runs
past the end of the line and swallows a genuine `?.`.
Tests: `ParameterListTest` is the round-trip table - seven forms that compress
correctly (including default-expression munging, enclosing-scope resolution,
and eval-in-a-default), three that fail loudly. `ModernJsTest` gains brace
tests in both directions, including the dangling-else binding and empty loop
bodies, plus the two generator-method cases.
The two `ModernJsTest` labelled-statement tests hard-coded `{{f();}}` in their
expected strings while actually testing labels and semicolons; their expected
values are updated and their intent is unchanged. Three `JsOutputSyntaxTest`
scanner comments cited `if(x){{}}` as real generator output; the comments stay
correct in substance but their worked examples are now written as the output
the generator actually produces.
… filter
Two halves of the same problem: the suite could not tell "the output parses"
from "the output means the same thing", and five golden pairs were excluded
from every test by a filename convention documented nowhere.
DifferentialExecutionTest runs each of 25 small, self-contained, deterministic
scripts under node twice - once as source, once compressed - and compares
stdout plus exit status. Seeded from the defects this release found, all of
which produced output that parses: the "??" un-munging, the deleted "?.",
optional-chain widening, the "a + +b" operator merges, dropped default and
rest parameters, "yield*", labelled break, generator methods, eval/with
protection, and the --line-break token splits at column 20.
It immediately found a defect nobody had reported: a shorthand property is one
identifier serving as BOTH the property key and the binding, so munging it
renamed the key with it.
function f({b}){ return b; } called with {b:7} -> undefined
function f(){ var longLocalName = 7; return { longLocalName }; }
-> property renamed to the munged name
Both compress to output that parses cleanly and simply returns the wrong
thing, in an object literal and in a destructuring pattern (including a
destructured parameter) alike. Fixed by expanding to "b:a" - key kept, binding
munged - and only when the two actually differ, so an unmunged shorthand stays
shorthand and costs nothing. jQuery output is byte-identical at 104,770.
The "_" filter is removed from all three test classes that carried it
(JsGoldenFileTest, JsOutputSyntaxTest, CssGoldenFileTest - the last had no
matching fixtures, so it only removed the convention). The five JS pairs it
hid are now listed in KNOWN_FAILURES with measured reasons, re-checked after
this wave's fixes rather than copied from the report:
- _munge.js: the "a:nomunge" hint is ignored AND emitted as a live string
statement. Its golden also looks wrong - it munges "var w = window" to "a"
while preserving a parameter also named "a", so its "a.alert(...)" calls
alert on the parameter, not on window. Ours keeps those distinct.
- _string_combo.js: string-literal merging absent.
- _string_combo2.js, _string_combo3.js: the ";}" difference only.
- _syntax_error.js: quote-character normalisation absent.
Correcting the report on one point: it predicted the double-brace fix would
make _string_combo3.js match. It does not. The "{{" is gone, but ";}" was a
second, independent cause, so the fixture stays quarantined.
Per ruling R25 the underlying optimisations (";}" stripping, string merging,
quote choice, nomunge hints) are Release 2 work and are recorded, not
implemented.
The jquery-1.6.4.js quarantine reason is rewritten from figures measured after
this wave (see the next commit's honesty pass for the rest): both stated causes
in the old text were false. The golden contains BOTH "/*!" banners, 537 bytes,
byte-identical to ours - accounting for zero of the gap, not 171 bytes - and
the names said to differ "never in length" differ by 1,280 characters over an
identical 18,037 identifier tokens, so our short-name allocation is genuinely
worse rather than merely different.
JsOutputSyntaxTest now covers 9 JS fixtures rather than 4; all five newly
included ones pass both node --check and the comment-injection scan.
Every number below was re-measured after this wave's fixes rather than carried
over.
CHANGELOG:
- the optional-chaining bullet read as an unconditional guarantee. It was true
for its own example but not for chains inside a node type that reached the
toSource() fallback, where "?." was deleted outright. Qualified, with a
pointer to the new entry that closes it
- "custom property values preserved verbatim" was false when a preserved
comment sat between the declaration and its "{" or ";". Qualified and the
fix recorded
- "node --check against every compressed fixture" ran against 4 of 9 JS
fixtures. It now genuinely runs against all 10
- "66 golden fixture pairs (62 CSS, 4 JS)" - there are 72 (62 CSS, 10 JS), of
which 64 execute and 8 are quarantined, plus 3 disabled by the .FAIL
convention
- "328 tests" is now 457 (2 skipped; 422 execute without node on PATH)
- added entries for everything this wave fixes, plus a "Documented" section for
the three options that are accepted and ignored
README:
- --line-break 0. The report said this was a no-op; that is only true of the
JavaScript path, which guards on "> 0". CssCompressor guards on ">= 0", so
--line-break 0 really does give a line break after each CSS rule, exactly as
documented. Verified both. Corrected to say so rather than deleting a claim
that turned out to be half right
- --preserve-semi, --disable-optimizations and -v/--verbose marked NOT
CURRENTLY IMPLEMENTED, matching new @PARAM javadoc on both compress()
overloads. Documentation only, per ruling R25
- the "nomunge" hints section claimed "the hint itself disappears from the
compressed file". It does not: the symbols are munged anyway AND the hint is
emitted as a live string statement (this is what _munge.js records)
ES6SupportTest.testParseDefaultParams and testParseRestParams called
parseSource and asserted assertNotNull(ast) - they never invoked the
compressor, but sat among 44 tests that do, so the class read as evidence of
support for syntax the generator silently broke. Now that it reconstructs both,
they are real round-trip tests named for what they check.
Correcting the report on the three "orphan" goldens (M6). Its premise is wrong
in two ways. `hsla-issue81.css`, `issue172.css` and `rgb-issue81.css` DO have
sources: the repository's .FAIL convention disables a fixture by renaming its
SOURCE, leaving the .min golden under its ordinary name, which is exactly why
they look orphaned. The convention is documented in
src/test/resources/README.md and docs/TESTING.md, and suite.sh pairs the two.
Deleting the .min files would have destroyed the record of the expected output
for three known bugs.
What suite.sh actually asks for is the opposite, and applies to a fourth file:
"Test passed, please remove the '.FAIL' from the filename". issue71.js.FAIL now
matches its golden byte for byte, so its suffix is removed and it becomes a
live fixture (JsGoldenFileTest 2 -> 3 executed). The three CSS ones still fail
and stay disabled, with their actual current output recorded in docs/TESTING.md
so the next reader does not have to re-derive it. Both README files now say not
to delete a .min whose source is present under a .FAIL name.
Follow-up to the Phase 3 boundary work, acting on the reviewer's instruction
not to widen the predicate by example. Enumerating the preservation passes
rather than reasoning from reproductions turned up a false positive I had
introduced.
Three placeholder textual forms exist by the time the at-rule and declaration
scans run, and only one can legitimately precede either construct:
- "/*" + placeholder + "*/" - a preserved "/*!" banner, the Mac/IE5 backslash
hack, or the IE7 ">/**/" hack. REAL: a banner can sit between "{" and a
declaration. This is the I3 case.
- a quoted placeholder - a preserved string literal. NOT a boundary. A string
abutting a declaration or at-rule ("a{content:\"x\"--y:1}") is not valid CSS,
and a string in a value is followed by ";" or "}", which is already a
boundary.
- a bare placeholder - the "\9" hack, or the inside of a preserved "url(...)".
NOT a boundary. "\9" ends a declaration value so ";" or "}" follows it, and a
bare placeholder inside "url(...)" is followed by ")".
The detector accepted the quoted form too, which I had added for consistency
rather than because anything required it. Measured consequence:
a{background:url(/x/"y"@property.png)}b{color:#ff0000;margin:0px}
-> a{background:url(/x/"y"@property.png)}b{color:#ff0000;margin:0px}
Rule "b" left entirely unminified - the I2 symptom, reintroduced through a
different door by the fix for it. Requiring BOTH comment delimiters closes it.
Also recorded, because it is the reason the predicate must not be widened
further: an ordinary (non-preserved) comment never reaches this test at all.
The "kill the comment" pass deletes it whole, "/*" and "*/" included, so "{"
ends up directly adjacent to what follows. That is why routine CSS such as
":root{/* note */--pad:0px}" was never affected by I3, and it now has a test so
it cannot start being affected.
Tests: four cases in ModernCssTest - the two rejected forms, the ordinary
comment, and the IE7 empty-comment hack as a guard on the narrowed detector.
461 tests, 0 failures.
The previous fix covered `{b}` and stopped one character short of `{b = 1}`.
Rhino reports `isShorthand() == false` for shorthand-with-default, so the
expansion never ran, and in that path the SAME Name object is both
`prop.getLeft()` and the `Assignment`'s left - so `visitName`'s property-key
guard fired for the BINDING too, emitting it un-munged while every reference to
it in the body was munged normally. A declaration and its uses under different
names.
Measured before, in all three positions:
function f(o) { var someKey; ({ someKey = 5 } = o); return someKey; }
-> function f(b){var a;({someKey:someKey=5}=b);return a;}
source "5 9" -> compressed "undefined undefined" SILENT, node --check clean
function f({ someKey = 5 }) { return someKey; }
-> function f({someKey:someKey=5}){return a;} ReferenceError
function f(o) { var { someKey = 5 } = o; return someKey; }
-> function f(b){var {someKey:someKey=5}=b;return a;} ReferenceError
After, all three run "5 9" like their sources:
({someKey:a=5}=b) function f({someKey:a=5}) var {someKey:a=5}=b
The discriminator is object identity, verified against Rhino 1.8.0 rather than
assumed: for `{b = 1}` the Assignment's left IS `prop.getLeft()`, while
`{k: b = 1}` has an identical node shape with two DISTINCT Name objects,
because there the key and the binding really are different identifiers. That
distinction is what lets one branch handle the shorthand form without touching
the non-shorthand one.
Fixed in `visitObjectLiteral`, which all three positions route through - the
destructuring pattern is an ObjectLiteral in each - so this is one change to
the shape rather than three to the positions. When the binding is not munged
the original `{b=1}` form is kept, so nothing is expanded for nothing.
Also handles a shorthand property whose left is not a Name: previously it fell
through every branch and emitted nothing at all. Not known to be reachable, but
silent truncation is the wrong direction.
Tests: three exact-output cases per position, plus the two neighbouring forms
that were already correct (`{k: b = 5}`, `[b = 5]`) so a future change to the
discriminator cannot quietly break them, plus the unmunged case. Five new
differential cases run all three broken forms and both controls under node.
472 tests, 0 failures.
…acy forms
Four fixes, all in the same class as the round's Critical: constructs emitted
wrongly rather than caught by the fallback.
1. A trailing array elision lost its slot. Commas in an array literal are
separators, so a trailing one is not an element - "[a,b,]" and "[a,b]" are
both length 2 - which means a trailing hole needs an extra comma of its own.
The separator-only loop never emitted it:
[, , alpha, , ] source [null,null,7,null] length 4
emitted [,,b,] length 3
Silent, parses, pre-existing. Rhino's element list already models this
correctly (it counts a trailing hole, does not count a trailing separator,
and matched JS `length` in all seven forms checked), so the list size is the
length to reproduce. Verified in node that every form now round-trips with
both the same contents and the same length.
2. BigInt literals had no handler. Harmless in the lenient path - a leaf with
no identifiers inside, so toSource() round-trips it - but strict mode could
not compress ANY file containing one, which matters because strict mode is
what Release 2 is meant to lean on. Emits getValue(), the source text, so
"0xffn" is not needlessly normalised to "255n" the way toSource() does.
3. "for each (var b in a)" put its keyword in the wrong place, emitting
"for(var a each in b)" - which this compressor's own parser rejects
("missing ; after for-loop initializer"). Invalid output with exit 0. The
keyword goes before the parenthesis. Its own output now re-compresses
cleanly.
4. "catch (e if e instanceof TypeError)" silently dropped its guard, widening
the catch to every exception. Rhino exposes it via getCatchCondition().
Both legacy forms are Mozilla-only and unsupported everywhere, which is why
they were left; but they are emitted wrongly rather than sent to the fallback,
so the strict tripwire cannot see them - that is what makes them worth the six
lines.
On the fallback surface: sweeping the constructs Rhino 1.8.0 accepts found SIX
unhandled node types, not the four reported - DOTDOT (a..b) and DOT-as-
XmlMemberGet (a.@b) were missing from the list. With BIGINT now handled the
remaining six are ARRAYCOMP, GENEXPR, XML, REF_NAME, DOTDOT and DOT, all
Rhino/E4X legacy no browser supports. They stay throwing under strict mode and
are now enumerated in the STRICT_PROPERTY javadoc and pinned by a test, so a
seventh appearing is a real change rather than a gap in the probe.
Tests: 8 exact-output cases for the elision rule in both directions, BigInt
form preservation, and the two legacy forms; 6 strict-mode cases pinning the
unhandled list; 2 new strict-mode BigInt rows; 4 differential cases covering
array .length and BigInt arithmetic. 491 tests, 0 failures. jQuery output
unchanged at 104,770 bytes.
…onest
Two rulings from the re-review, neither of which changes what is compressed.
R28 - do not contort differential testing into a compression assertion.
Confirmed the mutation result rather than taking it on trust: reverting the D1
brace change leaves DifferentialExecutionTest 34/34 green. That is inherent -
"{{f();}}" and "{f();}" behave identically, so a class that compares source
behaviour against compressed behaviour can never detect a missed optimisation,
and a compressor that does nothing at all trivially agrees with itself.
So the class is left alone and the claims around it are corrected:
- The class docblock now states what it cannot detect by construction
(under-compression, a dead munger, missed optimisations) and names the other
half of the net that does: the golden fixtures pin exact bytes, and
ParameterListTest/ModernJsTest pin exact output. Neither half is "the safety
net" on its own.
- Two cases carried a comment calling them guards for the D1 change. They are
not, and none could be. The comment now says so, names what actually pins D1
(eight ModernJsTest exact-output tests, measured by reverting it), and keeps
the cases under an honest description - loop and label semantics are still
worth running.
R29 - a missing tool may skip; a broken tool must fail. Both probes were
`catch (Exception e) { return false; }`, so a node that was present but broken,
sandboxed or hanging disabled the entire net silently and still reported BUILD
SUCCESS. Same shape as the "_"-prefix filter this release removed.
New NodeRuntime helper, shared by both classes. Exactly one outcome skips -
node not on PATH. Present-but-non-zero-exit, wrong output, or not finishing a
trivial script within a bounded 60s all throw. Measured:
broken node (exit 126) -> 34 failures, "node is on PATH but failed a
trivial script ... Refusing to skip"
hanging node -> 34 failures in 5.1s at a 5s timeout, rather
than hanging the build
The probe caches its failure as well as its success; without that a hanging
node re-probed once per case and spent 34 x the timeout before failing (170s
measured) instead of 5.1s.
Skips are now legible. @EnabledIf disabled whole methods, so 35 real executions
hid behind 3 skip lines - "Skipped: 5" for a build that had actually stopped
checking anything with node. A per-case assumption makes the count the real
count:
without node, before: 429 run, 5 skipped
without node, after: 491 run, 46 skipped
Process handling is bounded and cannot deadlock: both streams go to files
rather than pipes, so a script that outwrites the pipe buffer cannot hang, and
every wait has a deadline.
One deviation from the brief, deliberate. It asked for redirectErrorStream on
the differential runner; I redirect stdout and stderr to SEPARATE files
instead. That fixes the hang just as well, and merging would have broken the
comparison: a stack trace carries file names and line numbers that legitimately
differ between the source run and the compressed run, so every throwing script
would report a spurious difference. Exit status is still compared, so a script
that throws where its source did not is still caught. "node --check", which has
no such comparison, does merge.
491 tests, 0 failures, 2 skipped with node; 46 skipped without.
Documentation, measured after the round's three fix phases.
I-6. The CHANGELOG listed `@property` among at-rule names "now normalised to
lowercase". It is not: the whole block is replaced by a preserved token before
the lowercasing pass runs, so `@PROPERTY --c {...}` passes through unchanged.
Verified, along with the other five names in the claim, which do lowercase.
Whole-block preservation is deliberate, so the claim is corrected rather than
the behaviour.
I-5. Test counts were stale. Now measured after this round: 492 tests, 3
skipped with node; 46 skipped without it.
Minors taken:
- M-1. The CSS boundary javadoc reasoned only about declarations. Records the
one accepted cost of narrowing to comment placeholders: an at-rule directly
after a preserved `@property` block is not recognised, so
`@property --x{...}@media{...}` keeps `@MEDIA`. At-rule names are ASCII
case-insensitive, so this is a missed optimisation, and the alternative is
what reopened the corruption it fixed.
- M-2. Phase 6f converted two parse-only tests and left four. All four are now
round-trip tests, and the two `assertNotNull(result)` tests next to them
assert their actual output - one of which pins an array-destructuring hole,
which the elision fix in this round touches.
- M-3. `assertEveryLineBreakIsAtAStatementBoundary` asserted nothing when no
line break was inserted: with `addLineBreaks` made a no-op it stayed green,
and only the exact-output tests caught it. It now requires a break to exist
before checking where breaks landed.
- M-4. A size and shape pin for jquery-1.6.4.js: 104,770 bytes, zero `{{`,
1,259 `;}`. Quarantining it left the only large real-world fixture with no
byte-level guard - reverting the brace fix, worth 2,200 bytes on this exact
file, left the golden test 3/3 green.
- M-6. `-v/--verbose` was described as accepted and ignored. Wrong about the
flag though right about the `compress(...)` parameter: the CLI does read it,
for one informational line when an unsupported charset is replaced.
- M-7. `compressedOutputParses` returned early for the fixture whose SOURCE
node rejects, reporting PASSED, so the run showed 10 node --check executions
where 9 happened. Now an explicit skip, and the CHANGELOG says 9 of 10.
- M-9. StrictNodeCoverageTest's cases assert only "did not throw", which is
correct for a tripwire but must not be counted as output coverage. Said so in
the class javadoc, with what does cover the output.
Minors skipped: M-5 (the three `.css.FAIL` fixtures are excluded by the `*.css`
glob rather than a named list). The convention, the per-fixture reasons and
their current output were written into docs/TESTING.md and both READMEs last
round, which is where a reader looking at those files will be; adding a test
that asserts what a deliberately-disabled fixture currently does would pin
behaviour nobody has decided is correct. M-8 is not skipped - both halves were
fixed in this round's second commit.
Comment collection is the first pass over the stylesheet, so it has to
understand CSS structure itself. It was a bare `indexOf("/*")` loop with an
`endIndex = totallen` fallback, and that single pass produced both reported
defects. They are one defect: the scan was context-free and ran before string
and URL handling.
A. A comment-looking span inside a string or an unquoted url() was collected as
a comment. The placeholder then sat mid-value while LOOKING exactly like a
leading banner comment, which defeated startsAtBoundary at all four call
sites. Measured before:
url(/x/*!k*/@property.png) -> following rule left entirely unminified
url(/x/*!k*/@MEDIA.png) -> lowercased; URL paths are case-sensitive
url(/x/*!k*/@charset "y";...) -> encoding invented from a URL fragment,
and the fragment deleted from the URL
url(/x/*!k*/--y:0px) -> following rule left unminified
Narrowing the predicate a third time could not have fixed this, and the
reviewer's framing is why: a placeholder's shape records how it was created,
never where it sits. The boundary decision has to come from the original
stylesheet's structure, so that is where the fix goes.
B. An unterminated "/*" replaced everything to end-of-input with a marker the
later "kill the comment" pass could not match, because that pass looks for
the closing delimiter. Truncated stylesheet, following rules gone, internal
scaffolding emitted into shippable CSS, exit 0:
a{content:"/*"}b{color:#ff0000;margin:0px}
-> a{content:"/*___YUICSSMIN_PRESERVE_CANDIDATE_COMMENT_0___
a{background:url(/img/*/thumb.png)}b{color:#ff0000} c{content:"*/"}
-> a{background:url(/img"}
Both fixed at the root by scanning structurally: strings and url() tokens are
stepped over, so their contents can never be mistaken for a comment. Both of
B's reproductions are covered by that alone - neither "/*" was ever in comment
position.
A genuinely unterminated comment - outside any string or URL - now throws,
naming the offset and how many characters would have been discarded. Browsers
consume such a comment to end-of-input, so the stylesheet is already broken for
the author either way; reproducing that here would mean a minifier silently
discarding the rest of the file with a success exit code, which is exactly the
corruption this pass exists to stop. This is the R20 trade, applied to CSS.
Adversarial checks run against the new scanner, all correct: a comment inside a
QUOTED url; an escaped quote before "/*"; "url(" inside a string; string-looking
text inside a comment; uppercase URL(; an escaped ")" inside a url; a comment
immediately after a url; "url(" inside a comment; an identifier ending in "url"
("myurl(") not being taken for a url token; and a data URL carrying an
unterminated "/*", which is the realistic trigger for B and is now not a comment
at all rather than an error.
One residual, recorded not fixed: after an unterminated STRING, later comments
are not collected, so they are emitted rather than stripped. That is a missed
optimisation on malformed input, and strictly better than the previous
behaviour, which truncated.
The javadoc claim that "an ordinary comment never reaches this test at all: it
is deleted whole" was false for exactly case B. Corrected to say it holds for
terminated comments, to say what used to reach it, and not to restate it as a
settled invariant.
All 62 CSS goldens unchanged. 503 tests, 0 failures.
9b56de5 replaced a context-free comment scan with a context-free url() scan. skipUrlToken treats every "url(" as a raw url-token and looks for the first ")", stepping over strings and backslash escapes but not over comments, so a comment inside a QUOTED url() desynced it. Per CSS Syntax Level 3 4.3.4, that raw treatment is only correct for the unquoted form. When "url(" is followed - after any amount of whitespace - by a quote, the tokenizer emits an ordinary <function-token>, and its contents are ordinary tokens: strings, comments, and the closing ")". url("a.png" /* n */) is valid CSS that every browser accepts. So the split belongs in startsUrlToken, which now declines the quoted form and lets the main loop scan it with the normal rules. Three failures, one cause. Measured before, all exit 0 unless noted: .hero{background-image:url("a.png" /* legacy: url(b.png) /* keep */)} .nav{color:#ff0000} .footer{margin:0px} -> .hero{background-image:url("a.png" /* legacy:url(b.png))}.nav{...} An unterminated "/*" in shippable CSS. Browsers consume such a comment to end-of-input, so .nav and .footer are gone. This is the blocker: it never reaches the loud path, it just writes plausible-looking bytes over a good artifact. a{background:url("x" /* ) " */)}b{content:"/*"} -> exit 1, "unterminated CSS comment ... at offset 43", pointing inside the string in b{}. Valid CSS reaching the throw, which falsified the javadoc's "outside a string or URL" claim. @import url(...) the same. a{background:url("x.png" /* n */)} -> the comment shipped. The common case, needing no ")" and no nesting. All three now match the pre-9b56de5 output, which was correct on them. skipUrlToken is unchanged, and its javadoc now records why. It steps over a quoted span inside an UNQUOTED url(), which looks like a spec deviation - a quote there is a parse error, and 4.3.14 consumes bad-url remnants to the first unescaped ")". Scanning it that way was tried and measured: url(data:image/svg+xml,<svg xmlns="a)b/*x*/c"/>) -> the token ends at the ")" inside the attribute, the collector resumes inside the URL, and "/*x*/" is deleted from it - defect A of 9b56de5, reintroduced. Ending the token late can only suppress comment collection (a leak); ending it early can delete document bytes (corruption). On malformed input the scanner errs toward emitting too much, and that is the safe direction. One new loud case: an unterminated comment inside a quoted url() now throws instead of being emitted verbatim. That input is a stylesheet a browser reads to end-of-input as a comment; the previous behaviour was to ship it silently. 509 tests, 0 failures, 3 skipped. All 61 CSS goldens unchanged.
Found by auditing the comment/string/URL scanner for the defect class this release keeps repeating - a scan that matches without knowing what part of the document it is looking at. Both of these are older than 9b56de5 and neither is a regression from it; the first one was merely hidden by the C-3 bug and would have become visible now that a comment inside a quoted url() is collected. A. A comment inside a span captured by preserveToken was emitted as internal scaffolding. url(...data:...), calc(...) and progid:...Matrix(...) are captured verbatim and put back at the very end of compress(), after the loop that settles candidate comment markers, so anything the span swallowed is invisible to that loop. Measured on valid CSS, exit 0: a{width:calc(100% /* n */ - 10px)} -> a{width:calc(100% / *___YUICSSMIN_PRESERVE_CANDIDATE_COMMENT_0___ * / - 10px)} Inside calc() it is not only ugly. respaceCalcOperators runs after restoration and reads the marker's own "/*" and "*/" as division and multiplication, spaces them out, and the declaration a browser receives is broken. The other two shapes keep their comment delimiters, so a browser drops the marker, but it is still scaffolding in shipped CSS: url('data:image/png;base64,AAA=' /* n */) progid:DXImageTransform.Microsoft.Matrix(M11=1 /* n */,M12=0) Settled where the spans are captured, with the same rule the kill-comment loop applies: a "!" comment is kept, any other is deleted. B. The --line-break pass tracked string state but not comments, so a quote inside a preserved "/*! ... */" banner was taken for a string delimiter. One unpaired quote inverts the tracking for the rest of the file. Measured at --line-break 20, exit 0: /*! say "hi */a{content:"aaaa...}bbbb"}c{color:#ff0000} -> a newline inserted at the "}" INSIDE the string literal A newline in a CSS string is a parse error, so the declaration is dropped. With an apostrophe instead, the tracker sticks "inside a string" and every later linebreak is suppressed - the harmless half of the same fault. The pass now steps over comments before it looks at quotes. Two further findings of the same class are reported, not fixed, in round4-report.md: preserveToken's regexes match calc( and progid:...Matrix( inside strings, and respaceCalcOperators matches calc( inside strings and comments. They are one problem seen from two ends - fixing either alone moves the corruption rather than removing it - and that is a design change, not a patch to land at the end of a release. 513 tests, 0 failures, 3 skipped. All 61 CSS goldens unchanged.
Confirmed unused by reading every reference in compress(), not by trusting the
warning:
endIndex assigned 0 and never read or reassigned again.
totallen assigned css.length() and never read. Both were the old
indexOf("/*") loop's cursor and its "endIndex = totallen"
truncation fallback, which 9b56de5 replaced.
sb initialised with new StringBuffer(css) and unconditionally
reassigned before any read, so the copy was allocated and thrown
away. Declared without an initialiser instead.
startIndex stays: the IE7 ">/**/" hack branch still reads it.
The third item in the brief - dead code at CssCompressor.java:448 - is not
dead. That line is the "if (end < 0)" guard whose throw rejects an unterminated
comment, and it fires: "a{color:#ff0000} /* oops" exits 1 with "unterminated
CSS comment". Left alone.
513 tests, 0 failures, 3 skipped.
The collectComments javadoc and the CHANGELOG both said the throw fires for "an
unterminated comment outside a string or URL". That was false before this round
- a comment inside a quoted url() could reach it, and did, on valid CSS - and it
would still be false as a summary now that the quoted form is scanned normally.
Replaced with the condition itself rather than a tidier claim: a "/*" reaches
the test when it is not inside a string and not inside an UNQUOTED url() token,
and throws when no "*/" follows it anywhere in the input. Inside a quoted url()
it therefore does reach the test, deliberately, because a comment there is an
ordinary comment.
Also recorded what the throw does not cover, so the next reader does not have to
rediscover it: after an unterminated string or an unclosed "url(", the scan
stops collecting, so later comments are emitted rather than stripped. Malformed
input, and a leak rather than corruption - the safe direction to fail in.
CHANGELOG: the same correction, plus entries for the quoted-url() fix and for
the two scans corrected in 05f036d. Test count measured after every other phase,
503 -> 513. The "46 skipped without node on PATH" claim was re-measured on the
final tree and still holds.
513 tests, 0 failures, 3 skipped.
…R37) Both are deferred to Release 2 by ruling R37, and both are pre-existing - byte-identical at the release base 070bdd7 in every form measured. Pinned now so they cannot drift unnoticed and, more importantly, so neither end can be "fixed" without the other end announcing itself. The tests pin WRONG output and say so. A-3: preserveToken's regexes match "calc(" and "progid:...Matrix(" inside a string, and the captured span's placeholder is never resolved, because the restoration loop reaches index 0 before the enclosing string - a higher index - is put back: a{content:"calc(1px + 2px)"} -> a{content:"calc(___YUICSSMIN_PRESERVED_TOKEN_0___)"} a[data-x="calc(1px + 2px)"] -> same substitution a{font-family:"calc(x)",serif} -> same substitution a{content:"progid:...Matrix(M11=1)"} -> same substitution A-4: respaceCalcOperators runs after token restoration, so no string, comment or URL is protected by then: /*! calc(1px+2px) */ -> /*! calc(1px + 2px) */ url(/x/calc(1px+2px)/y.png) -> url(/x/calc(1px + 2px)/y.png) The trap is recorded in the tests, because A-3 has a tempting one-line fix - resolve the nested reference in the string-preserving pass, which resolvePreservedTokenReferences already does elsewhere. Landing it alone restores "calc(1px + 2px)" into the string, whereupon A-4 respaces it to "calc(1px + 2px)": visible scaffolding becomes an invisible rewrite of the author's text, which is worse because it looks plausible. A-3 currently hides A-4's only valid-CSS string case, which is why the pair has to move together. The control assertion in the A-4 test pins that ordinary calc() respacing still works, so a future fix cannot satisfy these tests by disabling the pass. 515 tests, 0 failures, 3 skipped. No behaviour change.
Preserving a data: URL ran token.replaceAll("\\s+", "") over the whole captured
span, including the contents of the quoted string. In a base64 payload that is a
convenience. In any other payload the data is literal, so it destroyed author
bytes - silently, exit 0, on valid CSS:
url("data:image/svg+xml,<svg viewBox='0 0 24 24'><text>hello world</text></svg>")
-> viewBox='002424' and <text>helloworld</text>
viewBox's grammar is four numbers separated by white space or commas, so
"002424" is one invalid value rather than four; no browser is needed to see the
image is broken. The text node's rendered content changed outright. The
percent-encoded spelling was unaffected, which is how this survived: %20 is the
machine-generated style and the literal space is the hand-written one.
Pre-existing - byte-identical at the release base 070bdd7 - and reported as a
deferral candidate. Reversed to a fix because it destroys data rather than
leaking, which is the line this release drew.
The discriminator needed checking rather than assuming, and "strip only when
base64" turned out to be too strong. Three regions, three answers:
outside the quotes always stripped. Two goldens pin this on
NON-base64 URLs - dataurl-nonbase64-noquotes has
"url( data:...)" and dataurl-nonbase64-doublequotes
puts the quoted string on its own line - so it
cannot be conditional on the payload.
inside, base64 stripped. RFC 2397 makes ";base64" the last thing
before the comma, after any media-type parameter,
and such a payload's white space is insignificant.
Load-bearing: dataurl-base64-linebreakindata splits
its payload across three lines inside the string.
inside, anything else kept. The data is literal.
The unquoted form keeps the old behaviour of losing all its white space, and
nothing legal is lost by that: white space inside an unquoted url-token other
than at the ends makes it a bad-url-token (CSS Syntax L3 4.3.6), so the input is
already invalid.
";base64" is matched case-insensitively and only where RFC 2397 puts it, so
data:text/plain;charset=UTF-8;base64 is recognised and a ";base64" occurring
later, in the data itself, is not - that payload stays literal.
One deliberate consequence, recorded in the javadoc. A NON-base64 quoted data
URL split across lines is no longer joined. A newline inside a CSS string is a
parse error, so that input was already invalid; the join used to repair it by
accident, and the repair is the same operation as the corruption above - delete
a character the author wrote. Preserving is the safe direction.
All 61 CSS goldens unchanged, no fixture edited. 519 tests, 0 failures, 3 skipped.
…isting The lead's check of my claim that the four leaks are regressions used inputs with no comments in them, so it could not have measured what the claim was about. Pinning the comment-bearing forms so the record settles it. Measured against the release base 070bdd7, which strips the comment in all four: a{background:url(x}/* n */b{...} unclosed url( a{content:"oops}b{...}/* n */c{...} unterminated string (R33) a{content:"oops\n}\nb{...}\n/* n */\nc{...} string closed by a newline a{background:url(/x/y'.png)}/* n */b{...} stray quote in an unquoted url All four now emit the comment. So they are REGRESSIONS from 9b56de5, not pre-existing: the old context-free scan did strip these, by not knowing where it was - the same blindness that truncated whole stylesheets. Leaking a comment on malformed input is what that trade bought, and it is a leak, not a rewrite: nothing is deleted or altered. Every input is invalid CSS. R32 is the opposite case and is pinned next to them for contrast: byte-identical at base, so genuinely pre-existing. An unquoted URL whose contents spell a declaration reaches the value optimisers and has its value rewritten ("--y:0px" -> "--y:0" inside the URL); its control pins that realistic URLs do not reach it, which is why it was deferred rather than fixed. No behaviour change. 521 tests, 0 failures, 3 skipped.
The data: URL white space fix joins the Fixed (CSS) list, with the measured
before-and-after, the RFC 2397 rule that decides which payloads may lose their
white space, and the one deliberate consequence - a non-base64 quoted data URL
split across lines is no longer joined.
New "Known limitations (CSS), carried to Release 2" section, naming the whole
remaining cluster in one place. The framing is the honest close for this
release: every entry is one instance of a single defect class - a pass that runs
without knowing what region of the document it is in - and Release 1 fixed every
instance its own changes touched and every instance that destroyed data, then
enumerated the rest rather than leaving them unknown. Each has a regression test
pinning it, so none can drift or be half-fixed unnoticed.
Split into two groups, because they are not equally serious and saying so
matters more than a tidy sentence:
rewrites confined to one declaration - wrong output, not just unminified
output: calc(/progid: matched inside strings and left as scaffolding; calc()
respacing running after restoration; an unquoted URL spelling name:value
reaching the value optimisers. The first two are entangled, which is the
reason neither is fixed here rather than a judgement about severity.
comments that survive into the output - nothing altered or lost, every trigger
already-invalid CSS: after an unterminated string, after an unclosed url(,
and after a stray quote in an unquoted url().
The second group is recorded as REGRESSIONS from the structural comment scanner
rather than as pre-existing behaviour, which is what the measurements say.
Test count measured after every change, 513 -> 521. The "46 skipped without node
on PATH" claim re-measured on the final tree and still holds.
521 tests, 0 failures, 3 skipped.
…ollection (R40) 05f036d taught the --line-break pass to step over comments but not over URLs, so a "/*" inside an unquoted url-token - ordinary URL content, and correctly not a comment to collectComments - started a phantom comment that ran to the next "*/" anywhere in the file. Measured at --line-break 10, exit 0: a{background:url(/x/*p.png)} b{content:"*/z"} c{content:"aaaaaaaaaa}bbbbbbbbbb"} d{color:#ff0000} -> a{background:url(/x/*p.png)}b{content:"*/z"}c{content:"aaaaaaaaaa} bbbbbbbbbb"}d{color:red} The newline lands inside c's string literal. A raw newline in a CSS string is a parse error, so c is dropped. Valid CSS in, silent corruption out. Also reachable through an unquoted data: URL; the quoted form was never affected, because a quote opens a string region either way. Rather than adding a fourth ad-hoc step to a scanner that has now guessed wrong three times, the pass asks its question - what region is this offset in - with the same three primitives collectComments uses, in the same order: skipString, startsUrlToken/skipUrlToken, then a comment. It is now the only region model in the file. The loop also builds its output rather than mutating a buffer it is scanning, which is what lets the shared helpers apply. That removed a second corruption I did not know about, found by running a contrived case rather than re-reading the code. "}" is legal inside a url-token (4.3.6 stops only at ")", whitespace, a quote or "("), so this is valid CSS: a{background:url(/x/}p.png)} at --line-break 10, at d484f4e and before -> url(/x/} p.png)} The newline goes inside the URL, which is exactly what a url-token may not contain, so the declaration is dropped. Pre-existing, not from this range. Regions enumerated and measured, one case each: preserved comments (including a "}" inside one, the Mac/IE5 "/*\*/" pair and the IE7 ">/**/" hack), quoted strings (including an escaped quote and a "}"), quoted URLs, unquoted URLs (including "/*", "}", data: and @import forms), "myurl(" not being a url token, a leaked PRESERVED_TOKEN placeholder, and the "\9" hack - all inert or handled. Controls pin that ordinary rules still break, at --line-break 10 and 0, and that the 05f036d fix still works. Every failure mode of the three primitives ends a region LATE - an unterminated string, URL or comment runs to the end of input. Here that only produces a long line, which is why the sharing is safe in this direction: refusing to break cannot corrupt, breaking in the wrong place can. Two residuals stay in that safe direction and are unchanged: a stray quote inside an unquoted url() and an unterminated string both suppress later breaks. One thing is deliberately not shared, and the javadoc says why: collectComments throws on an unterminated comment, and this pass steps to the end instead. A comment following an unterminated string is never collected, so it reaches the output as written; throwing there would fail a compression that has succeeded. CHANGELOG: the entry claiming "--line-break no longer inserts a newline inside a string literal" was falsified by the repro above and is rewritten to describe all three faults and the shared model. Test count measured after the change, 521 -> 524; "46 skipped without node on PATH" re-measured and still holds. All 61 CSS goldens unchanged, no fixture edited. 524 tests, 0 failures, 3 skipped.
…defect (R41) Documentation and tests only. The one non-comment line in the source diff is a single-line javadoc becoming a block. d9d2ab8 asserted, in CssCompressor.java and in the CHANGELOG, that EVERY failure mode of the shared region primitives ends a region late. That is the same shape of guarantee that commit was written to replace, and it is wrong in the same way: it covers where regions end and says nothing about where they start. Corrected to the property that actually holds, stated as what was tested rather than as a property of the design: fuzzing 68,383 (source, width) pairs against an independent 4.3 tokenizer found no early end. Region STARTS are called out as the known exception. The exception, deferred by ruling R41 and now pinned: skipString is entered at any quote without either caller asking whether the quote is escaped, and "\"" is a valid identifier escape (4.3.7). The phantom region ends at the OPENING quote of the next real string, so the scan resumes inside it. Both consequences destroy data, on valid CSS, exit 0: a\"b{color:red} --line-break 10 c{content:"XXXXXXXXXX}YYYYYYYYYY"} -> newline inside c's string literal, so c is dropped a\"b{color:red} c{content:"keep /* this */ text"} -> c{content:"keep text"}, author bytes deleted Both byte-identical at 070bdd7. Pinned with two controls that guard the fix rather than the defect: without the escaped quote both cases are correct, and a backslash PAIR before a quote is NOT an escape, so "previous character is a backslash" is the wrong repair. One correction to the ruling's reasoning, which does not change its outcome. The premise that the trigger is vanishingly rare in authored stylesheets is wrong: Tailwind's arbitrary-value classes (content-['x']) emit an escaped quote in the generated selector, so the shape occurs in ordinary machine-generated CSS. I could not get destruction out of it - measured, the effect there is a span left unminified, because the destroying variants need a comment or a "}" inside a later real string and that shape has neither. So the trigger is reachable and the harm is not, which is the distinction R41 actually rests on; the CHANGELOG and the tests now say that rather than "rare". That probe did find one genuine regression from this release, pinned with the rest: after the phantom string, a following "/*!" banner is no longer collected, so its text reaches the minifier and loses the space after "!". At 070bdd7 the context-free scan collected it regardless. Cosmetic, inside a comment, but it is author text and it is new. The Known limitations section gains a third group, "Destroys data", above the existing two, and its introduction no longer claims that nothing in the list costs a declaration. 526 tests, 0 failures, 3 skipped; 46 skipped without node on PATH. All 61 CSS goldens unchanged, no fixture edited, no behaviour change.
Convert the remaining Japanese prose and in-snippet code comments in docs/ES6_MIGRATION_PLAN.md to English so all Markdown docs in the repository are English. Content, structure, checkbox states and code snippets are unchanged.
Adds seven test classes, 210 tests, all passing against this branch:
- CliOptionsProcessTest - --charset, --type inference, -m, --line-break,
exercised by forking the entry point so exit
codes and System.exit are observable
- CompressorApiTest - the four no-op options and the mungemap
overload, none of which any test had passed
- FixtureParseGuardTest - a tripwire for the two bare-return catch blocks
in JsOutputSyntaxTest that JUnit reports as
PASSED rather than skipped
- IdempotencyTest - compress(compress(x)) == compress(x) over the
whole fixture corpus, including quarantined ones
- KnownCssLimitationsTest - pre-existing CssCompressor defects, pinned as
wrong output and labelled as such; every case
verified byte-identical at the release base
070bdd7, so none is a regression here
- RobustnessTest - malformed and adversarial input handling
- UncoveredBranchGuardTest - branches with no previous execution
Test-only change; no production code is touched.
Each of these was verified by running the same input through a build of main (070bdd7): main produced the expected value and this branch did not. CSS - Empty-rule removal matched an at-rule prelude with a separate '@'-anchored alternative, and '@' is an ordinary character in a class name once escaped. ".\@container{}p{color:red}" therefore had its only '@' in the middle of a plain prelude no alternative could match; the '@' alternative matched from the '@' onward and deleting "@container{}" welded the leftover ".\" to the next rule, giving ".\p{color:red}". The prelude is now one character class and the '@layer' exception is decided on the matched text. - The prelude is also anchored to the boundary its character class already implies, which restores linear time. '@Property' blocks are preserved whole by this release, so a stylesheet of them is one long brace-free run that the unanchored pattern retried from every offset: 800 of them took 3.2s. - The custom property scan had no notion of strings or url tokens. "url(/x/;--y.png)" put a '--' straight after a ';' so it read as a declaration, the search for its ':' ran out of the URL into the next rule's "b:hover", and the value scan ran from there to end of input - one URL left a whole stylesheet unminified with exit code 0. It now uses the same region model as collectComments and insertLineBreaks. JavaScript - ScopeBuilder never declared a function's own name, so the munger handed it out as free: "function f(x){...}" beside six locals produced "var f=1" and the call beside it read the variable. A declaration's name now goes in the enclosing scope and a named function expression's in its own, which is also what makes a recursive self-call resolve to the function instead of to the outer variable. The name is reserved rather than renamed, in the declaring scope and every enclosing scope that is munged, because an outer variable munged to the same spelling is shadowed inside the function's body. jQuery 1.6.4: 104,770 -> 104,815 bytes (0.04%), shape counts unchanged, and still 137,795 -> 104,815 against main. - isStrict() only tested the property for non-null, so -Dyuicompressor.strict=false enabled strict mode. It is parsed as a boolean now; the CHANGELOG's bare -Dyuicompressor.strict is spelled =true to match. Command line - The destination was opened, and so truncated, before the compression that can fail. A stylesheet this release declines to guess at left an empty output file, and with -o pointing at the input an empty source file: 29 bytes to 0. The compressed text is produced in full first. - That refusal left main as an uncaught IllegalArgumentException; it is reported as "[ERROR] <file>: <message>" with a non-zero exit. - The per-file writer wrapped System.out and was closed after the first file, so later files in the same run wrote to a closed stream and were discarded. Pre-existing on main, fixed here because the same block was being changed. Full suite: 765 tests, 0 failures, 0 errors, 3 skipped.
The three classes held back from the previous test commit because they were
red. The fixes in the commit before this one make them pass.
- MergeGateRegressionTest - one minimal input per regression, each verified
against a build of main (070bdd7) so the claim
"main is right, this branch is not" is measured
rather than asserted. Two of the eleven are
labelled pre-existing so the regression count
stays honest.
- StrictModePropertyTest - what values of -Dyuicompressor.strict mean.
Saves and RESTORES the property rather than
clearing it, so it cannot disable strict mode for
classes scheduled after it in the same JVM.
- CommandLineProcessTest - the process contract, by forking a JVM: exit
codes, --help, a missing file, an unparseable
script, stdout compression, the -o pattern, and
what a refusal does to the files involved.
Full suite: 765 tests, 0 failures, 0 errors, 3 skipped.
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
1 participant
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.
Summary
Release 1 of the modernisation work: correctness fixes for modern CSS and JavaScript, plus a test suite strong enough to detect regressions in the larger Release 2.
The theme throughout is replacing silent output corruption with either correct output or a loud failure. Most defects fixed here produced output that parsed cleanly and behaved differently from the source.
Java 11 remains the floor, and the public API is unchanged —
JavaScriptCompressor(Reader, ErrorReporter), its 6-arg and 8-argcompress(...),CssCompressor(Reader)and its 2-argcompress(...)all keep byte-identical signatures, so downstream callers need no change.Fixed — JavaScript
toSource()fallback re-emitted a subtree's original source, so identifiers kept their old names while their declarations were renamed — the code then read and wrote globals. Affected??,??=,||=,&&=,**=anddebugger. A strict mode (system property) now makes unhandled node types fail loudly instead, and the remaining unhandled types are enumerated in the javadoc.function f({b}){return b;}called with{b:7}returnedundefined. Also fixed for shorthand carrying a default ({b = 1}) in all three positions — destructuring assignment, destructured parameter, andvardestructuring.f(...args)becamef(args), changing both behaviour andf.length.[, , a, , ]emitted as[,,b,].--line-breakno longer splits identifiers or string literals.yield*, labeled statements, optional catch binding, generator object methods, template and regex literals, and three operator-merge hazards (including- -abecoming--a).evalandwithagain suppress munging, restoring the guarantee the README has always made.//and Annex B<!--.Fixed — CSS
/*without knowing whether it was inside a string or a URL. An unbalanced/*—content:"/*"is valid CSS — replaced everything to end-of-input with an internal placeholder, truncating the stylesheet at exit 0. Comment collection, the--line-breakpass and the at-rule/declaration boundary tests now share one region model.@property,@charsetand the at-directive lowercasing pass matched context-free, so text inside aurl()could disable minification of the following rule, case-fold a case-sensitive URL path, or hoist an invented@charsetto the top of the stylesheet.data:URL is no longer deleted — this was corrupting inline SVG (viewBox='0 0 24 24'becameviewBox='002424'). Base64 payloads are still joined, per RFC 2397.@layerblocks are kept (they are cascade-meaningful) while other empty at-rules are still removed.<time>values keep their units. CSS Values and Units Level 3 grants the unitless zero to<length>only, sotransition-duration: 0would be invalid.Tests and tooling
nodetwice, as source and as compressed, comparing stdout.node --checkproves output parses; this proves it still means the same thing. It found a silent corruption on its first run.nodeskips honestly and reports the count; a broken or hangingnodenow fails the build instead of silently disabling the checks.Compression
jQuery 1.6.4 output improves from 137,798 to 104,770 bytes (−24.0%), caused mainly by a scope-traversal gap that left most nested functions unmunged. All 61 executed CSS golden fixtures are unchanged, and no golden file was edited in this branch.
Known limitations carried to Release 2
Documented in the CHANGELOG with reproductions and pinned by tests so they cannot be "fixed" from one end without the other becoming visible:
calc(inside a string is preserved by one pass and rewritten by another. Fixing either end alone relocates the corruption rather than removing it.}inside a later string.;}stripping, string-literal merging, quote-character choice, and short-name allocation quality — together about 2.5 KB on jQuery.Verification
mvn -o clean package: BUILD SUCCESS, 526 tests, 0 failures, 3 skipped. The skips are class-syntax cases Rhino cannot parse at any version, labelled with an accurate reason.