Skip to content

Commit

Permalink
fix: comments gen regression (#5003)
Browse files Browse the repository at this point in the history 
try to fix: rolldown/rolldown#2013
1. Before we only considering the ast is untouched, but considering the
scenario.
```js

const a = /*__PURE__*/ test(),
//    ^^^              ^^^^^^ is removed during transform
	b = a();

```
Then according to the previous algorithm, `PURE` will attach to `b =
a()`
2. Now, we try to attach comments as much as possible unless the
comments are separated by comments, for the case above, `PURE` will not
be attached to `a()` since the content between `b = a()` and `/*
__PURE__*/` is not all whitespace.
3. we added back `MoveMap`, for the special case 
```js
/*__NODE_SIDE_EFFECTS__*/ export const c = 100;
// ^^^^^^^^^^^^^^^^^^^^^         should be attached to first declarator,
//                        ^^^^^^ are not whitespace

```
  • Loading branch information
@IWANABETHATGUY
IWANABETHATGUY authored Aug 20, 2024
1 parent cd9cf5e commit b7db235
Show file tree
Hide file tree
Showing 6 changed files with 113 additions and 34 deletions.
1 change: 1 addition & 0 deletions Cargo.lock
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 14 additions & 0 deletions crates/oxc_ast/src/trivia.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,20 @@ impl Comment {
let span = Span::new(start, end);
Self { kind, span }
}

pub fn real_span_end(&self) -> u32 {
match self.kind {
CommentKind::SingleLine => self.span.end,
// length of `*/`
CommentKind::MultiLine => self.span.end + 2,
}
}

pub fn real_span_start(&self) -> u32 {
match self.kind {
CommentKind::SingleLine | CommentKind::MultiLine => self.span.start - 2,
}
}
}

#[derive(Debug, Clone, Copy, Eq, PartialEq)]
Expand Down
9 changes: 5 additions & 4 deletions crates/oxc_codegen/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,11 @@ oxc_sourcemap = { workspace = true }
oxc_mangler = { workspace = true }
oxc_index = { workspace = true }

bitflags = { workspace = true }
nonmax = { workspace = true }
once_cell = { workspace = true }
daachorse = { workspace = true }
bitflags = { workspace = true }
nonmax = { workspace = true }
once_cell = { workspace = true }
daachorse = { workspace = true }
rustc-hash = { workspace = true }

[dev-dependencies]
oxc_parser = { workspace = true }
Expand Down
43 changes: 28 additions & 15 deletions crates/oxc_codegen/src/annotation_comment.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,19 @@ impl<'a> Codegen<'a> {
if !self.comment_options.preserve_annotate_comments {
return vec![];
}
self.get_leading_comments(self.latest_consumed_comment_end, node_start)
let mut latest_comment_start = node_start;
let mut ret = self
.get_leading_comments(self.latest_consumed_comment_end, node_start)
.rev()
// each comment should be separated by whitespaces
.take_while(|comment| {
let comment_end = comment.real_span_end();
let range_content =
&self.source_text[comment_end as usize..latest_comment_start as usize];
let all_whitespace = range_content.chars().all(char::is_whitespace);
latest_comment_start = comment.real_span_start();
all_whitespace
})
.filter_map(|comment| {
let source_code = self.source_text;
let comment_content =
Expand All @@ -68,7 +80,9 @@ impl<'a> Codegen<'a> {
}
None
})
.collect::<Vec<_>>()
.collect::<Vec<_>>();
ret.reverse();
ret
}

