Skip to content

Commit f697667

Browse files
committed
fix: skip decorator factory when scanning for constructor parens
Address Gemini Code Assist review: when the member is a nameless Constructor and it carries a decorator factory (e.g. @dec()), using node.Pos() as the parameter-paren search origin matches the `(` of the decorator factory first and produces an inverted range. Anchor the scan at the first token after the decorators instead, keeping the prior behaviour for named methods/getters/setters via the name.End() override. Adds a utils-level regression test that asserts the range spans just the `constructor` keyword. Also sync the `require-yield` JS mirror test (and its snapshot) with the Go expectations updated in the previous commit — the columns now land after the decorators instead of on `@`.
1 parent 0b9e8e3 commit f697667

4 files changed

Lines changed: 86 additions & 17 deletions

File tree

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
package utils_test
2+
3+
import (
4+
"strings"
5+
"testing"
6+
7+
"github.com/microsoft/typescript-go/shim/ast"
8+
"github.com/web-infra-dev/rslint/internal/plugins/typescript/rules/fixtures"
9+
"github.com/web-infra-dev/rslint/internal/rule_tester"
10+
"github.com/web-infra-dev/rslint/internal/utils"
11+
)
12+
13+
// TestGetFunctionHeadLocConstructorWithDecoratorFactory pins down that the
14+
// reported range for a nameless member (constructor) whose class has a
15+
// decorator factory starts *after* the decorators and ends at the
16+
// parameter-list `(` — not at the decorator-factory `(`. Without skipping
17+
// the decorators when computing the parameter-search origin, the scan would
18+
// match the `(` inside `@Dec()` and produce an inverted range.
19+
func TestGetFunctionHeadLocConstructorWithDecoratorFactory(t *testing.T) {
20+
code := "declare function Dec(): any;\nclass A {\n @Dec()\n constructor(x: number) {}\n}\n"
21+
22+
helper := rule_tester.NewProgramHelper(fixtures.GetRootDir())
23+
_, sourceFile, err := helper.CreateTestProgram(code, "a.ts", "tsconfig.json")
24+
if err != nil {
25+
t.Fatalf("CreateTestProgram: %v", err)
26+
}
27+
28+
var ctor *ast.Node
29+
var walk func(n *ast.Node) bool
30+
walk = func(n *ast.Node) bool {
31+
if n == nil {
32+
return false
33+
}
34+
if n.Kind == ast.KindConstructor {
35+
ctor = n
36+
return true
37+
}
38+
stop := false
39+
n.ForEachChild(func(c *ast.Node) bool {
40+
if walk(c) {
41+
stop = true
42+
return true
43+
}
44+
return false
45+
})
46+
return stop
47+
}
48+
walk(sourceFile.AsNode())
49+
50+
if ctor == nil {
51+
t.Fatal("constructor not found in parsed source")
52+
}
53+
54+
r := utils.GetFunctionHeadLoc(sourceFile, ctor)
55+
if r.Pos() >= r.End() {
56+
t.Fatalf("inverted range: pos=%d end=%d", r.Pos(), r.End())
57+
}
58+
59+
got := sourceFile.Text()[r.Pos():r.End()]
60+
// "constructor" is 11 characters, the head range must cover just the
61+
// keyword up to (but not including) the parameter `(`.
62+
if strings.TrimSpace(got) != "constructor" {
63+
t.Fatalf("unexpected head range text: %q (pos=%d end=%d)", got, r.Pos(), r.End())
64+
}
65+
}

