Fix error spans for @satisfies

[Andarist]

No description provided.

[Andarist]

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?

[jakebailey]

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.

[jakebailey]

I don't think there's a better way. However, I think you could pull out a helper and avoid the nil call, like:

func findOriginatingJSDocSatisfiesTag(sourceFile *ast.SourceFile, node *ast.Node) *ast.Node {
	targetType := node.AsSatisfiesExpression().Type
	if targetType.Flags&ast.NodeFlagsReparsed == 0 {
		return nil
	}
	for current := node.Parent; current != nil; current = current.Parent {
		if current.Flags&ast.NodeFlagsHasJSDoc == 0 {
			continue
		}
		var firstSatisfiesTag *ast.Node
		for _, jsDoc := range current.JSDoc(sourceFile) {
			if tags := jsDoc.AsJSDoc().Tags; tags != nil {
				for _, tag := range tags.Nodes {
					if !ast.IsJSDocSatisfiesTag(tag) {
						continue
					}
					if firstSatisfiesTag == nil {
						firstSatisfiesTag = tag
					}
					if typeExpr := tag.AsJSDocSatisfiesTag().TypeExpression; typeExpr != nil {
						if t := typeExpr.Type(); t != nil && t.Loc == targetType.Loc {
							return tag
						}
					}
				}
			}
		}
		return firstSatisfiesTag
	}
	return nil
}

Or something.

[Copilot]

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 GetErrorRangeForNode to map reparsed SatisfiesExpression errors back to the JSDoc @satisfies tag.
• Change checkSatisfiesExpression to report assignability diagnostics on the SatisfiesExpression node (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.

[Copilot]

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.

Suggested change

	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)
	}

[Copilot]

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.

[jakebailey]

This is failing all the tests

[jakebailey]

Should this be EagerJSDoc to avoid parsing JSDoc in TS files with satisifes, or is that not what happens?

[Andarist]

This is already kinda guarded by NodeFlagsReparsed - so it shouldn't end up parsing JSDoc in TS files

[jakebailey]

Right, though we continue, so we are still going to walk up the entire tree to the SourceFile?

[Andarist]

This is only walked when there is an error to be raised - so it should hit some actual node with @satisfies before reaching the SourceFile

[jakebailey]

Fair, though I think I'd still like to see EagerJSDoc used just because the checker does not actually call JSDoc at all when working with these tags, I think?

[Andarist]

I just pushed out this change, so you'll have to approve the PR again

Forgot my comment

[Andarist]

This is failing all the tests

sorry about that, the failures crept in here after syncing with main ;p