Skip to content

Commit

Permalink
feat: emit warning for const assign code (#538)
Browse files Browse the repository at this point in the history
* fix: forbit const assign for esbuild

* fix: move to `visit_binding_identifier`

* refactor

* fix: update test
  • Loading branch information
kazupon committed Mar 11, 2024
1 parent e2ab72f commit f7ec1b5
Show file tree
Hide file tree
Showing 16 changed files with 215 additions and 35 deletions.
1 change: 1 addition & 0 deletions crates/rolldown/src/ast_scanner/impl_visit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ impl<'ast> Visit<'ast> for AstScanner<'ast> {
if self.is_top_level(symbol_id) {
self.add_declared_id(symbol_id);
}
self.try_diagnostic_forbid_const_assign(symbol_id);
}

fn visit_identifier_reference(&mut self, ident: &IdentifierReference) {
Expand Down
19 changes: 19 additions & 0 deletions crates/rolldown/src/ast_scanner/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -368,4 +368,23 @@ impl<'ast> AstScanner<'ast> {
fn is_top_level(&self, symbol_id: SymbolId) -> bool {
self.scope.root_scope_id() == self.symbol_table.scope_id_for(symbol_id)
}

fn try_diagnostic_forbid_const_assign(&mut self, symbol_id: SymbolId) {
if self.symbol_table.get_flag(symbol_id).is_const_variable() {
for reference in self.scope.get_resolved_references(symbol_id) {
if reference.is_write() {
self.result.warnings.push(
BuildError::forbid_const_assign(
self.file_path.to_string(),
Arc::clone(self.source),
self.symbol_table.get_name(symbol_id).into(),
self.symbol_table.get_span(symbol_id),
reference.span(),
)
.with_severity_warning(),
);
}
}
}
}
}
6 changes: 5 additions & 1 deletion crates/rolldown/src/ast_scanner/side_effect_detector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,11 @@ mod test {
let ast_scope = {
let semantic = program.make_semantic(source_type);
let (mut symbol_table, scope) = semantic.into_symbol_table_and_scope_tree();
AstScope::new(scope, std::mem::take(&mut symbol_table.references))
AstScope::new(
scope,
std::mem::take(&mut symbol_table.references),
std::mem::take(&mut symbol_table.resolved_references),
)
};

let has_side_effect = program
Expand Down
6 changes: 5 additions & 1 deletion crates/rolldown/src/module_loader/normal_module_task.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,11 @@ impl<'task, T: FileSystem + Default + 'static> NormalModuleTask<'task, T> {

let semantic = program.make_semantic(source_type);
let (mut symbol_table, scope) = semantic.into_symbol_table_and_scope_tree();
let ast_scope = AstScope::new(scope, std::mem::take(&mut symbol_table.references));
let ast_scope = AstScope::new(
scope,
std::mem::take(&mut symbol_table.references),
std::mem::take(&mut symbol_table.resolved_references),
);
let mut symbol_for_module = AstSymbols::from_symbol_table(symbol_table);
let repr_name = self.resolved_path.path.representative_name();
let scanner = AstScanner::new(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,11 @@ impl RuntimeNormalModuleTask {

let semantic = program.make_semantic(source_type);
let (mut symbol_table, scope) = semantic.into_symbol_table_and_scope_tree();
let ast_scope = AstScope::new(scope, std::mem::take(&mut symbol_table.references));
let ast_scope = AstScope::new(
scope,
std::mem::take(&mut symbol_table.references),
std::mem::take(&mut symbol_table.resolved_references),
);
let mut symbol_for_module = AstSymbols::from_symbol_table(symbol_table);
let facade_path = FilePath::new("runtime");
let scanner = AstScanner::new(
Expand Down
20 changes: 17 additions & 3 deletions crates/rolldown/src/types/ast_symbols.rs
Original file line number Diff line number Diff line change
@@ -1,19 +1,21 @@
use index_vec::IndexVec;
use oxc::{
semantic::{ScopeId, SymbolId, SymbolTable},
span::CompactString,
semantic::{ScopeId, SymbolFlags, SymbolId, SymbolTable},
span::{CompactString, Span},
};

#[derive(Debug, Default)]
pub struct AstSymbols {
pub names: IndexVec<SymbolId, CompactString>,
pub scope_ids: IndexVec<SymbolId, ScopeId>,
pub spans: IndexVec<SymbolId, Span>,
pub flags: IndexVec<SymbolId, SymbolFlags>,
}

impl AstSymbols {
pub fn from_symbol_table(table: SymbolTable) -> Self {
debug_assert!(table.references.is_empty());
Self { names: table.names, scope_ids: table.scope_ids }
Self { names: table.names, scope_ids: table.scope_ids, spans: table.spans, flags: table.flags }
}

pub fn create_symbol(&mut self, name: CompactString, scope_id: ScopeId) -> SymbolId {
Expand All @@ -24,4 +26,16 @@ impl AstSymbols {
pub fn scope_id_for(&self, symbol_id: SymbolId) -> ScopeId {
self.scope_ids[symbol_id]
}

pub fn get_span(&self, symbol_id: SymbolId) -> Span {
self.spans[symbol_id]
}

pub fn get_name(&self, symbol_id: SymbolId) -> &str {
&self.names[symbol_id]
}

pub fn get_flag(&self, symbol_id: SymbolId) -> SymbolFlags {
self.flags[symbol_id]
}
}

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
---
source: crates/rolldown/tests/common/case.rs
expression: content
input_file: crates/rolldown/tests/esbuild/default/forbid_const_assign_when_bundling
---
# warnings

## FORBID_CONST_ASSIGN

```text
[FORBID_CONST_ASSIGN] Warning: Unexpected re-assignment of const variable `x`
╭─[tests/esbuild/default/forbid_const_assign_when_bundling/entry.js:4:3]
3 │ const x = 1
│ ┬
│ ╰── `x` is declared here as const
4 │ x = 2
│ ┬
│ ╰── `x` is re-assigned here
───╯
```
## FORBID_CONST_ASSIGN

```text
[FORBID_CONST_ASSIGN] Warning: Unexpected re-assignment of const variable `y`
╭─[tests/esbuild/default/forbid_const_assign_when_bundling/entry.js:14:5]
13 │ const y = 1
│ ┬
│ ╰── `y` is declared here as const
14 │ y = 2
│ ┬
│ ╰── `y` is re-assigned here
────╯
```
# Assets

## entry_js.mjs

```js
// entry.js
let err;
try{
const x = 1;
x = 2;
}catch(e){
err = e;
}assert(typeof err !== 'undefined');
function foo() {
let err$1;
try{
const y = 1;
y = 2;
}catch(e){
err$1 = e;
} assert(typeof err$1 !== 'undefined');
}
foo();
```
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
let err
try {
const x = 1
x = 2
} catch (e) {
err = e
}
assert(typeof err !== 'undefined')

function foo() {
let err
try {
const y = 1
y = 2
} catch (e) {
err = e
}
assert(typeof err !== 'undefined')
}

foo()
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,4 @@
}
]
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,4 @@ console.log(false ? require.resolve('dead-if') : 0)
console.log(true ? 0 : require.resolve('dead-if'))
console.log(false && require.resolve('dead-and'))
console.log(true || require.resolve('dead-or'))
console.log(true ?? require.resolve('dead-nullish'))
console.log(true ?? require.resolve('dead-nullish'))
16 changes: 14 additions & 2 deletions crates/rolldown_common/src/types/ast_scope.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,16 @@ use oxc::semantic::{Reference, ReferenceId, ScopeTree, SymbolId};
pub struct AstScope {
inner: ScopeTree,
references: IndexVec<ReferenceId, Reference>,
resolved_references: IndexVec<SymbolId, Vec<ReferenceId>>,
}

impl AstScope {
pub fn new(inner: ScopeTree, references: IndexVec<ReferenceId, Reference>) -> Self {
Self { inner, references }
pub fn new(
inner: ScopeTree,
references: IndexVec<ReferenceId, Reference>,
resolved_references: IndexVec<SymbolId, Vec<ReferenceId>>,
) -> Self {
Self { inner, references, resolved_references }
}

pub fn is_unresolved(&self, reference_id: ReferenceId) -> bool {
Expand All @@ -19,6 +24,13 @@ impl AstScope {
pub fn symbol_id_for(&self, reference_id: ReferenceId) -> Option<SymbolId> {
self.references[reference_id].symbol_id()
}

pub fn get_resolved_references(
&self,
symbol_id: SymbolId,
) -> impl Iterator<Item = &Reference> + '_ {
self.resolved_references[symbol_id].iter().map(|reference_id| &self.references[*reference_id])
}
}

impl std::ops::Deref for AstScope {
Expand Down
17 changes: 14 additions & 3 deletions crates/rolldown_error/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,10 @@ use oxc::span::Span;
use crate::{
diagnostic::Diagnostic,
error_kind::{
external_entry::ExternalEntry, sourcemap_error::SourceMapError,
unresolved_entry::UnresolvedEntry, unresolved_import::UnresolvedImport,
unsupported_eval::UnsupportedEval, BuildErrorLike, NapiError,
external_entry::ExternalEntry, forbid_const_assign::ForbitConstAssign,
sourcemap_error::SourceMapError, unresolved_entry::UnresolvedEntry,
unresolved_import::UnresolvedImport, unsupported_eval::UnsupportedEval, BuildErrorLike,
NapiError,
},
};

Expand Down Expand Up @@ -108,6 +109,16 @@ impl BuildError {
pub fn unsupported_eval(filename: String, source: Arc<str>, span: Span) -> Self {
Self::new_inner(UnsupportedEval { filename, eval_span: span, source })
}

pub fn forbid_const_assign(
filename: String,
source: Arc<str>,
name: String,
reference_span: Span,
re_assign_span: Span,
) -> Self {
Self::new_inner(ForbitConstAssign { filename, source, name, reference_span, re_assign_span })
}
}

impl From<std::io::Error> for BuildError {
Expand Down
50 changes: 50 additions & 0 deletions crates/rolldown_error/src/error_kind/forbid_const_assign.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
use std::{path::Path, sync::Arc};

use ariadne::Label;
use oxc::span::Span;

use crate::{diagnostic::DiagnosticBuilder, PathExt};

use super::BuildErrorLike;

#[derive(Debug)]
pub struct ForbitConstAssign {
pub filename: String,
pub source: Arc<str>,
pub name: String,
pub reference_span: Span,
pub re_assign_span: Span,
}

impl BuildErrorLike for ForbitConstAssign {
//
fn code(&self) -> &'static str {
"FORBID_CONST_ASSIGN"
}

fn message(&self) -> String {
format!("Unexpected re-assignment of const variable `{0}` at {1}", self.name, self.filename)
}

fn diagnostic_builder(&self) -> crate::diagnostic::DiagnosticBuilder {
let filename = Path::new(&self.filename).relative_display();
DiagnosticBuilder {
code: Some(self.code()),
summary: Some(format!("Unexpected re-assignment of const variable `{0}`", self.name)),
files: Some(vec![(filename.clone(), self.source.to_string())]),
labels: Some(vec![
Label::new((
filename.clone(),
(self.re_assign_span.start as usize..self.re_assign_span.end as usize),
))
.with_message(format!("`{0}` is re-assigned here", self.name)),
Label::new((
filename,
(self.reference_span.start as usize..self.reference_span.end as usize),
))
.with_message(format!("`{0}` is declared here as const", self.name)),
]),
..Default::default()
}
}
}
1 change: 1 addition & 0 deletions crates/rolldown_error/src/error_kind/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use std::fmt::Debug;

use crate::diagnostic::DiagnosticBuilder;
pub mod external_entry;
pub mod forbid_const_assign;
pub mod sourcemap_error;
pub mod unresolved_entry;
pub mod unresolved_import;
Expand Down

0 comments on commit f7ec1b5

Please sign in to comment.