internal/utils/ts_eslint.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -196,9 +196,10 @@ func GetFunctionHeadLoc(sourceFile *ast.SourceFile, node *ast.Node) core.TextRan
196196
switch node.Kind {
197197
case ast.KindMethodDeclaration, ast.KindGetAccessor, ast.KindSetAccessor, ast.KindConstructor:
198198
start := nodeStartSkippingDecorators(sourceFile, node)
199-
// Start scanning for the parameters `(` after the method name to avoid
200-
// matching the `(` of a decorator factory call like `@dec()`.
201-
searchFrom := node.Pos()
199+
// Start scanning for the parameters `(` after any decorator factory
200+
// (e.g. `@dec()`) and after the method name. Nameless constructors
201+
// fall back to the first token after the decorators.
202+
searchFrom := start.Pos()
202203
if name := node.Name(); name != nil {
203204
searchFrom = name.End()
204205
}

packages/rslint-test-tools/tests/eslint/rules/__snapshots__/require-yield.test.ts.snap

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -669,7 +669,7 @@ class A { @dec *foo() { return 0; } }",
669669
"line": 2,
670670
},
671671
"start": {
672-
"column": 11,
672+
"column": 16,
673673
"line": 2,
674674
},
675675
},
@@ -696,7 +696,7 @@ class A { @dec() *foo() { return 0; } }",
696696
"line": 2,
697697
},
698698
"start": {
699-
"column": 11,
699+
"column": 18,
700700
"line": 2,
701701
},
702702
},
@@ -723,7 +723,7 @@ class A { @d1 @d2 *foo() { return 0; } }",
723723
"line": 2,
724724
},
725725
"start": {
726-
"column": 11,
726+
"column": 19,
727727
"line": 2,
728728
},
729729
},
@@ -750,7 +750,7 @@ class A { @dec public static *foo() { return 0; } }",
750750
"line": 2,
751751
},
752752
"start": {
753-
"column": 11,
753+
"column": 16,
754754
"line": 2,
755755
},
756756
},
@@ -777,7 +777,7 @@ class A { @dec async *foo() { return 0; } }",
777777
"line": 2,
778778
},
779779
"start": {
780-
"column": 11,
780+
"column": 16,
781781
"line": 2,
782782
},
783783
},
@@ -832,7 +832,7 @@ class A { @dec foo = function*() { return 0; }; }",
832832
"line": 2,
833833
},
834834
"start": {
835-
"column": 11,
835+
"column": 16,
836836
"line": 2,
837837
},
838838
},
@@ -863,7 +863,7 @@ class A {
863863
},
864864
"start": {
865865
"column": 3,
866-
"line": 3,
866+
"line": 4,
867867
},
868868
},
869869
"ruleName": "require-yield",

packages/rslint-test-tools/tests/eslint/rules/require-yield.test.ts

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -443,13 +443,16 @@ ruleTester.run('require-yield', {
443443
},
444444

445445
// ---- Decorator scenarios ----
446+
// GetFunctionHeadLoc mirrors typescript-eslint's getFunctionHeadLoc,
447+
// which excludes leading decorators from the reported range, so the
448+
// start column lands on the first token after the last decorator.
446449
{
447450
code: 'declare function dec(t: any, k: any, d: any): void;\nclass A { @dec *foo() { return 0; } }',
448451
errors: [
449452
{
450453
messageId: 'missingYield',
451454
line: 2,
452-
column: 11,
455+
column: 16,
453456
endLine: 2,
454457
endColumn: 20,
455458
},
@@ -461,7 +464,7 @@ ruleTester.run('require-yield', {
461464
{
462465
messageId: 'missingYield',
463466
line: 2,
464-
column: 11,
467+
column: 18,
465468
endLine: 2,
466469
endColumn: 22,
467470
},
@@ -473,7 +476,7 @@ ruleTester.run('require-yield', {
473476
{
474477
messageId: 'missingYield',
475478
line: 2,
476-
column: 11,
479+
column: 19,
477480
endLine: 2,
478481
endColumn: 23,
479482
},
@@ -485,7 +488,7 @@ ruleTester.run('require-yield', {
485488
{
486489
messageId: 'missingYield',
487490
line: 2,
488-
column: 11,
491+
column: 16,
489492
endLine: 2,
490493
endColumn: 34,
491494
},
@@ -497,7 +500,7 @@ ruleTester.run('require-yield', {
497500
{
498501
messageId: 'missingYield',
499502
line: 2,
500-
column: 11,
503+
column: 16,
501504
endLine: 2,
502505
endColumn: 26,
503506
},
@@ -521,7 +524,7 @@ ruleTester.run('require-yield', {
521524
{
522525
messageId: 'missingYield',
523526
line: 2,
524-
column: 11,
527+
column: 16,
525528
endLine: 2,
526529
endColumn: 31,
527530
},
@@ -532,7 +535,7 @@ ruleTester.run('require-yield', {
532535
errors: [
533536
{
534537
messageId: 'missingYield',
535-
line: 3,
538+
line: 4,
536539
column: 3,
537540
endLine: 4,
538541
endColumn: 7,

0 commit comments

Comments
 (0)