fix(parser): improve diagnostics around modifier checks

crates/oxc_parser/src/diagnostics.rs

@@ -3,7 +3,7 @@ use std::borrow::Cow;
 use oxc_diagnostics::OxcDiagnostic;
 use oxc_span::Span;
 
-use crate::modifiers::Modifier;
+use crate::modifiers::{Modifier, ModifierKind};
 
 #[inline]
 fn ts_error<C, M>(code: C, message: M) -> OxcDiagnostic
@@ -536,6 +536,20 @@ pub fn modifier_cannot_be_used_here(modifier: &Modifier) -> OxcDiagnostic {
         .with_label(modifier.span)
 }
 
+#[cold]
+pub fn modifier_only_on_property_declaration_or_index_signature(
+    modifier: &Modifier,
+) -> OxcDiagnostic {
+    ts_error(
+        "1024",
+        format!(
+            "'{}' modifier can only appear on a property declaration or index signature.",
+            modifier.kind
+        ),
+    )
+    .with_label(modifier.span)
+}
+
 #[cold]
 pub fn accessibility_modifier_already_seen(modifier: &Modifier) -> OxcDiagnostic {
     ts_error("1028", "Accessibility modifier already seen.")
@@ -544,8 +558,15 @@ pub fn accessibility_modifier_already_seen(modifier: &Modifier) -> OxcDiagnostic
 }
 
 #[cold]
