Fix parsing of optional chaining in new expressions (#393)

* Implement check for optional chaining syntax error in new expressions

* Fix decorator parsing and formatting in AST to JS conversion

* Minor code style and XML doc corrections

Author: adams85

src/Esprima/JavascriptParser.cs

@@ -39,6 +39,7 @@ public void Reset()
             InSwitch = false;
             Strict = false;
             AllowIdentifierEscape = false;
+            MemberAccessContext = MemberAccessContext.Unknown;
 
             Decorators.Clear();
             LabelSet.Clear();
@@ -75,6 +76,7 @@ public void ReleaseLargeBuffers()
         public bool InClassBody;
         public bool Strict;
         public bool AllowIdentifierEscape;
+        public MemberAccessContext MemberAccessContext;
 
         public ArrayList<Decorator> Decorators;
 
@@ -83,6 +85,13 @@ public void ReleaseLargeBuffers()
         public StrongBox<Token>? FirstCoverInitializedNameError;
     }
 
+    internal enum MemberAccessContext : byte
+    {
+        Unknown = 0,
+        NewExpressionCallee = 1,
+        Decorator = 2,
+    }
+
     [StringMatcher("=", "*=", "**=", "/=", "%=", "+=", "-=", "<<=", ">>=", ">>>=", "&=", "^=", "|=", "&&=", "||=", "??=")]
     private static partial bool IsAssignmentOperator(string id);
 
@@ -1645,7 +1654,11 @@ private Expression ParseNewExpression()
         }
         else
         {
+            var previousMemberAccessContext = _context.MemberAccessContext;
+            _context.MemberAccessContext = MemberAccessContext.NewExpressionCallee;
             var callee = IsolateCoverGrammar(_parseLeftHandSideExpression);
+            _context.MemberAccessContext = previousMemberAccessContext;
+
             var args = Match("(") ? ParseArguments() : new NodeList<Expression>();
             expr = new NewExpression(callee, args);
             _context.IsAssignmentTarget = false;
@@ -1827,22 +1840,40 @@ private Expression ParseLeftHandSideExpressionAllowCall()
         }
 
         var hasOptional = false;