pub(crate) fn print_comment(&mut self, comment: AnnotationComment) {
Expand All @@ -85,19 +99,19 @@ impl<'a> Codegen<'a> {
// in this example, `Object.getOwnPropertyNames(Symbol)` and `Object.getOwnPropertyNames(Symbol).filter()`, `Object.getOwnPropertyNames(Symbol).filter().map()`
// share the same leading comment. since they both are call expr and has same span start, we need to avoid print the same comment multiple times.
let comment_span = comment.span();

if self.latest_consumed_comment_end >= comment_span.end {
let real_span_end = comment.comment.real_span_end();
if self.latest_consumed_comment_end >= real_span_end {
return;
}
let real_comment_end = match comment.kind() {
self.update_last_consumed_comment_end(real_span_end);
match comment.kind() {
CommentKind::SingleLine => {
self.print_str("//");
self.print_range_of_source_code(
comment_span.start as usize..comment_span.end as usize,
);
self.print_soft_newline();
self.print_indent();
comment_span.end
}
CommentKind::MultiLine => {
self.print_str("/*");
Expand All @@ -106,10 +120,8 @@ impl<'a> Codegen<'a> {
);
self.print_str("*/");
self.print_soft_space();
comment_span.end + 2
}
};
self.update_last_consumed_comment_end(real_comment_end);
}
// FIXME: esbuild function `restoreExprStartFlags`
self.start_of_default_export = self.code_len();
}
Expand All @@ -118,25 +130,26 @@ impl<'a> Codegen<'a> {
if !self.comment_options.preserve_annotate_comments {
return;
}
let annotation_kind_set = AnnotationKind::empty();

let mut annotation_kind_set = AnnotationKind::empty();
if let Some(comments) = self.try_take_moved_comment(node_start) {
self.print_comments(&comments, &mut annotation_kind_set);
}
let leading_annotate_comments = self.get_leading_annotate_comments(node_start);
self.print_comments(&leading_annotate_comments, annotation_kind_set);
self.print_comments(&leading_annotate_comments, &mut annotation_kind_set);
}

#[inline]
pub(crate) fn print_comments(
&mut self,
leading_annotate_comment: &Vec<AnnotationComment>,
mut annotation_kind_set: AnnotationKind,
annotation_kind_set: &mut AnnotationKind,
) {
for &comment in leading_annotate_comment {
let kind = comment.annotation_kind();
if !annotation_kind_set.intersects(kind) {
if !annotation_kind_set.contains(kind) {
annotation_kind_set.insert(kind);
self.print_comment(comment);
}
self.update_last_consumed_comment_end(comment.span().end);
}
}

Expand Down
35 changes: 22 additions & 13 deletions crates/oxc_codegen/src/gen.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,10 +155,6 @@ impl<'a> Gen for Statement<'a> {
p.print_semicolon_after_statement();
}
}

if p.comment_options.preserve_annotate_comments {
p.update_last_consumed_comment_end(self.span().end);
}
}
}

Expand Down Expand Up @@ -592,9 +588,17 @@ impl<'a> Gen for VariableDeclaration<'a> {
}

if p.comment_options.preserve_annotate_comments
&& !matches!(self.kind, VariableDeclarationKind::Const)
&& matches!(self.kind, VariableDeclarationKind::Const)
{
p.update_last_consumed_comment_end(self.span.start);
if let Some(declarator) = self.declarations.first() {
if let Some(ref init) = declarator.init {
let leading_annotate_comments =
p.get_leading_annotate_comments(self.span.start);
if !leading_annotate_comments.is_empty() {
p.move_comments(init.span().start, leading_annotate_comments);
}
}
}
}
p.print_str(match self.kind {
VariableDeclarationKind::Const => "const",
Expand Down Expand Up @@ -864,9 +868,17 @@ impl<'a> Gen for ExportNamedDeclaration<'a> {
p.gen_comments(self.span.start);
}
Some(Declaration::VariableDeclaration(var_decl))
if !matches!(var_decl.kind, VariableDeclarationKind::Const) =>
if matches!(var_decl.kind, VariableDeclarationKind::Const) =>
{
p.update_last_consumed_comment_end(self.span.start);
if let Some(declarator) = var_decl.declarations.first() {
if let Some(ref init) = declarator.init {
let leading_annotate_comments =
p.get_leading_annotate_comments(self.span.start);
if !leading_annotate_comments.is_empty() {
p.move_comments(init.span().start, leading_annotate_comments);
}
}
}
}
_ => {}
};
Expand Down Expand Up @@ -1062,9 +1074,6 @@ impl<'a> GenExpr for Expression<'a> {
Self::TSNonNullExpression(e) => e.gen_expr(p, precedence, ctx),
Self::TSInstantiationExpression(e) => e.gen_expr(p, precedence, ctx),
}
if p.comment_options.preserve_annotate_comments {
p.update_last_consumed_comment_end(self.span().end);
}
}
}