-pub fn export_modifier_must_precede_declare(modifier: &Modifier) -> OxcDiagnostic {
-    ts_error("1029", "'export' modifier must precede 'declare' modifier.").with_label(modifier.span)
+pub fn modifier_must_precede_other_modifier(
+    modifier: &Modifier,
+    other_modifier: ModifierKind,
+) -> OxcDiagnostic {
+    ts_error(
+        "1029",
+        format!("'{}' modifier must precede '{}' modifier.", modifier.kind, other_modifier),
+    )
+    .with_label(modifier.span)
 }
 
 #[cold]

crates/oxc_parser/src/js/class.rs

@@ -278,6 +278,23 @@ impl<'a> ParserImpl<'a> {
             for decorator in decorators {
                 self.error(diagnostics::decorators_are_not_valid_here(decorator.span));
             }
+
+            let mut has_seen_readonly_modifier = false;
+            for modifier in modifiers.iter() {
+                match modifier.kind {
+                    ModifierKind::Readonly => has_seen_readonly_modifier = true,
+                    ModifierKind::Static => {
+                        if has_seen_readonly_modifier {
+                            self.error(diagnostics::modifier_must_precede_other_modifier(
+                                modifier,
+                                ModifierKind::Readonly,
+                            ));
+                        }
+                    }
+                    _ => self.error(diagnostics::cannot_appear_on_an_index_signature(modifier)),
+                }
+            }
+
             return ClassElement::TSIndexSignature(
                 self.parse_index_signature_declaration(span, &modifiers),
             );
@@ -476,6 +493,21 @@ impl<'a> ParserImpl<'a> {
         let optional = optional_span.is_some();
 
         if generator || matches!(self.cur_kind(), Kind::LParen | Kind::LAngle) {
+            for modifier in modifiers.iter() {
+                match modifier.kind {
+                    ModifierKind::Declare => {
+                        self.error(diagnostics::cannot_appear_on_class_elements(modifier));
+                    }
+                    ModifierKind::Readonly => {
+                        self.error(
+                            diagnostics::modifier_only_on_property_declaration_or_index_signature(
+                                modifier,
+                            ),
+                        );
+                    }
+                    _ => {}
+                }
+            }
             return self.parse_method_declaration(
                 span, r#type, generator, name, computed, optional, modifiers, decorators,
             );

crates/oxc_parser/src/js/function.rs

@@ -49,9 +49,7 @@ impl<'a> ParserImpl<'a> {
         self.expect(Kind::LParen);
         let this_param = if self.is_ts && self.at(Kind::This) {
             let param = self.parse_ts_this_parameter();
-            if !self.at(Kind::RParen) {
-                self.expect(Kind::Comma);
-            }
+            self.bump(Kind::Comma);
             Some(param)
         } else {
             None

crates/oxc_parser/src/modifiers.rs

@@ -309,7 +309,10 @@ impl<'a> ParserImpl<'a> {
             self.bump_any();
             let modifier = self.modifier(kind, self.end_span(span));
             if modifier.kind == ModifierKind::Export {
-                self.error(diagnostics::export_modifier_must_precede_declare(&modifier));
+                self.error(diagnostics::modifier_must_precede_other_modifier(
+                    &modifier,
+                    ModifierKind::Declare,
+                ));
             }
             self.check_for_duplicate_modifiers(flags, &modifier);
             flags.set(modifier_flags, true);
@@ -468,14 +471,14 @@ impl<'a> ParserImpl<'a> {
     }
 
     fn check_for_duplicate_modifiers(&mut self, seen_flags: ModifierFlags, modifier: &Modifier) {
-        if seen_flags.contains(modifier.kind.into())
-            || (matches!(
-                modifier.kind,
-                ModifierKind::Public | ModifierKind::Protected | ModifierKind::Private
-            ) && seen_flags.intersects(
-                ModifierFlags::PUBLIC | ModifierFlags::PROTECTED | ModifierFlags::PRIVATE,
-            ))
+        if matches!(
+            modifier.kind,
+            ModifierKind::Public | ModifierKind::Protected | ModifierKind::Private
+        ) && seen_flags
+            .intersects(ModifierFlags::PUBLIC | ModifierFlags::PROTECTED | ModifierFlags::PRIVATE)
         {
+            self.error(diagnostics::accessibility_modifier_already_seen(modifier));
+        } else if seen_flags.contains(modifier.kind.into()) {
             self.error(diagnostics::modifier_already_seen(modifier));
         }
     }

crates/oxc_parser/src/ts/statement.rs

@@ -244,6 +244,18 @@ impl<'a> ParserImpl<'a> {
             /* permit_const_as_modifier */ true,
             /* stop_on_start_of_class_static_block */ false,
         );
+
+        if self.is_index_signature() {
+            self.verify_modifiers(
+                &modifiers,
+                ModifierFlags::READONLY,
+                diagnostics::cannot_appear_on_an_index_signature,
+            );
+            return TSSignature::TSIndexSignature(
+                self.parse_index_signature_declaration(span, &modifiers),
+            );
+        }
+
         self.verify_modifiers(
             &modifiers,
             ModifierFlags::READONLY,
@@ -258,12 +270,6 @@ impl<'a> ParserImpl<'a> {
             return self.parse_getter_setter_signature_member(span, TSMethodSignatureKind::Set);
         }
 
-        if self.is_index_signature() {
-            return TSSignature::TSIndexSignature(
-                self.parse_index_signature_declaration(span, &modifiers),
-            );
-        }
-
         self.parse_property_or_method_signature(span, &modifiers)
     }
 
@@ -555,14 +561,11 @@ impl<'a> ParserImpl<'a> {
 
     pub(crate) fn parse_ts_this_parameter(&mut self) -> TSThisParameter<'a> {
         let span = self.start_span();
-        self.parse_class_element_modifiers(true);
-
-        let this_span = self.start_span();
         self.bump_any();
-        let this = self.end_span(this_span);
+        let this_span = self.end_span(span);
 
         let type_annotation = self.parse_ts_type_annotation();
-        self.ast.ts_this_parameter(self.end_span(span), this, type_annotation)
+        self.ast.ts_this_parameter(self.end_span(span), this_span, type_annotation)
     }
 
     pub(crate) fn at_start_of_ts_declaration(&mut self) -> bool {

crates/oxc_parser/src/ts/types.rs

@@ -6,7 +6,7 @@ use oxc_syntax::operator::UnaryOperator;
 use crate::{
     Context, ParserImpl, diagnostics,
     lexer::Kind,
-    modifiers::{Modifier, ModifierFlags, ModifierKind, Modifiers},
+    modifiers::{ModifierFlags, ModifierKind, Modifiers},
 };
 
 use super::{super::js::FunctionKind, statement::CallOrConstructorSignature};
@@ -1178,6 +1178,15 @@ impl<'a> ParserImpl<'a> {
         let optional = self.eat(Kind::Question);
 
         if self.at(Kind::LParen) || self.at(Kind::LAngle) {
+            for modifier in modifiers.iter() {
+                if modifier.kind == ModifierKind::Readonly {
+                    self.error(
+                        diagnostics::modifier_only_on_property_declaration_or_index_signature(
+                            modifier,
+                        ),
+                    );
+                }
+            }
             let type_parameters = self.parse_ts_type_parameters();
             let (this_param, params) = self
                 .parse_formal_parameters(FunctionKind::Declaration, FormalParameterKind::Signature);
@@ -1213,11 +1222,6 @@ impl<'a> ParserImpl<'a> {
         span: u32,
         modifiers: &Modifiers<'a>,
     ) -> Box<'a, TSIndexSignature<'a>> {
-        self.verify_modifiers(
-            modifiers,
-            ModifierFlags::READONLY | ModifierFlags::STATIC,
-            diagnostics::cannot_appear_on_an_index_signature,
-        );
         self.expect(Kind::LBrack);
         let (params, comma_span) = self.parse_delimited_list(
             Kind::RBrack,
@@ -1270,91 +1274,6 @@ impl<'a> ParserImpl<'a> {
         }
     }
 
-    pub(crate) fn parse_class_element_modifiers(
-        &mut self,
-        is_constructor_parameter: bool,
-    ) -> Modifiers<'a> {
-        if !self.is_ts {
-            return Modifiers::empty();
-        }
-
-        let mut flags = ModifierFlags::empty();
-        let mut modifiers = None;
-
-        loop {
-            if !self.is_at_modifier(is_constructor_parameter) {
-                break;
-            }
-
-            if let Ok(kind) = ModifierKind::try_from(self.cur_kind()) {
-                let modifier = Modifier { kind, span: self.cur_token().span() };
-                let new_flag = ModifierFlags::from(kind);
-                if flags.contains(new_flag) {
-                    self.error(diagnostics::accessibility_modifier_already_seen(&modifier));
-                } else {
-                    flags.insert(new_flag);
-                    modifiers.get_or_insert_with(|| self.ast.vec()).push(modifier);
-                }
-            } else {
-                break;
-            }
-
-            self.bump_any();
-        }
-
-        Modifiers::new(modifiers, flags)
-    }
-
-    fn is_at_modifier(&mut self, is_constructor_parameter: bool) -> bool {
-        if !matches!(
-            self.cur_kind(),
-            Kind::Public
-                | Kind::Protected
-                | Kind::Private
-                | Kind::Static
-                | Kind::Abstract
-                | Kind::Readonly
-                | Kind::Declare
-                | Kind::Override
-                | Kind::Export
-        ) {
-            return false;
-        }
-
-        let checkpoint = self.checkpoint();
-        self.bump_any();
-        let next = self.cur_token();
-
-        if next.is_on_new_line() {
-            self.rewind(checkpoint);
-            return false;
-        }
-
-        let followed_by_any_member = matches!(next.kind(), Kind::PrivateIdentifier | Kind::LBrack)
-            || next.kind().is_literal_property_name();
-        if followed_by_any_member {
-            self.rewind(checkpoint);
-            return true;
-        }
-
-        let followed_by_class_member = !is_constructor_parameter && next.kind() == Kind::Star;
-        if followed_by_class_member {
-            self.rewind(checkpoint);
-            return true;
-        }
-
-        // allow `...` for error recovery
-        let followed_by_parameter = is_constructor_parameter
-            && matches!(next.kind(), Kind::LCurly | Kind::LBrack | Kind::Dot3);
-        if followed_by_parameter {
-            self.rewind(checkpoint);
-            return true;
-        }
-
-        self.rewind(checkpoint);
-        false
-    }
-
     fn parse_js_doc_unknown_or_nullable_type(&mut self) -> TSType<'a> {
         let span = self.start_span();
         self.bump_any(); // bump `?`

tasks/coverage/misc/fail/oxc-11713-13.ts

@@ -1,2 +1,3 @@
-const [index: string]: number
-}
+interface Foo {
+  const [index: string]: number
+}

tasks/coverage/misc/fail/oxc-11713-23.ts

@@ -0,0 +1,3 @@
+interface Foo {
+  readonly method();
+}

tasks/coverage/misc/fail/oxc-11713-24.ts

@@ -0,0 +1,3 @@
+class Foo {
+  declare method() {}
+}

tasks/coverage/misc/fail/oxc-11713-25.ts

@@ -0,0 +1,3 @@
+class Foo {
+  readonly method() {}
+}

tasks/coverage/misc/fail/oxc-11713-26.ts

@@ -0,0 +1 @@
+function foo(readonly parameter) {}

tasks/coverage/misc/fail/oxc-11713-27.ts

@@ -0,0 +1 @@
+class Foo { method(readonly parameter) {} }