+        var hasCall = false;
         while (true)
         {
             var optional = false;
             if (Match("?."))
             {
+                if (_context.MemberAccessContext == MemberAccessContext.Decorator)
+                {
+                    ThrowError(Messages.InvalidDecoratorMemberExpression);
+                }
+
                 optional = true;
                 hasOptional = true;
                 Expect("?.");
             }
 
             if (Match("("))
             {
+                if (hasCall && _context.MemberAccessContext == MemberAccessContext.Decorator)
+                {
+                    ThrowError(Messages.InvalidDecoratorMemberExpression);
+                }
+
+                hasCall = true;
+
                 expr = ParseCallExpression(maybeAsync, startMarker, expr, optional);
             }
             else if (Match("["))
             {
+                if (_context.MemberAccessContext == MemberAccessContext.Decorator)
+                {
+                    ThrowError(Messages.InvalidDecoratorMemberExpression);
+                }
+
                 _context.IsBindingElement = false;
                 _context.IsAssignmentTarget = !optional;
                 Expect("[");
@@ -1852,6 +1883,11 @@ private Expression ParseLeftHandSideExpressionAllowCall()
             }
             else if (_lookahead.Type == TokenType.Template && _lookahead.Head)
             {
+                if (_context.MemberAccessContext == MemberAccessContext.Decorator)
+                {
+                    ThrowError(Messages.InvalidDecoratorMemberExpression);
+                }
+
                 // Optional template literal is not included in the spec.
                 // https://github.com/tc39/proposal-optional-chaining/issues/54
                 if (optional)
@@ -1869,6 +1905,11 @@ private Expression ParseLeftHandSideExpressionAllowCall()
             }
             else if (Match(".") || optional)
             {
+                if (hasCall && _context.MemberAccessContext == MemberAccessContext.Decorator)
+                {
+                    ThrowError(Messages.InvalidDecoratorMemberExpression);
+                }
+
                 var previousAllowIdentifierEscape = _context.AllowIdentifierEscape;
 
                 _context.IsBindingElement = false;
@@ -1957,6 +1998,11 @@ private Expression ParseLeftHandSideExpression()
             var optional = false;
             if (Match("?."))
             {
+                if (_context.MemberAccessContext == MemberAccessContext.NewExpressionCallee)
+                {
+                    ThrowError(Messages.InvalidOptionalChainFromNewExpression);
+                }
+
                 optional = true;
                 hasOptional = true;
                 Expect("?.");
@@ -2028,7 +2074,11 @@ private Expression ParseUpdateExpression()
         }
         else
         {
+            var previousMemberAccessContext = _context.MemberAccessContext;
+            _context.MemberAccessContext = MemberAccessContext.Unknown;
             expr = InheritCoverGrammar(_parseLeftHandSideExpressionAllowCall);
+            _context.MemberAccessContext = previousMemberAccessContext;
+
             if (!_hasLineTerminator && _lookahead.Type == TokenType.Punctuator && (Match("++") || Match("--")))
             {
                 expr = ParsePostfixUnaryExpression(expr, startMarker);
@@ -2120,7 +2170,7 @@ private UnaryExpression ParseBasicUnaryExpression()
         {
             TolerateError(Messages.StrictDelete);
         }
-        if (_context.Strict && unaryExpr.Operator == UnaryOperator.Delete && unaryExpr.Argument is MemberExpression m && m.Property is PrivateIdentifier)
+        if (_context.Strict && unaryExpr.Operator == UnaryOperator.Delete && unaryExpr.Argument is MemberExpression { Property: PrivateIdentifier })
         {
             TolerateError(Messages.PrivateFieldNoDelete);
         }
@@ -2566,7 +2616,7 @@ private protected Expression ParseAssignmentExpression()
                 _context.IsAssignmentTarget = false;
                 _context.IsBindingElement = false;
 
-                var isAsync = expr is ArrowParameterPlaceHolder arrow && arrow.Async;
+                var isAsync = expr is ArrowParameterPlaceHolder { Async: true };
                 var result = ReinterpretAsCoverFormalsList(expr);
 
                 if (result != null)
@@ -4678,7 +4728,11 @@ private Decorator ParseDecorator()
         _context.AllowYield = true;
         _context.IsAsync = false;
 
+        var previousMemberAccessContext = _context.MemberAccessContext;
+        _context.MemberAccessContext = MemberAccessContext.Decorator;
         var expression = IsolateCoverGrammar(_parseLeftHandSideExpressionAllowCall);
+        _context.MemberAccessContext = previousMemberAccessContext;
+
         _context.Strict = previousStrict;
         _context.AllowYield = previousAllowYield;
         _context.IsAsync = previousIsAsync;
@@ -4991,7 +5045,10 @@ private ClassDeclaration ParseClassDeclarationCore(in Marker node, bool identifi
         if (MatchKeyword("extends"))
         {
             NextToken();
+            var previousMemberAccessContext = _context.MemberAccessContext;
+            _context.MemberAccessContext = MemberAccessContext.Unknown;
             superClass = IsolateCoverGrammar(_parseLeftHandSideExpressionAllowCall);
+            _context.MemberAccessContext = previousMemberAccessContext;
         }
 
         var classBody = ParseClassBody(hasSuperClass: superClass is not null);
@@ -5033,7 +5090,10 @@ private ClassExpression ParseClassExpression()
         if (MatchKeyword("extends"))
         {
             NextToken();
+            var previousMemberAccessContext = _context.MemberAccessContext;
+            _context.MemberAccessContext = MemberAccessContext.Unknown;
             superClass = IsolateCoverGrammar(_parseLeftHandSideExpressionAllowCall);
+            _context.MemberAccessContext = previousMemberAccessContext;
         }
 
         var classBody = ParseClassBody(hasSuperClass: superClass is not null);

src/Esprima/Messages.cs

@@ -1,6 +1,7 @@
 namespace Esprima;
 
 // Error messages should be identical to V8.
+// TODO: Replace the messages marked with "temporary" with the actual V8 messages once they become available (see https://github.com/v8/v8/blob/main/src/common/message-template.h).
 internal static class Messages
 {
     public const string ArgumentsNotAllowedInClassField = "'arguments' is not allowed in class field initializer or static initialization block";
@@ -22,8 +23,7 @@ internal static class Messages
     public const string DuplicateConstructor = "A class may only have one constructor";
     public const string DuplicateParameter = "Duplicate parameter name not allowed in this context";
     public const string DuplicateProtoProperty = "Duplicate __proto__ fields are not allowed in object literals";
-    // TODO: Replace this with the actual V8 message once it becomes available (see https://github.com/v8/v8/blob/main/src/common/message-template.h).
-    public const string DuplicateKeyInImportAttributes = "Import attributes has duplicate key '{0}'";
+    public const string DuplicateKeyInImportAttributes = "Import attributes has duplicate key '{0}'"; // temporary
     public const string ForInOfLoopInitializer = "'{0} loop variable declaration may not have an initializer";
     public const string GeneratorInLegacyContext = "Generator declarations are not allowed in legacy contexts";
     public const string IllegalBreak = "Illegal break statement";
@@ -32,13 +32,14 @@ internal static class Messages
     public const string IllegalImportDeclaration = "Unexpected token";
     public const string IllegalLanguageModeDirective = "Illegal 'use strict' directive in function with non-simple parameter list";
     public const string IllegalReturn = "Illegal return statement";
+    public const string InvalidDecoratorMemberExpression = "Invalid decorator member expression"; // temporary
     public const string InvalidEscapedReservedWord = "Keyword must not contain escaped characters";
-    public const string NoSemicolonAfterDecorator = "Decorators must not be followed by a semicolon.";
     public const string InvalidHexEscapeSequence = "Invalid hexadecimal escape sequence";
     public const string InvalidLHSInAssignment = "Invalid left-hand side in assignment";
     public const string InvalidLHSInForIn = "Invalid left-hand side in for-in";
     public const string InvalidLHSInForLoop = "Invalid left-hand side in for-loop";
     public const string InvalidModuleSpecifier = "Unexpected token";
+    public const string InvalidOptionalChainFromNewExpression = "Invalid optional chain from new expression";
     public const string InvalidRegExpFlags = "Invalid regular expression flags";
     public const string InvalidTaggedTemplateOnOptionalChain = "Invalid tagged template on optional chain";
     public const string InvalidUnicodeEscapeSequence = "Invalid Unicode escape sequence";
@@ -49,6 +50,7 @@ internal static class Messages
     public const string NewTargetNotAllowedHere = "new.target expression is not allowed here";
     public const string NoAsAfterImportNamespace = "Unexpected token";
     public const string NoCatchOrFinally = "Missing catch or finally after try";
+    public const string NoSemicolonAfterDecorator = "Decorators must not be followed by a semicolon.";
     public const string NumericSeparatorAfterLeadingZero = "Numeric separator can not be used after leading 0";
     public const string NumericSeparatorNotAllowedHere = "Numeric separator is not allowed here";
     public const string NumericSeparatorOneUnderscore = "Numeric separator must be exactly one underscore";

src/Esprima/Utils/AstToJavascriptConverter.Enums.cs

@@ -36,7 +36,11 @@ protected internal enum ExpressionFlags
         SpaceAfterBracketsRecommended = JavaScriptTextWriter.ExpressionFlags.SpaceAfterBracketsRecommended,
         SpaceAroundBracketsRecommended = JavaScriptTextWriter.ExpressionFlags.SpaceAroundBracketsRecommended,
 
-        IsMethod = 1 << 16,
+        IsRootExpression = 1 << 16,
+
+        IsMethod = 1 << 17,
+
+        IsInsideDecorator = 1 << 22, // automatically propagated to sub-expressions
 
         IsInAmbiguousInOperatorContext = 1 << 24, // automatically propagated to sub-expressions
 
@@ -52,6 +56,6 @@ protected internal enum ExpressionFlags
 
         IsInsideStatementExpression = 1 << 31, // automatically propagated to sub-expressions
 
-        IsInPotentiallyAmbiguousContext = IsInAmbiguousInOperatorContext | IsInsideArrowFunctionBody | IsInsideNewCallee | IsInsideLeftHandSideExpression | IsInsideStatementExpression,
+        IsInPotentiallyAmbiguousContext = IsInAmbiguousInOperatorContext | IsInsideArrowFunctionBody | IsInsideDecorator | IsInsideNewCallee | IsInsideLeftHandSideExpression | IsInsideStatementExpression,
     }
 }

src/Esprima/Utils/AstToJavascriptConverter.Helpers.cs

@@ -94,13 +94,7 @@ protected void VisitStatementListItem(Statement statement, int index, int count,
     [MethodImpl(MethodImplOptions.AggressiveInlining)]
     private protected static ExpressionFlags RootExpressionFlags(bool needsBrackets)
     {
-        return ExpressionFlags.IsLeftMost | needsBrackets.ToFlag(ExpressionFlags.NeedsBrackets);
-    }
-
-    [MethodImpl(MethodImplOptions.AggressiveInlining)]
-    private protected static ExpressionFlags LeftHandSideRootExpressionFlags(bool needsBrackets)
-    {
-        return ExpressionFlags.IsInsideLeftHandSideExpression | ExpressionFlags.IsLeftMostInLeftHandSideExpression | RootExpressionFlags(needsBrackets);
+        return ExpressionFlags.IsRootExpression | ExpressionFlags.IsLeftMost | needsBrackets.ToFlag(ExpressionFlags.NeedsBrackets);
     }
 
     [MethodImpl(MethodImplOptions.AggressiveInlining)]
@@ -127,7 +121,7 @@ protected ExpressionFlags PropagateExpressionFlags(ExpressionFlags flags)
             flags = flags & ~isLeftMostFlags | _currentExpressionFlags & isLeftMostFlags;
         }
 
-        // Propagates IsInsideStatementExpression, IsInsideArrowFunctionBody and IsInsideLeftHandSideExpression to current expression.
+        // Propagates IsInAmbiguousInOperatorContext and IsInside* flags to current expression.
         flags |= _currentExpressionFlags & ExpressionFlags.IsInPotentiallyAmbiguousContext;
 
         return flags;
@@ -144,9 +138,10 @@ protected ExpressionFlags DisambiguateExpression(Expression expression, Expressi
         if ((flags & ExpressionFlags.IsInPotentiallyAmbiguousContext) != 0)
         {
             if (flags.HasFlagFast(ExpressionFlags.IsInsideStatementExpression | ExpressionFlags.IsLeftMost) && ExpressionIsAmbiguousAsStatementExpression(expression) ||
-                flags.HasFlagFast(ExpressionFlags.IsInsideLeftHandSideExpression | ExpressionFlags.IsLeftMostInLeftHandSideExpression) && LeftHandSideExpressionIsParenthesized(expression) ||
                 flags.HasFlagFast(ExpressionFlags.IsInsideArrowFunctionBody | ExpressionFlags.IsLeftMostInArrowFunctionBody) && ExpressionIsAmbiguousAsArrowFunctionBody(expression) ||
-                flags.HasFlagFast(ExpressionFlags.IsInsideNewCallee | ExpressionFlags.IsLeftMostInNewCallee) && ExpressionIsAmbiguousAsNewCallee(expression))
+                flags.HasFlagFast(ExpressionFlags.IsInsideNewCallee | ExpressionFlags.IsLeftMostInNewCallee) && ExpressionIsAmbiguousAsNewCallee(expression) ||
+                flags.HasFlagFast(ExpressionFlags.IsInsideLeftHandSideExpression | ExpressionFlags.IsLeftMostInLeftHandSideExpression) && LeftHandSideExpressionIsParenthesized(expression) ||
+                flags.HasFlagFast(ExpressionFlags.IsInsideDecorator | ExpressionFlags.IsLeftMost) && DecoratorLeftMostExpressionIsParenthesized(expression, isRoot: flags.HasFlagFast(ExpressionFlags.IsRootExpression)))
             {
                 return (flags | ExpressionFlags.NeedsBrackets) & ~ExpressionFlags.IsInAmbiguousInOperatorContext;
             }
@@ -327,6 +322,22 @@ protected virtual bool LeftHandSideExpressionIsParenthesized(Expression expressi
         return false;
     }
 
+    protected virtual bool DecoratorLeftMostExpressionIsParenthesized(Expression expression, bool isRoot)
+    {
+        // https://tc39.es/proposal-decorators/
+
+        switch (expression.Type)
+        {
+            case Nodes.Identifier:
+            case Nodes.MemberExpression when expression.As<MemberExpression>() is { Computed: false }:
+                return false;
+            case Nodes.CallExpression:
+                return !isRoot;
+        }
+
+        return true;
+    }
+
     protected virtual bool ExpressionNeedsBracketsInList(Expression expression)
     {
         return expression.Type is

src/Esprima/Utils/AstToJavascriptConverter.cs

@@ -456,7 +456,7 @@ binaryExpression.Right is UnaryExpression rightUnaryExpression &&
             Writer.WriteKeyword("extends", TokenFlags.SurroundingSpaceRecommended, ref _writeContext);
 
             _writeContext.SetNodeProperty(nameof(classDeclaration.SuperClass), static node => node.As<ClassDeclaration>().SuperClass);
-            VisitRootExpression(classDeclaration.SuperClass, LeftHandSideRootExpressionFlags(needsBrackets: false));
+            VisitRootExpression(classDeclaration.SuperClass, ExpressionFlags.IsInsideLeftHandSideExpression | ExpressionFlags.IsLeftMostInLeftHandSideExpression | RootExpressionFlags(needsBrackets: false));
         }
 
         _writeContext.SetNodeProperty(nameof(classDeclaration.Body), static node => node.As<ClassDeclaration>().Body);
@@ -489,7 +489,7 @@ binaryExpression.Right is UnaryExpression rightUnaryExpression &&
             Writer.WriteKeyword("extends", TokenFlags.SurroundingSpaceRecommended, ref _writeContext);
 
             _writeContext.SetNodeProperty(nameof(classExpression.SuperClass), static node => node.As<ClassExpression>().SuperClass);
-            VisitRootExpression(classExpression.SuperClass, LeftHandSideRootExpressionFlags(needsBrackets: false));
+            VisitRootExpression(classExpression.SuperClass, ExpressionFlags.IsInsideLeftHandSideExpression | ExpressionFlags.IsLeftMostInLeftHandSideExpression | RootExpressionFlags(needsBrackets: false));
         }
 
         _writeContext.SetNodeProperty(nameof(classExpression.Body), static node => node.As<ClassExpression>().Body);
@@ -558,7 +558,7 @@ binaryExpression.Right is UnaryExpression rightUnaryExpression &&
         Writer.WritePunctuator("@", TokenFlags.Leading | (ParentNode is not Expression).ToFlag(TokenFlags.LeadingSpaceRecommended), ref _writeContext);
 
         _writeContext.SetNodeProperty(nameof(decorator.Expression), static node => node.As<Decorator>().Expression);
-        VisitRootExpression(decorator.Expression, LeftHandSideRootExpressionFlags(needsBrackets: false));
+        VisitRootExpression(decorator.Expression, ExpressionFlags.IsInsideDecorator | RootExpressionFlags(needsBrackets: false));
 
         Writer.SpaceRecommendedAfterLastToken();
 

src/Esprima/Utils/JavascriptTextWriter.Enums.cs

@@ -115,7 +115,7 @@ public enum StatementFlags
         /// The statement comes last in the current statement list (more precisely, it is the right-most part in the textual representation of the current statement list).
         /// </summary>
         /// <remarks>
-        /// In the the visitation handlers of <see cref="AstToJavaScriptConverter"/> the flag is interpreted differently: it indicates that the statement comes last in the parent statement.
+        /// In the visitation handlers of <see cref="AstToJavaScriptConverter"/> the flag is interpreted differently: it indicates that the statement comes last in the parent statement.
         /// (Upon visiting a statement, this flag of the parent and child statement gets combined to determine its effective value for the current statement list.)
         /// </remarks>
         IsRightMost = 1 << 2,
@@ -140,7 +140,7 @@ public enum ExpressionFlags
         /// The expression comes first in the current expression tree, more precisely, it is the left-most part in the textual representation of the currently visited expression tree (incl. brackets).
         /// </summary>
         /// <remarks>
-        /// In the the visitation handlers of <see cref="AstToJavaScriptConverter"/> the flag is interpreted differently: it indicates that the expression comes first in the parent expression.
+        /// In the visitation handlers of <see cref="AstToJavaScriptConverter"/> the flag is interpreted differently: it indicates that the expression comes first in the parent expression.
         /// (Upon visiting an expression, this flag of the parent and child expression gets combined to determine its effective value for the expression tree.)
         /// </remarks>
         IsLeftMost = 1 << 1,