@samitouri / QOS-React-2 / commits / a32a2baa73

[rust] Start of error handling

Until now i've freely used `panic!`, `unwrap()`, and friends for "error handling". This PR switches to consistently returning `Result` within the HIR builder, using a structured error representation that exploits helpers from `thiserror` and `miette` crates. Miette has a super graphical formatter for diagnostics as you can see in the screenshot (also see the [repo](https://docs.rs/miette/5.9.0/miette/index.html)). This is just a first pass and we'll need to flush out the error handing story more. Two obvious directions to go next: * Make HIR construction error-tolerant, so that it can find as many errors as possible at once rather than failing on the first error. We did this in Relay Compiler as well, and we can likely borrow some of its helpers. * Decouple from `miette`. It's very nice but less flexible than I'd like. We can define our own more generic diagnostic type that contains structured data, then have a generic conversion mechanism into a miette type so we can use their display logic. <img width="789" alt="Screenshot 2023-07-06 at 3 55 38 PM" src="https://github.com/facebook/react-forget/assets/6425824/e1f1ed4b-5188-4af5-9af4-8f6c5c345023">

Joe Savona committed Jul 8, 2023 at 22:59 UTC a32a2baa73ce3d42b231e59cc582930666ac1e66
13 files changed +503 -135
compiler/forget/Cargo.lock
+151 -6
@@ -153,6 +153,15 @@ dependencies = [
153 "rustc-demangle",
154 ]
155
156 +[[package]]
157 +name = "backtrace-ext"
158 +version = "0.2.1"
159 +source = "registry+https://github.com/rust-lang/crates.io-index"
160 +checksum = "537beee3be4a18fb023b570f80e3ae28003db9167a751266b259926e25539d50"
161 +dependencies = [
162 + "backtrace",
163 +]
164 +
165 [[package]]
166 name = "base64"
167 version = "0.13.1"
@@ -232,6 +241,8 @@ dependencies = [
241 "estree",
242 "hir",
243 "indexmap 2.0.0",
244 + "miette 5.9.0",
245 + "thiserror",
246 ]
247
248 [[package]]
@@ -276,7 +287,7 @@ dependencies = [
287 "encode_unicode",
288 "lazy_static",
289 "libc",
279 - "windows-sys",
290 + "windows-sys 0.45.0",
291 ]
292
293 [[package]]
@@ -423,6 +434,27 @@ version = "1.0.0"
434 source = "registry+https://github.com/rust-lang/crates.io-index"
435 checksum = "88bffebc5d80432c9b140ee17875ff173a8ab62faad5b257da912bd2f6c1c0a1"
436
437 +[[package]]
438 +name = "errno"
439 +version = "0.3.1"
440 +source = "registry+https://github.com/rust-lang/crates.io-index"
441 +checksum = "4bcfec3a70f97c962c307b2d2c56e358cf1d00b558d74262b5f929ee8cc7e73a"
442 +dependencies = [
443 + "errno-dragonfly",
444 + "libc",
445 + "windows-sys 0.48.0",
446 +]
447 +
448 +[[package]]
449 +name = "errno-dragonfly"
450 +version = "0.1.2"
451 +source = "registry+https://github.com/rust-lang/crates.io-index"
452 +checksum = "aa68f1b12764fab894d2755d2518754e71b4fd80ecfb822714a1206c2aab39bf"
453 +dependencies = [
454 + "cc",
455 + "libc",
456 +]
457 +
458 [[package]]
459 name = "estree"
460 version = "0.1.0"
@@ -460,6 +492,7 @@ dependencies = [
492 "estree-swc",
493 "hir",
494 "insta",
495 + "miette 5.9.0",
496 ]
497
498 [[package]]
@@ -667,6 +700,17 @@ dependencies = [
700 "yaml-rust",
701 ]
702
703 +[[package]]
704 +name = "io-lifetimes"
705 +version = "1.0.11"
706 +source = "registry+https://github.com/rust-lang/crates.io-index"
707 +checksum = "eae7b9aee968036d54dce06cebaefd919e4472e753296daccd6d344e3e2df0c2"
708 +dependencies = [
709 + "hermit-abi 0.3.2",
710 + "libc",
711 + "windows-sys 0.48.0",
712 +]
713 +
714 [[package]]
715 name = "is-macro"
716 version = "0.3.0"
@@ -680,6 +724,18 @@ dependencies = [
724 "syn 2.0.23",
725 ]
726
727 +[[package]]
728 +name = "is-terminal"
729 +version = "0.4.7"
730 +source = "registry+https://github.com/rust-lang/crates.io-index"
731 +checksum = "adcf93614601c8129ddf72e2d5633df827ba6551541c6d8c59520a371475be1f"
732 +dependencies = [
733 + "hermit-abi 0.3.2",
734 + "io-lifetimes",
735 + "rustix",
736 + "windows-sys 0.48.0",
737 +]
738 +
739 [[package]]
740 name = "is_ci"
741 version = "1.1.1"
@@ -810,6 +866,12 @@ version = "0.5.6"
866 source = "registry+https://github.com/rust-lang/crates.io-index"
867 checksum = "0717cef1bc8b636c6e1c1bbdefc09e6322da8a9321966e8928ef80d20f7f770f"
868
869 +[[package]]
870 +name = "linux-raw-sys"
871 +version = "0.3.8"
872 +source = "registry+https://github.com/rust-lang/crates.io-index"
873 +checksum = "ef53942eb7bf7ff43a617b3e2c1c4a5ecf5944a7c1bc12d7ee39bbb15e5c1519"
874 +
875 [[package]]
876 name = "lock_api"
877 version = "0.4.10"
@@ -858,12 +920,33 @@ checksum = "1c90329e44f9208b55f45711f9558cec15d7ef8295cc65ecd6d4188ae8edc58c"
920 dependencies = [
921 "atty",
922 "backtrace",
861 - "miette-derive",
923 + "miette-derive 4.7.1",
924 + "once_cell",
925 + "owo-colors",
926 + "supports-color 1.3.1",
927 + "supports-hyperlinks 1.2.0",
928 + "supports-unicode 1.0.2",
929 + "terminal_size",
930 + "textwrap",
931 + "thiserror",
932 + "unicode-width",
933 +]
934 +
935 +[[package]]
936 +name = "miette"
937 +version = "5.9.0"
938 +source = "registry+https://github.com/rust-lang/crates.io-index"
939 +checksum = "a236ff270093b0b67451bc50a509bd1bad302cb1d3c7d37d5efe931238581fa9"
940 +dependencies = [
941 + "backtrace",
942 + "backtrace-ext",
943 + "is-terminal",
944 + "miette-derive 5.9.0",
945 "once_cell",
946 "owo-colors",
864 - "supports-color",
865 - "supports-hyperlinks",
866 - "supports-unicode",
947 + "supports-color 2.0.0",
948 + "supports-hyperlinks 2.1.0",
949 + "supports-unicode 2.0.0",
950 "terminal_size",
951 "textwrap",
952 "thiserror",
@@ -881,6 +964,17 @@ dependencies = [
964 "syn 1.0.109",
965 ]
966
967 +[[package]]
968 +name = "miette-derive"
969 +version = "5.9.0"
970 +source = "registry+https://github.com/rust-lang/crates.io-index"
971 +checksum = "4901771e1d44ddb37964565c654a3223ba41a594d02b8da471cc4464912b5cfa"
972 +dependencies = [
973 + "proc-macro2",
974 + "quote",
975 + "syn 2.0.23",
976 +]
977 +
978 [[package]]
979 name = "minimal-lexical"
980 version = "0.2.1"
@@ -1287,6 +1381,20 @@ dependencies = [
1381 "semver 0.9.0",
1382 ]
1383
1384 +[[package]]
1385 +name = "rustix"
1386 +version = "0.37.22"
1387 +source = "registry+https://github.com/rust-lang/crates.io-index"
1388 +checksum = "8818fa822adcc98b18fedbb3632a6a33213c070556b5aa7c4c8cc21cff565c4c"
1389 +dependencies = [
1390 + "bitflags 1.3.2",
1391 + "errno",
1392 + "io-lifetimes",
1393 + "libc",
1394 + "linux-raw-sys",
1395 + "windows-sys 0.48.0",
1396 +]
1397 +
1398 [[package]]
1399 name = "rustversion"
1400 version = "1.0.13"
@@ -1549,6 +1657,16 @@ dependencies = [
1657 "is_ci",
1658 ]
1659
1660 +[[package]]
1661 +name = "supports-color"
1662 +version = "2.0.0"
1663 +source = "registry+https://github.com/rust-lang/crates.io-index"
1664 +checksum = "4950e7174bffabe99455511c39707310e7e9b440364a2fcb1cc21521be57b354"
1665 +dependencies = [
1666 + "is-terminal",
1667 + "is_ci",
1668 +]
1669 +
1670 [[package]]
1671 name = "supports-hyperlinks"
1672 version = "1.2.0"
@@ -1558,6 +1676,15 @@ dependencies = [
1676 "atty",
1677 ]
1678
1679 +[[package]]
1680 +name = "supports-hyperlinks"
1681 +version = "2.1.0"
1682 +source = "registry+https://github.com/rust-lang/crates.io-index"
1683 +checksum = "f84231692eb0d4d41e4cdd0cabfdd2e6cd9e255e65f80c9aa7c98dd502b4233d"
1684 +dependencies = [
1685 + "is-terminal",
1686 +]
1687 +
1688 [[package]]
1689 name = "supports-unicode"
1690 version = "1.0.2"
@@ -1567,6 +1694,15 @@ dependencies = [
1694 "atty",
1695 ]
1696
1697 +[[package]]
1698 +name = "supports-unicode"
1699 +version = "2.0.0"
1700 +source = "registry+https://github.com/rust-lang/crates.io-index"
1701 +checksum = "4b6c2cb240ab5dd21ed4906895ee23fe5a48acdbd15a3ce388e7b62a9b66baf7"
1702 +dependencies = [
1703 + "is-terminal",
1704 +]
1705 +
1706 [[package]]
1707 name = "swc"
1708 version = "0.264.8"
@@ -2293,7 +2429,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
2429 checksum = "108322b719696e8c368c39dc6d8748494ea2aa870e7d80ea5956078aa6b4dd4d"
2430 dependencies = [
2431 "anyhow",
2296 - "miette",
2432 + "miette 4.7.1",
2433 "once_cell",
2434 "parking_lot",
2435 "swc_common",
@@ -2758,6 +2894,15 @@ dependencies = [
2894 "windows-targets 0.42.2",
2895 ]
2896
2897 +[[package]]
2898 +name = "windows-sys"
2899 +version = "0.48.0"
2900 +source = "registry+https://github.com/rust-lang/crates.io-index"
2901 +checksum = "677d2418bec65e3338edb076e806bc1ec15693c5d0104683f2efe857f61056a9"
2902 +dependencies = [
2903 + "windows-targets 0.48.1",
2904 +]
2905 +
2906 [[package]]
2907 name = "windows-targets"
2908 version = "0.42.2"
compiler/forget/Cargo.toml
+12 -2
@@ -1,5 +1,5 @@
1 [workspace]
2 -
2 +resolver = "2"
3 members = [
4 "crates/build-hir",
5 "crates/fixtures",
@@ -7,4 +7,14 @@ members = [
7 "crates/swc-demo",
8 "crates/estree",
9 "crates/estree-swc",
10 -]
\ No newline at end of file
10 +]
11 +
12 +# Make insta run faster by compiling with release mode optimizations
13 +# https://docs.rs/insta/latest/insta/#optional-faster-runs
14 +[profile.dev.package.insta]
15 +opt-level = 3
16 +
17 +# Make insta diffing libary faster by compiling with release mode optimizations
18 +# https://docs.rs/insta/latest/insta/#optional-faster-runs
19 +[profile.dev.package.similar]
20 +opt-level = 3
\ No newline at end of file
compiler/forget/crates/build-hir/Cargo.toml
+2
@@ -10,3 +10,5 @@ hir = { path = "../hir" }
10 estree = { path = "../estree" }
11 indexmap = "2.0.0"
12 bumpalo = "3.13.0"
13 +miette = { version = "5.9.0" }
14 +thiserror = "1.0.41"
compiler/forget/crates/build-hir/src/build.rs
+100 -69
@@ -1,4 +1,4 @@
1 -use bumpalo::collections::{CollectIn, String};
1 +use bumpalo::collections::{String, Vec};
2 use estree::{
3 AssignmentTarget, BinaryExpression, ExpressionLike, ForInit, ForStatement, FunctionDeclaration,
4 IfStatement, Literal, LiteralValue, Pattern, Statement, VariableDeclarationKind,
@@ -9,7 +9,11 @@ use hir::{
9 PrimitiveValue, TerminalValue,
10 };
11
12 -use crate::builder::{Binding, Builder, LoopScope};
12 +use crate::{
13 + builder::{Binding, Builder, LoopScope},
14 + error::DiagnosticError,
15 + BuildDiagnostic, ErrorSeverity,
16 +};
17
18 /// Converts a React function in ESTree format into HIR. Returns the HIR
19 /// if it was constructed sucessfully, otherwise a list of diagnostics
@@ -20,7 +24,7 @@ use crate::builder::{Binding, Builder, LoopScope};
24 pub fn build<'a>(
25 environment: &'a Environment<'a>,
26 fun: FunctionDeclaration,
23 -) -> Result<Function<'a>, Diagnostic> {
27 +) -> Result<Function<'a>, BuildDiagnostic> {
28 let mut builder = Builder::new(environment);
29
30 lower_statement(environment, &mut builder, fun.body.unwrap(), None)?;
@@ -57,7 +61,7 @@ fn lower_statement<'a>(
61 builder: &mut Builder<'a>,
62 stmt: Statement,
63 label: Option<String<'a>>,
60 -) -> Result<(), Diagnostic> {
64 +) -> Result<(), BuildDiagnostic> {
65 match stmt {
66 Statement::BlockStatement(stmt) => {
67 for stmt in stmt.body {
@@ -86,7 +90,7 @@ fn lower_statement<'a>(
90 }
91 Statement::ReturnStatement(stmt) => {
92 let value = match stmt.argument {
89 - Some(argument) => lower_expression_to_temporary(env, builder, argument),
93 + Some(argument) => lower_expression_to_temporary(env, builder, argument)?,
94 None => lower_value_to_temporary(
95 env,
96 builder,
@@ -101,7 +105,7 @@ fn lower_statement<'a>(
105 );
106 }
107 Statement::ExpressionStatement(stmt) => {
104 - lower_expression_to_temporary(env, builder, stmt.expression);
108 + lower_expression_to_temporary(env, builder, stmt.expression)?;
109 }
110 Statement::EmptyStatement(_) => {
111 // no-op
@@ -110,25 +114,37 @@ fn lower_statement<'a>(
114 let kind = match stmt.kind {
115 VariableDeclarationKind::Const => InstructionKind::Const,
116 VariableDeclarationKind::Let => InstructionKind::Let,
113 - VariableDeclarationKind::Var => panic!("`var` declarations are not supported"),
117 + VariableDeclarationKind::Var => {
118 + return Err(BuildDiagnostic::new(
119 + DiagnosticError::VariableDeclarationKindIsVar,
120 + ErrorSeverity::Unsupported,
121 + stmt.range,
122 + ));
123 + }
124 };
125 for declaration in stmt.declarations {
126 if let Some(init) = declaration.init {
117 - let value = lower_expression_to_temporary(env, builder, init);
127 + let value = lower_expression_to_temporary(env, builder, init)?;
128 lower_assignment(
129 env,
130 builder,
131 kind,
132 AssignmentTarget::Pattern(declaration.id.into()),
133 value,
124 - );
134 + )?;
135 } else {
136 if let Pattern::Identifier(id) = declaration.id {
137 // TODO: handle unbound variables
128 - let binding = builder.resolve_binding(&id).unwrap();
138 + let binding = builder.resolve_binding(&id)?;
139 let identifier = match binding {
140 Binding::Local(identifier) => identifier,
131 - _ => panic!("Expected variable declaration to be a local binding"),
141 + _ => {
142 + return Err(BuildDiagnostic::new(
143 + DiagnosticError::VariableDeclarationBindingIsNonLocal,
144 + ErrorSeverity::Invariant,
145 + id.range,
146 + ));
147 + }
148 };
149 let place = Place {
150 effect: None,
@@ -158,24 +174,24 @@ fn lower_statement<'a>(
174 } = *stmt;
175
176 let consequent_block = builder.enter(BlockKind::Block, |builder| {
161 - lower_statement(env, builder, consequent, None).unwrap();
162 - TerminalValue::Goto(hir::GotoTerminal {
177 + lower_statement(env, builder, consequent, None)?;
178 + Ok(TerminalValue::Goto(hir::GotoTerminal {
179 block: fallthrough_block.id,
180 kind: GotoKind::Break,
165 - })
166 - });
181 + }))
182 + })?;
183
184 let alternate_block = builder.enter(BlockKind::Block, |builder| {
185 if let Some(alternate) = alternate {
170 - lower_statement(env, builder, alternate, None).unwrap();
186 + lower_statement(env, builder, alternate, None)?;
187 }
172 - TerminalValue::Goto(hir::GotoTerminal {
188 + Ok(TerminalValue::Goto(hir::GotoTerminal {
189 block: fallthrough_block.id,
190 kind: GotoKind::Break,
175 - })
176 - });
191 + }))
192 + })?;
193
178 - let test = lower_expression_to_temporary(env, builder, test);
194 + let test = lower_expression_to_temporary(env, builder, test)?;
195 let terminal = TerminalValue::If(hir::IfTerminal {
196 test,
197 consequent: consequent_block,
@@ -201,26 +217,31 @@ fn lower_statement<'a>(
217
218 let init_block = builder.enter(BlockKind::Loop, |builder| {
219 if let Some(ForInit::VariableDeclaration(decl)) = init {
204 - lower_statement(env, builder, Statement::VariableDeclaration(decl), None)
205 - .unwrap();
206 - TerminalValue::Goto(hir::GotoTerminal {
220 + lower_statement(env, builder, Statement::VariableDeclaration(decl), None)?;
221 + Ok(TerminalValue::Goto(hir::GotoTerminal {
222 block: test_block.id,
223 kind: GotoKind::Break,
209 - })
224 + }))
225 } else {
211 - panic!("Expected for statement to have a variable declaration initializer")
226 + Err(BuildDiagnostic::new(
227 + DiagnosticError::ForStatementIsMissingInitializer,
228 + ErrorSeverity::Todo,
229 + None,
230 + ))
231 }
213 - });
232 + })?;
233
215 - let update_block = update.map(|update| {
216 - builder.enter(BlockKind::Loop, |builder| {
217 - lower_expression_to_temporary(env, builder, update);
218 - TerminalValue::Goto(hir::GotoTerminal {
219 - block: test_block.id,
220 - kind: GotoKind::Break,
234 + let update_block = update
235 + .map(|update| {
236 + builder.enter(BlockKind::Loop, |builder| {
237 + lower_expression_to_temporary(env, builder, update)?;
238 + Ok(TerminalValue::Goto(hir::GotoTerminal {
239 + block: test_block.id,
240 + kind: GotoKind::Break,
241 + }))
242 })
243 })
223 - });
244 + .transpose()?;
245
246 let body_block = builder.enter(BlockKind::Block, |builder| {
247 let loop_ = LoopScope {
@@ -229,13 +250,13 @@ fn lower_statement<'a>(
250 break_block: fallthrough_block.id,
251 };
252 builder.enter_loop(loop_, |builder| {
232 - lower_statement(env, builder, body, None).unwrap();
233 - TerminalValue::Goto(hir::GotoTerminal {
253 + lower_statement(env, builder, body, None)?;
254 + Ok(TerminalValue::Goto(hir::GotoTerminal {
255 block: update_block.unwrap_or(test_block.id),
256 kind: GotoKind::Continue,
236 - })
257 + }))
258 })
238 - });
259 + })?;
260
261 let terminal = TerminalValue::For(ForTerminal {
262 body: body_block,
@@ -247,7 +268,7 @@ fn lower_statement<'a>(
268 builder.terminate_with_fallthrough(terminal, test_block);
269
270 if let Some(test) = test {
250 - let test_value = lower_expression_to_temporary(env, builder, test);
271 + let test_value = lower_expression_to_temporary(env, builder, test)?;
272 let terminal = TerminalValue::Branch(BranchTerminal {
273 test: test_value,
274 consequent: body_block,
@@ -255,7 +276,11 @@ fn lower_statement<'a>(
276 });
277 builder.terminate_with_fallthrough(terminal, fallthrough_block);
278 } else {
258 - panic!("Expected for statement to have a tesst block");
279 + return Err(BuildDiagnostic::new(
280 + DiagnosticError::ForStatementIsMissingTest,
281 + ErrorSeverity::Todo,
282 + stmt.range,
283 + ));
284 }
285 }
286 _ => todo!("Lower {stmt:#?}"),
@@ -268,9 +293,9 @@ fn lower_expression_to_temporary<'a>(
293 env: &'a Environment<'a>,
294 builder: &mut Builder<'a>,
295 expr: ExpressionLike,
271 -) -> Place<'a> {
272 - let value = lower_expression(env, builder, expr);
273 - lower_value_to_temporary(env, builder, value)
296 +) -> Result<Place<'a>, BuildDiagnostic> {
297 + let value = lower_expression(env, builder, expr)?;
298 + Ok(lower_value_to_temporary(env, builder, value))
299 }
300
301 /// Converts an ESTree Expression into an HIR InstructionValue. Note that while only a single
@@ -281,11 +306,11 @@ fn lower_expression<'a>(
306 env: &'a Environment<'a>,
307 builder: &mut Builder<'a>,
308 expr: ExpressionLike,
284 -) -> InstructionValue<'a> {
285 - match expr {
309 +) -> Result<InstructionValue<'a>, BuildDiagnostic> {
310 + Ok(match expr {
311 ExpressionLike::Identifier(expr) => {
312 // TODO: handle unbound variables
288 - let binding = builder.resolve_binding(&expr).unwrap();
313 + let binding = builder.resolve_binding(&expr)?;
314 match binding {
315 Binding::Local(identifier) => {
316 let place = Place {
@@ -303,23 +328,23 @@ fn lower_expression<'a>(
328 value: lower_primitive(env, builder, *expr),
329 }),
330 ExpressionLike::ArrayExpression(expr) => {
306 - let elements = expr
307 - .elements
308 - .into_iter()
309 - .map(|expr| match expr {
331 + let mut elements = Vec::with_capacity_in(expr.elements.len(), &env.allocator);
332 + for expr in expr.elements {
333 + let element = match expr {
334 ExpressionLike::SpreadElement(expr) => ArrayElement::Spread(
311 - lower_expression_to_temporary(env, builder, expr.argument),
335 + lower_expression_to_temporary(env, builder, expr.argument)?,
336 ),
313 - _ => ArrayElement::Place(lower_expression_to_temporary(env, builder, expr)),
314 - })
315 - .collect_in(env.allocator);
337 + _ => ArrayElement::Place(lower_expression_to_temporary(env, builder, expr)?),
338 + };
339 + elements.push(element);
340 + }
341 InstructionValue::Array(hir::Array { elements })
342 }
343
344 ExpressionLike::AssignmentExpression(expr) => match expr.operator {
345 estree::AssignmentOperator::Equals => {
321 - let right = lower_expression_to_temporary(env, builder, expr.right);
322 - lower_assignment(env, builder, InstructionKind::Reassign, expr.left, right)
346 + let right = lower_expression_to_temporary(env, builder, expr.right)?;
347 + lower_assignment(env, builder, InstructionKind::Reassign, expr.left, right)?
348 }
349 _ => todo!("lower assignment expr {:#?}", expr),
350 },
@@ -331,8 +356,8 @@ fn lower_expression<'a>(
356 right,
357 ..
358 } = *expr;
334 - let left = lower_expression_to_temporary(env, builder, left);
335 - let right = lower_expression_to_temporary(env, builder, right);
359 + let left = lower_expression_to_temporary(env, builder, left)?;
360 + let right = lower_expression_to_temporary(env, builder, right)?;
361 InstructionValue::Binary(hir::Binary {
362 left,
363 operator,
@@ -342,11 +367,15 @@ fn lower_expression<'a>(
367
368 // Cases that cannot appear in expression position but which are included in ExpressionLike
369 // to make serialization easier
345 - ExpressionLike::SpreadElement(_) => {
346 - panic!("SpreadElement may not appear in normal expression position")
370 + ExpressionLike::SpreadElement(expr) => {
371 + return Err(BuildDiagnostic::new(
372 + DiagnosticError::NonExpressionInExpressionPosition,
373 + ErrorSeverity::Invariant,
374 + expr.range,
375 + ));
376 }
377 _ => todo!("Lower expr {expr:#?}"),
349 - }
378 + })
379 }
380
381 fn lower_assignment<'a>(
@@ -355,11 +384,11 @@ fn lower_assignment<'a>(
384 kind: InstructionKind,
385 lvalue: AssignmentTarget,
386 value: Place<'a>,
358 -) -> InstructionValue<'a> {
359 - match lvalue {
387 +) -> Result<InstructionValue<'a>, BuildDiagnostic> {
388 + Ok(match lvalue {
389 AssignmentTarget::Pattern(lvalue) => match *lvalue {
390 Pattern::Identifier(lvalue) => {
362 - let place = lower_identifier_for_assignment(env, builder, kind, *lvalue).unwrap();
391 + let place = lower_identifier_for_assignment(env, builder, kind, *lvalue)?;
392 let temporary = lower_value_to_temporary(
393 env,
394 builder,
@@ -373,7 +402,7 @@ fn lower_assignment<'a>(
402 _ => todo!("lower assignment pattern for {:#?}", lvalue),
403 },
404 _ => todo!("lower assignment for {:#?}", lvalue),
376 - }
405 + })
406 }
407
408 fn lower_identifier_for_assignment<'a>(
@@ -381,11 +410,15 @@ fn lower_identifier_for_assignment<'a>(
410 builder: &mut Builder<'a>,
411 _kind: InstructionKind,
412 identifier: estree::Identifier,
384 -) -> Option<Place<'a>> {
413 +) -> Result<Place<'a>, BuildDiagnostic> {
414 let binding = builder.resolve_binding(&identifier)?;
415 match binding {
387 - Binding::Module(..) | Binding::Global => panic!("Cannot reassign a global"),
388 - Binding::Local(id) => Some(Place {
416 + Binding::Module(..) | Binding::Global => Err(BuildDiagnostic::new(
417 + DiagnosticError::ReassignedGlobal,
418 + ErrorSeverity::InvalidReact,
419 + identifier.range,
420 + )),
421 + Binding::Local(id) => Ok(Place {
422 identifier: id,
423 effect: None,
424 }),
@@ -440,5 +473,3 @@ fn lower_primitive<'a>(
473 _ => todo!("Lower literal {literal:#?}"),
474 }
475 }
443 -
444 -type Diagnostic = ();
compiler/forget/crates/build-hir/src/builder.rs
+73 -42
@@ -7,6 +7,8 @@ use hir::{
7 };
8 use indexmap::IndexMap;
9
10 +use crate::{invariant, BuildDiagnostic, DiagnosticError, ErrorSeverity};
11 +
12 /// Helper struct used when converting from ESTree to HIR. Includes:
13 /// - Variable resolution
14 /// - Label resolution (for labeled statements and break/continue)
@@ -46,7 +48,9 @@ pub(crate) enum Binding<'a> {
48 #[derive(Clone, PartialEq, Eq, Debug)]
49 enum ControlFlowScope<'a> {
50 Loop(LoopScope<'a>),
51 +
52 // Switch(SwitchScope<'a>),
53 + #[allow(dead_code)]
54 Label(LabelScope<'a>),
55 }
56
@@ -102,7 +106,7 @@ impl<'a> Builder<'a> {
106 ///
107 /// TODO: refine the type, only invariants should be possible here,
108 /// not other types of errors
105 - pub(crate) fn build(self) -> Result<HIR<'a>, Diagnostic> {
109 + pub(crate) fn build(self) -> Result<HIR<'a>, BuildDiagnostic> {
110 let mut hir = HIR {
111 entry: self.entry,
112 blocks: self.completed,
@@ -168,22 +172,34 @@ impl<'a> Builder<'a> {
172 }
173 }
174
171 - pub(crate) fn enter<F>(&mut self, kind: BlockKind, f: F) -> BlockId
175 + pub(crate) fn enter<F>(&mut self, kind: BlockKind, f: F) -> Result<BlockId, BuildDiagnostic>
176 where
173 - F: FnOnce(&mut Self) -> TerminalValue<'a>,
177 + F: FnOnce(&mut Self) -> Result<TerminalValue<'a>, BuildDiagnostic>,
178 {
179 let wip = self.reserve(kind);
180 let id = wip.id;
177 - self.enter_reserved(wip, f);
178 - id
181 + self.enter_reserved(wip, f)?;
182 + Ok(id)
183 }
184
181 - fn enter_reserved<F>(&mut self, wip: WipBlock<'a>, f: F)
185 + fn enter_reserved<F>(&mut self, wip: WipBlock<'a>, f: F) -> Result<(), BuildDiagnostic>
186 where
183 - F: FnOnce(&mut Self) -> TerminalValue<'a>,
187 + F: FnOnce(&mut Self) -> Result<TerminalValue<'a>, BuildDiagnostic>,
188 {
189 let current = std::mem::replace(&mut self.wip, wip);
186 - let terminal = f(self);
190 +
191 + let (result, terminal) = match f(self) {
192 + Ok(terminal) => (Ok(()), terminal),
193 + Err(error) => (
194 + Err(error),
195 + // TODO: add a `Terminal::Error` variant
196 + TerminalValue::Goto(hir::GotoTerminal {
197 + block: current.id,
198 + kind: GotoKind::Break,
199 + }),
200 + ),
201 + };
202 +
203 let completed = std::mem::replace(&mut self.wip, current);
204 self.completed.insert(
205 completed.id,
@@ -198,11 +214,16 @@ impl<'a> Builder<'a> {
214 predecessors: Default::default(),
215 },
216 );
217 + result
218 }
219
203 - pub(crate) fn enter_loop<F>(&mut self, scope: LoopScope<'a>, f: F) -> TerminalValue<'a>
220 + pub(crate) fn enter_loop<F>(
221 + &mut self,
222 + scope: LoopScope<'a>,
223 + f: F,
224 + ) -> Result<TerminalValue<'a>, BuildDiagnostic>
225 where
205 - F: FnOnce(&mut Self) -> TerminalValue<'a>,
226 + F: FnOnce(&mut Self) -> Result<TerminalValue<'a>, BuildDiagnostic>,
227 {
228 self.scopes.push(ControlFlowScope::Loop(scope.clone()));
229 let terminal = f(self);
@@ -230,7 +251,7 @@ impl<'a> Builder<'a> {
251 pub(crate) fn resolve_break(
252 &self,
253 label: Option<&estree::Identifier>,
233 - ) -> Result<BlockId, Diagnostic> {
254 + ) -> Result<BlockId, BuildDiagnostic> {
255 for scope in self.scopes.iter().rev() {
256 match (label, scope.label()) {
257 // If this is an unlabeled break, return the most recent break target
@@ -243,7 +264,11 @@ impl<'a> Builder<'a> {
264 _ => continue,
265 }
266 }
246 - Err(())
267 + Err(BuildDiagnostic::new(
268 + DiagnosticError::UnresolvedBreakTarget,
269 + ErrorSeverity::InvalidSyntax,
270 + None,
271 + ))
272 }
273
274 /// Resolves the target for the given continue label (if present), or returns the default
@@ -252,7 +277,7 @@ impl<'a> Builder<'a> {
277 pub(crate) fn resolve_continue(
278 &self,
279 label: Option<&estree::Identifier>,
255 - ) -> Result<BlockId, Diagnostic> {
280 + ) -> Result<BlockId, BuildDiagnostic> {
281 for scope in self.scopes.iter().rev() {
282 match scope {
283 ControlFlowScope::Loop(scope) => {
@@ -273,31 +298,46 @@ impl<'a> Builder<'a> {
298 match (label, scope.label()) {
299 (Some(label), Some(scope_label)) if label.name.as_str() == scope_label => {
300 // Error, the continue referred to a label that is not a loop
276 - return Err(());
301 + return Err(BuildDiagnostic::new(
302 + DiagnosticError::ContinueTargetIsNotALoop,
303 + ErrorSeverity::InvalidSyntax,
304 + None,
305 + ));
306 }
307 _ => continue,
308 }
309 }
310 }
311 }
283 - Err(())
312 + Err(BuildDiagnostic::new(
313 + DiagnosticError::UnresolvedContinueTarget,
314 + ErrorSeverity::InvalidSyntax,
315 + None,
316 + ))
317 }
318
319 pub(crate) fn resolve_binding(
320 &mut self,
321 identifier: &estree::Identifier,
289 - ) -> Option<Binding<'a>> {
290 - identifier.binding.as_ref().map(|binding| match binding {
291 - estree::Binding::Global => Binding::Global,
292 - estree::Binding::Local(id) => Binding::Local(
293 - self.environment
294 - .resolve_binding_identifier(&identifier.name, *id),
295 - ),
296 - estree::Binding::Module(id) => Binding::Module(
297 - self.environment
298 - .resolve_binding_identifier(&identifier.name, *id),
299 - ),
300 - })
322 + ) -> Result<Binding<'a>, BuildDiagnostic> {
323 + match &identifier.binding {
324 + Some(binding) => Ok(match binding {
325 + estree::Binding::Global => Binding::Global,
326 + estree::Binding::Local(id) => Binding::Local(
327 + self.environment
328 + .resolve_binding_identifier(&identifier.name, *id),
329 + ),
330 + estree::Binding::Module(id) => Binding::Module(
331 + self.environment
332 + .resolve_binding_identifier(&identifier.name, *id),
333 + ),
334 + }),
335 + _ => Err(BuildDiagnostic::new(
336 + DiagnosticError::UnknownIdentifier,
337 + ErrorSeverity::Invariant,
338 + identifier.range.clone(),
339 + )),
340 + }
341 }
342 }
343
@@ -403,13 +443,17 @@ fn remove_unreachable_do_while_statements<'a>(hir: &mut HIR<'a>) {
443
444 /// Updates the instruction ids for all instructions and blocks
445 /// Relies on the blocks being in reverse postorder to ensure that id ordering is correct
406 -fn mark_instruction_ids<'a>(hir: &mut HIR<'a>) -> Result<(), Diagnostic> {
446 +fn mark_instruction_ids<'a>(hir: &mut HIR<'a>) -> Result<(), BuildDiagnostic> {
447 let mut id_gen = InstructionIdGenerator::new();
448 let mut visited = HashSet::<(usize, usize)>::new();
449 for (block_ix, block) in hir.blocks.values_mut().enumerate() {
450 for (instr_ix, instr) in block.instructions.iter_mut().enumerate() {
451 invariant(visited.insert((block_ix, instr_ix)), || {
412 - format!("Expected bb{block_ix} i{instr_ix} not to have been visited yet")
452 + BuildDiagnostic::new(
453 + DiagnosticError::BlockVisitedTwice { block: block.id },
454 + ErrorSeverity::Invariant,
455 + None,
456 + )
457 })?;
458 instr.id = id_gen.next();
459 }
@@ -443,16 +487,3 @@ fn mark_predecessors<'a>(hir: &mut HIR<'a>) {
487 }
488 visit(hir.entry, None, hir, &mut visited);
489 }
446 -
447 -fn invariant<F>(cond: bool, f: F) -> Result<(), Diagnostic>
448 -where
449 - F: FnOnce() -> std::string::String,
450 -{
451 - if !cond {
452 - let msg = f();
453 - panic!("Invariant: {msg}");
454 - }
455 - Ok(())
456 -}
457 -
458 -type Diagnostic = ();
compiler/forget/crates/build-hir/src/error.rs new
+125
@@ -0,0 +1,125 @@
1 +use estree::SourceRange;
2 +use hir::BlockId;
3 +use miette::{ByteOffset, Diagnostic, SourceSpan};
4 +use thiserror::Error;
5 +
6 +#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Debug, Error)]
7 +pub enum ErrorSeverity {
8 + /// A feature that is intended to work but not yet implemented
9 + #[error("Not implemented")]
10 + Todo,
11 +
12 + /// Syntax that is valid but inentionally not supported
13 + #[error("Unsupported")]
14 + Unsupported,
15 +
16 + /// Invalid syntax
17 + #[error("Invalid JavaScript")]
18 + InvalidSyntax,
19 +
20 + /// Valid syntax, but invalid React
21 + #[error("Invalid React")]
22 + InvalidReact,
23 +
24 + /// Internal compiler error (ICE)
25 + #[error("Internal error")]
26 + Invariant,
27 +}
28 +
29 +/// Errors which can occur during HIR construction
30 +#[derive(Error, Diagnostic, Debug)]
31 +pub enum DiagnosticError {
32 + /// ErrorSeverity::Unsupported
33 + #[error(
34 + "Variable declarations must be `let` or `const`, `var` declarations are not supported"
35 + )]
36 + VariableDeclarationKindIsVar,
37 +
38 + /// ErrorSeverity::Invariant
39 + #[error("Invariant: Expected variable declaration to declare a fresh binding")]
40 + VariableDeclarationBindingIsNonLocal,
41 +
42 + /// ErrorSeverity::Todo
43 + #[error("`for` statements must have an initializer, eg `for (**let i = 0**; ...)`")]
44 + ForStatementIsMissingInitializer,
45 +
46 + /// ErrorSeverity::Todo
47 + #[error(
48 + "`for` statements must have a test condition, eg `for (let i = 0; **i < count**; ...)`"
49 + )]
50 + ForStatementIsMissingTest,
51 +
52 + /// ErrorSeverity::Invariant
53 + #[error("Invariant: Expected an expression node")]
54 + NonExpressionInExpressionPosition,
55 +
56 + /// ErrorSeverity::InvalidReact
57 + #[error("React functions may not reassign variables defined outside of the component or hook")]
58 + ReassignedGlobal,
59 +
60 + /// ErrorSeverity::Invariant
61 + #[error("Invariant: Expected block {block} not to have been visited yet")]
62 + BlockVisitedTwice { block: BlockId },
63 +
64 + /// ErrorSeverity::InvalidSyntax
65 + #[error("Could not resolve a target for `break` statement")]
66 + UnresolvedBreakTarget,
67 +
68 + /// ErrorSeverity::InvalidSyntax
69 + #[error("Could not resolve a target for `continue` statement")]
70 + UnresolvedContinueTarget,
71 +
72 + /// ErrorSeverity::InvalidSyntax
73 + #[error("Labeled `continue` statements must use the label of a loop statement")]
74 + ContinueTargetIsNotALoop,
75 +
76 + /// ErrorSeverity::Invariant
77 + #[error("Invariant: Identifier was not resolved (did name resolution run successfully?)")]
78 + UnknownIdentifier,
79 +}
80 +
81 +#[derive(Error, Diagnostic, Debug)]
82 +#[error("{error}")]
83 +pub struct BuildDiagnostic {
84 + /// The actual error
85 + pub error: DiagnosticError,
86 +
87 + /// Error severity
88 + pub severity: ErrorSeverity,
89 +
90 + /// Source of the error
91 + #[label]
92 + pub range: Option<SourceSpan>,
93 +}
94 +
95 +impl BuildDiagnostic {
96 + pub fn new(
97 + error: DiagnosticError,
98 + severity: ErrorSeverity,
99 + range: Option<SourceRange>,
100 + ) -> Self {
101 + Self {
102 + error,
103 + severity,
104 + range: range.map(|range| {
105 + SourceSpan::new(
106 + ByteOffset::from(range.start as usize - 1).into(),
107 + ByteOffset::from((u32::from(range.end) - range.start) as usize).into(),
108 + )
109 + }),
110 + }
111 + }
112 +}
113 +
114 +/// Returns Ok(()) if the condition is true, otherwise returns Err()
115 +/// with the diagnostic produced by the provided callback
116 +pub fn invariant<F>(cond: bool, f: F) -> Result<(), BuildDiagnostic>
117 +where
118 + F: FnOnce() -> BuildDiagnostic,
119 +{
120 + if cond {
121 + Ok(())
122 + } else {
123 + Err(f())
124 + }
125 +}
compiler/forget/crates/build-hir/src/lib.rs
+2
@@ -1,4 +1,6 @@
1 mod build;
2 mod builder;
3 +mod error;
4
5 pub use build::build;
6 +pub use error::*;
compiler/forget/crates/estree/Cargo.toml
-11
@@ -12,14 +12,3 @@ insta = { version = "1.30.0", features = ["glob"] }
12 serde = { version = "1.0.164", features = ["derive"] }
13 serde_json = "1.0.99"
14 static_assertions = "1.1.0"
15 -
16 -
17 -# Make insta run faster by compiling with release mode optimizations
18 -# https://docs.rs/insta/latest/insta/#optional-faster-runs
19 -[profile.dev.package.insta]
20 -opt-level = 3
21 -
22 -# Make insta diffing libary faster by compiling with release mode optimizations
23 -# https://docs.rs/insta/latest/insta/#optional-faster-runs
24 -[profile.dev.package.similar]
25 -opt-level = 3
\ No newline at end of file
compiler/forget/crates/estree/src/lib.rs
+2 -2
@@ -11,7 +11,7 @@ pub struct SourceLocation {
11 pub end: Position,
12 }
13
14 -#[derive(Serialize, Deserialize, Debug)]
14 +#[derive(Serialize, Deserialize, Debug, Clone)]
15 pub struct Position {
16 /// >= 1
17 pub line: NonZeroU32,
@@ -20,7 +20,7 @@ pub struct Position {
20 }
21 assert_eq_size!(Option<Position>, u64);
22
23 -#[derive(Serialize, Deserialize, Debug)]
23 +#[derive(Serialize, Deserialize, Debug, Clone)]
24 pub struct SourceRange {
25 pub start: u32,
26 // end is exclusive so it can always be non-zero. This allows
compiler/forget/crates/fixtures/Cargo.toml
+1
@@ -13,3 +13,4 @@ estree-swc = { path = "../estree-swc" }
13 hir = { path = "../hir" }
14 build-hir = { path = "../build-hir" }
15 bumpalo = { version = "3.13.0", features = ["collections"] }
16 +miette = { version = "5.9.0", features = ["backtrace", "fancy"] }
compiler/forget/crates/fixtures/tests/fixtures/error.assign-to-global.js new
+3
@@ -0,0 +1,3 @@
1 +function foo() {
2 + x = true;
3 +}
compiler/forget/crates/fixtures/tests/fixtures_test.rs
+19 -3
@@ -1,9 +1,12 @@
1 +use std::fmt::Write;
2 +
3 use build_hir::build;
4 use bumpalo::Bump;
5 use estree::{ModuleItem, Statement};
6 use estree_swc::parse;
7 use hir::{Environment, Print, Registry};
8 use insta::{assert_snapshot, glob};
9 +use miette::{NamedSource, Report};
10
11 #[test]
12 fn fixtures() {
@@ -24,12 +27,25 @@ fn fixtures() {
27 },
28 Registry,
29 ));
27 - let hir = build(&environment, *fun).unwrap();
28 -
30 if ix != 0 {
31 output.push_str("\n\n");
32 }
32 - hir.print(&mut output).unwrap();
33 + match build(&environment, *fun) {
34 + Ok(hir) => {
35 + hir.print(&mut output).unwrap();
36 + }
37 + Err(error) => {
38 + write!(&mut output, "{}", error,).unwrap();
39 + eprintln!(
40 + "{:?}",
41 + Report::new(error).with_source_code(NamedSource::new(
42 + path.to_string_lossy(),
43 + input.clone(),
44 + ))
45 + );
46 + continue;
47 + }
48 + };
49 }
50 }
51 }
compiler/forget/crates/fixtures/tests/snapshots/fixtures_test__fixtures@error.assign-to-global.js.snap new
+13
@@ -0,0 +1,13 @@
1 +---
2 +source: crates/fixtures/tests/fixtures_test.rs
3 +expression: "format!(\"Input:\\n{input}\\n\\nOutput:\\n{output}\")"
4 +input_file: crates/fixtures/tests/fixtures/error.assign-to-global.js
5 +---
6 +Input:
7 +function foo() {
8 + x = true;
9 +}
10 +
11 +
12 +Output:
13 +React functions may not reassign variables defined outside of the component or hook