-
-
Notifications
You must be signed in to change notification settings - Fork 727
docs(transformer/styled-components): add comments about CSSMinifier
#12197
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
docs(transformer/styled-components): add comments about CSSMinifier
#12197
Conversation
CodSpeed Instrumentation Performance ReportMerging #12197 will not alter performanceComparing Summary
|
Merge activity
|
overlookmotel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from the typo Graphite's spotted (it finally got something right), all looks good. Thank you for documenting this.
| // SAFETY: | ||
| // This is safe because template literal has ensured that `quasis` always | ||
| // has one more element than `expressions`, and the `CSSMinifier` guarantees that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What makes it true that "template literal has ensured that quasis always has one more element than expressions"? How is that ensured?
If in some other transform, I do expressions.push(...) without corresponding quasis.push(...) by accident, this invariant is broken, and quasis.set_len(expressions.len() + 1) is UB. Safety invariants should be local and provable. "SAFETY: There are no bugs in the rest of the codebase" isn't very convincing!
Also, there is a
assertin codegen to ensure thatquasisis always one greater thanexpressionsoxc/crates/oxc_codegen/src/gen.rs
Lines 2091 to 2092 in c003eba
debug_assert_eq!(self.quasis.len(), self.expressions.len() + 1);
- It's
debug_assert!notassert!. That's no good as a safety guarantee. - It's in codegen. Even if it was
assert!, it's too late. We's already invoke UB before we get to that.
In addition, I found that we can use
truncate()instead ofset_len. Both are the same, buttruncateis a safe function.
truncate would be fine - it might panic if some other code has broken the rule, but won't cause UB.
Or we could add an assertion at top of the function assert!(quasis.len() == expressions.len() + 1), and then use unsafe set_len.
But... this PR is mainly about adding comments. Let's leave it as is here, and address it in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I'd quite like to make this a type system invariant by changing TemplateLiteral to:
pub struct TemplateLiteral<'a> {
pub span: Span,
pub lead: Vec<'a, TemplateElementAndExpression<'a>>,
pub tail: TemplateElement<'a>,
}
pub struct TemplateElementAndExpression<'a> {
quasi: TemplateElement<'a>,
expression: Expression<'a>,
}Then we can stop worrying about this! Rust will prevent us making mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Dunqing Ping just to make sure you see this. I had to mark it as "resolved" to merge the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new type looks good, the current one is probably to align with the ESTree. I will add that assertion later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
b8b68dd to
e22c81f
Compare
…#12197) Added some comments to describe `CSSMinifier`, and clarify why `quasis.set_len(expressions.len() + 1);` is safe. Also, there is a `assert` in codegen to ensure that `quasis` is always one greater than `expressions` https://github.com/oxc-project/oxc/blob/c003ebad60970d27aae48b488ff361ef2b612379/crates/oxc_codegen/src/gen.rs#L2091-L2092 In addition, I found that we can use `truncate()` instead of `set_len`. Both are the same, but `truncate` is a safe function.
e22c81f to
86eb108
Compare

Added some comments to describe
CSSMinifier, and clarify whyquasis.set_len(expressions.len() + 1);is safe.Also, there is a
assertin codegen to ensure thatquasisis always one greater thanexpressionsoxc/crates/oxc_codegen/src/gen.rs
Lines 2091 to 2092 in c003eba
In addition, I found that we can use
truncate()instead ofset_len. Both are the same, buttruncateis a safe function.