Conversation
| for current := node.Parent; current != nil; current = current.Parent { | ||
| for _, jsDoc := range current.JSDoc(nil) { | ||
| if tags := jsDoc.AsJSDoc().Tags; tags != nil { | ||
| for _, tag := range tags.Nodes { | ||
| if ast.IsJSDocSatisfiesTag(tag) { | ||
| pos := SkipTrivia(sourceFile.Text(), tag.TagName().Pos()) | ||
| return GetRangeOfTokenAtPosition(sourceFile, pos) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not super fond of this but I couldn't find a better way to go from a reparsed node to the original JSDoc right now, and I tried a bunch. Am I missing something?
There was a problem hiding this comment.
I'm not sure there's anything better, given we are in the scanner package and therefore if such a helper existed in astnav it would be unavailable.
But maybe @gabritto / @andrewbranch have a better thought on their minds.
There was a problem hiding this comment.
Pull request overview
Adjusts diagnostic locations for JSDoc @satisfies so errors point at the satisfies tag (and related spans/columns) instead of the reparsed type node, aligning baseline expectations for conformance tests.
Changes:
- Update
GetErrorRangeForNodeto map reparsedSatisfiesExpressionerrors back to the JSDoc@satisfiestag. - Change
checkSatisfiesExpressionto report assignability diagnostics on theSatisfiesExpressionnode (enabling the new error range behavior). - Refresh reference baseline outputs/diffs for several
checkJsdocSatisfiesTag*conformance cases.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/scanner/scanner.go | Special-cases error ranges for SatisfiesExpression with reparsed types to target the JSDoc @satisfies tag. |
| internal/checker/checker.go | Reports satisfies assignability diagnostics on the expression node to use the updated error-span logic. |
| testdata/baselines/reference/submodule/conformance/checkJsdocSatisfiesTag1.errors.txt | Updates expected error location/underline for @satisfies case. |
| testdata/baselines/reference/submodule/conformance/checkJsdocSatisfiesTag1.errors.txt.diff | Updates divergence diff for the same case. |
| testdata/baselines/reference/submodule/conformance/checkJsdocSatisfiesTag4.errors.txt | Updates expected error location/underline for @satisfies case. |
| testdata/baselines/reference/submodule/conformance/checkJsdocSatisfiesTag4.errors.txt.diff | Updates divergence diff for the same case. |
| testdata/baselines/reference/submodule/conformance/checkJsdocSatisfiesTag12.errors.txt | Updates expected error locations/underlines for multiple @satisfies scenarios. |
| testdata/baselines/reference/submodule/conformance/checkJsdocSatisfiesTag12.errors.txt.diff | Updates divergence diff for the same cases. |
| for current := node.Parent; current != nil; current = current.Parent { | ||
| for _, jsDoc := range current.JSDoc(nil) { | ||
| if tags := jsDoc.AsJSDoc().Tags; tags != nil { | ||
| for _, tag := range tags.Nodes { | ||
| if ast.IsJSDocSatisfiesTag(tag) { | ||
| pos := SkipTrivia(sourceFile.Text(), tag.TagName().Pos()) | ||
| return GetRangeOfTokenAtPosition(sourceFile, pos) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
When the satisfies type is reparsed, this returns the first @satisfies tag found on any ancestor JSDoc. If a JSDoc comment contains multiple @satisfies tags (which the reparser will apply by wrapping the initializer multiple times), this can highlight the wrong tag for inner/outer SatisfiesExpression nodes. Consider selecting the tag whose TypeExpression.Type().Loc (or pos/end) matches node.AsSatisfiesExpression().Type.Loc, and only fall back to the first tag if no match is found.
| for current := node.Parent; current != nil; current = current.Parent { | |
| for _, jsDoc := range current.JSDoc(nil) { | |
| if tags := jsDoc.AsJSDoc().Tags; tags != nil { | |
| for _, tag := range tags.Nodes { | |
| if ast.IsJSDocSatisfiesTag(tag) { | |
| pos := SkipTrivia(sourceFile.Text(), tag.TagName().Pos()) | |
| return GetRangeOfTokenAtPosition(sourceFile, pos) | |
| } | |
| } | |
| } | |
| } | |
| } | |
| targetType := node.AsSatisfiesExpression().Type | |
| var firstSatisfiesTag *ast.Node | |
| for current := node.Parent; current != nil; current = current.Parent { | |
| for _, jsDoc := range current.JSDoc(nil) { | |
| if tags := jsDoc.AsJSDoc().Tags; tags != nil { | |
| for _, tag := range tags.Nodes { | |
| if ast.IsJSDocSatisfiesTag(tag) { | |
| if firstSatisfiesTag == nil { | |
| firstSatisfiesTag = tag | |
| } | |
| typeExpr := tag.AsJSDocSatisfiesTag().TypeExpression() | |
| if typeExpr != nil { | |
| if typeNode := typeExpr.Type(); typeNode != nil { | |
| if typeNode.Pos() == targetType.Pos() && typeNode.End() == targetType.End() { | |
| pos := SkipTrivia(sourceFile.Text(), tag.TagName().Pos()) | |
| return GetRangeOfTokenAtPosition(sourceFile, pos) | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| if firstSatisfiesTag != nil { | |
| pos := SkipTrivia(sourceFile.Text(), firstSatisfiesTag.TagName().Pos()) | |
| return GetRangeOfTokenAtPosition(sourceFile, pos) | |
| } |
| for current := node.Parent; current != nil; current = current.Parent { | ||
| for _, jsDoc := range current.JSDoc(nil) { | ||
| if tags := jsDoc.AsJSDoc().Tags; tags != nil { | ||
| for _, tag := range tags.Nodes { | ||
| if ast.IsJSDocSatisfiesTag(tag) { | ||
| pos := SkipTrivia(sourceFile.Text(), tag.TagName().Pos()) | ||
| return GetRangeOfTokenAtPosition(sourceFile, pos) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
current.JSDoc(nil) triggers a GetSourceFileOfNode walk each time (since file is nil), and this code is already walking up the parent chain. This makes the reparsed-@satisfies path potentially O(depth^2). Consider computing sourceFile := ast.GetSourceFileOfNode(node) once and using current.JSDoc(sourceFile) inside the loop.
No description provided.