Apply code review

boa_engine/src/bytecompiler/mod.rs

@@ -1469,6 +1469,16 @@ impl<'b> ByteCompiler<'b> {
         Ok(())
     }
 
+    /// Compile a property access expression, prepending `this` to the property value in the stack.
+    ///
+    /// This compiles the access in a way that the state of the stack after executing the property
+    /// access becomes `...rest, this, value`. where `...rest` is the rest of the stack, `this` is the
+    /// `this` value of the access, and `value` is the final result of the access.
+    ///
+    /// This is mostly useful for optional chains with calls (`a.b?.()`) and for regular chains
+    /// with calls (`a.b()`), since both of them must have `a` be the value of `this` for the function
+    /// call `b()`, but a regular compilation of the access would lose the `this` value after accessing
+    /// `b`.
     fn compile_access_preserve_this(&mut self, access: &PropertyAccess) -> JsResult<()> {
         match access {
             PropertyAccess::Simple(access) => {
@@ -1511,6 +1521,17 @@ impl<'b> ByteCompiler<'b> {
         Ok(())
     }
 
+    /// Compile an optional chain expression, prepending `this` to the property value in the stack.
+    ///
+    /// This compiles the access in a way that the state of the stack after executing the optional
+    /// chain becomes `...rest, this, value`. where `...rest` is the rest of the stack, `this` is the
+    /// `this` value of the chain, and `value` is the result of the chain.
+    ///
+    /// This is mostly useful for inner optional chains with external calls (`(a?.b)()`), because the
+    /// external call is not in the optional chain, and compiling an optional chain in the usual way
+    /// would only return the result of the chain without preserving the `this` value. In other words,
+    /// `this` would be set to `undefined` for that call, which is incorrect since `a` should be the
+    /// `this` value of the call.
     fn compile_optional_preserve_this(&mut self, optional: &Optional) -> JsResult<()> {
         let mut jumps = Vec::with_capacity(optional.chain().len());
 
@@ -1554,6 +1575,22 @@ impl<'b> ByteCompiler<'b> {
         Ok(())
     }
 
+    /// Compile a single operation in an optional chain.
+    ///
+    /// On successful compilation, the state of the stack on execution will become `...rest, this, value`,
+    /// where `this` is the target of the property access (`undefined` on calls), and `value` is the
+    /// result of executing the action.
+    /// For example, in the expression `a?.b.c()`, after compiling and executing:
+    ///
+    /// - `a?.b`, the state of the stack will become `...rest, a, b`.
+    /// - `b.c`, the state of the stack will become `...rest, b, c`.
+    /// - `c()`, the state of the stack will become `...rest, undefined, c()`.
+    ///
+    /// # Requirements
+    /// - This should only be called after verifying that the previous value of the chain
+    /// is not null or undefined (if the operator `?.` was used).
+    /// - This assumes that the state of the stack before compiling is `...rest, this, value`,
+    /// since the operation compiled by this function could be a call.
     fn compile_optional_item_kind(&mut self, kind: &OptionalItemKind) -> JsResult<()> {
         match kind {
             OptionalItemKind::SimplePropertyAccess { field } => {

boa_engine/src/syntax/ast/expression/optional.rs

@@ -4,7 +4,7 @@ use crate::syntax::ast::{join_nodes, ContainsSymbol};
 
 use super::{access::PropertyAccessField, Expression};
 
-/// List of valid expressions in an [`Optional`] chain.
+/// List of valid operations in an [`Optional`] chain.
 #[cfg_attr(feature = "deser", derive(serde::Serialize, serde::Deserialize))]
 #[derive(Clone, Debug, PartialEq)]
 pub enum OptionalItemKind {
@@ -44,11 +44,11 @@ impl OptionalItemKind {
     }
 }
 
-/// Item within an [`Optional`] chain.
+/// Operation within an [`Optional`] chain.
 ///
-/// An item within an `Optional` chain can be either shorted or non-shorted. A shorted item
+/// An operation within an `Optional` chain can be either shorted or non-shorted. A shorted operation
 /// (`?.item`) will force the expression to return `undefined` if the target is `undefined` or `null`.
-/// In contrast, a non-shorted item (`.item`) will try to access the property, even if the target
+/// In contrast, a non-shorted operation (`.prop`) will try to access the property, even if the target
 /// is `undefined` or `null`.
 #[cfg_attr(feature = "deser", derive(serde::Serialize, serde::Deserialize))]
 #[derive(Clone, Debug, PartialEq)]
@@ -59,15 +59,19 @@ pub struct OptionalItem {
 
 impl OptionalItem {
     /// Creates a new `OptionalItem`.
+    #[inline]
     pub fn new(kind: OptionalItemKind, shorted: bool) -> Self {
         Self { kind, shorted }
     }
     /// Gets the kind of optional chain item.
+    #[inline]
     pub fn kind(&self) -> &OptionalItemKind {
         &self.kind
     }
 
-    /// Returns `true` if the item short-circuits the [`Optional`] chain when `undefined`/`null`.
+    /// Returns `true` if the item short-circuits the [`Optional`] chain when the target is
+    /// `undefined` or `null`.
+    #[inline]
     pub fn shorted(&self) -> bool {
         self.shorted
     }
@@ -84,7 +88,6 @@ impl OptionalItem {
 }
 
 impl ToInternedString for OptionalItem {
-    #[inline]
     fn to_interned_string(&self, interner: &Interner) -> String {
         let mut buf = if self.shorted {
             String::from("?.")
@@ -149,6 +152,7 @@ pub struct Optional {
 
 impl Optional {
     /// Creates a new `Optional` expression.
+    #[inline]
     pub fn new(target: Expression, chain: Box<[OptionalItem]>) -> Self {
         Self {
             target: Box::new(target),
@@ -157,11 +161,13 @@ impl Optional {
     }
 
     /// Gets the target of this `Optional` expression.
+    #[inline]
     pub fn target(&self) -> &Expression {
         self.target.as_ref()
     }
 
     /// Gets the chain of accesses and calls that will be applied to the target at runtime.
+    #[inline]
     pub fn chain(&self) -> &[OptionalItem] {
         self.chain.as_ref()
     }

boa_engine/src/syntax/parser/expression/left_hand_side/mod.rs

@@ -72,6 +72,15 @@ where
     type Output = Expression;
 
     fn parse(self, cursor: &mut Cursor<R>, interner: &mut Interner) -> ParseResult<Self::Output> {
+        /// Checks if we need to parse a super call expression `super()`.
+        ///
+        /// It first checks if the next token is `super`, and if it is, it checks if the second next
+        /// token is the open parenthesis (`(`) punctuator.
+        ///
+        /// This is needed because the `if let` chain is very complex, and putting it inline in the
+        /// initialization of `lhs` would make it very hard to return an expression over all
+        /// possible branches of the `if let`s. Instead, we extract the check into its own function,
+        /// then use it inside the condition of a simple `if ... else` expression.
         fn is_super_call<R: Read>(
             cursor: &mut Cursor<R>,
             interner: &mut Interner,