Expand Down Expand Up @@ -1409,7 +1418,7 @@ impl<'a> GenExpr for CallExpression<'a> {
wrap = true;
}
p.wrap(wrap, |p| {
p.print_comments(&annotate_comments, AnnotationKind::empty());
p.print_comments(&annotate_comments, &mut AnnotationKind::empty());
p.add_source_mapping(self.span.start);
self.callee.gen_expr(p, Precedence::Postfix, Context::empty());
if self.optional {
Expand Down Expand Up @@ -2078,7 +2087,7 @@ impl<'a> GenExpr for NewExpression<'a> {
wrap = true;
}
p.wrap(wrap, |p| {
p.print_comments(&annotate_comment, AnnotationKind::empty());
p.print_comments(&annotate_comment, &mut AnnotationKind::empty());
p.print_space_before_identifier();
p.add_source_mapping(self.span.start);
p.print_str("new ");
Expand Down
45 changes: 43 additions & 2 deletions crates/oxc_codegen/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ mod gen;
mod operator;
mod sourcemap_builder;

use std::{borrow::Cow, ops::Range};
use std::{borrow::Cow, collections::hash_map::Entry, ops::Range};

use oxc_ast::{
ast::{BindingIdentifier, BlockStatement, Expression, IdentifierReference, Program, Statement},
Expand All @@ -23,6 +23,7 @@ use oxc_syntax::{
operator::{BinaryOperator, UnaryOperator, UpdateOperator},
precedence::Precedence,
};
use rustc_hash::FxHashMap;

use crate::{
binary_expr_visitor::BinaryExpressionVisitor, operator::Operator,
Expand All @@ -33,6 +34,8 @@ pub use crate::{
gen::{Gen, GenExpr},
};

use self::annotation_comment::AnnotationComment;

/// Code generator without whitespace removal.
pub type CodeGenerator<'a> = Codegen<'a>;

Expand Down Expand Up @@ -95,7 +98,13 @@ pub struct Codegen<'a> {
sourcemap_builder: Option<SourcemapBuilder>,

latest_consumed_comment_end: u32,

/// The key of map is the node start position,
/// the first element of value is the start of the comment
/// the second element of value includes the end of the comment and comment kind.
move_comment_map: MoveCommentMap,
}
pub(crate) type MoveCommentMap = FxHashMap<u32, Vec<AnnotationComment>>;

impl<'a> Default for Codegen<'a> {
fn default() -> Self {
Expand Down Expand Up @@ -140,6 +149,7 @@ impl<'a> Codegen<'a> {
quote: b'"',
sourcemap_builder: None,
latest_consumed_comment_end: 0,
move_comment_map: MoveCommentMap::default(),
}
}

Expand Down Expand Up @@ -516,7 +526,38 @@ impl<'a> Codegen<'a> {
self.code.extend_from_slice(self.source_text[range].as_bytes());
}

fn get_leading_comments(&self, start: u32, end: u32) -> impl Iterator<Item = &'_ Comment> + '_ {
fn get_leading_comments(
&self,
start: u32,
end: u32,
) -> impl DoubleEndedIterator<Item = &'_ Comment> + '_ {
self.trivias.comments_range(start..end)
}
/// In some scenario, we want to move the comment that should be codegened to another position.
/// ```js
/// /* @__NO_SIDE_EFFECTS__ */ export const a = function() {
///
/// }, b = 10000;
///
/// ```
/// should generate such output:
/// ```js
/// export const /* @__NO_SIDE_EFFECTS__ */ a = function() {
///
/// }, b = 10000;
/// ```
fn move_comments(&mut self, position: u32, full_comment_infos: Vec<AnnotationComment>) {
match self.move_comment_map.entry(position) {
Entry::Occupied(mut occ) => {
occ.get_mut().extend(full_comment_infos);
}
Entry::Vacant(vac) => {
vac.insert(full_comment_infos);
}
}
}

fn try_take_moved_comment(&mut self, node_start: u32) -> Option<Vec<AnnotationComment>> {
self.move_comment_map.remove(&node_start)
}
}

0 comments on commit b7db235

Please sign in to comment.