Bring back `SourceFile.EndOfFileToken` #1257

Andarist · 2025-06-21T13:01:20Z

No description provided.

internal/astnav/tokens_test.go

internal/parser/reparser.go

Andarist · 2025-06-21T13:07:10Z

testdata/baselines/reference/astnav/GetTouchingPropertyName.mapCode.ts.baseline.txt

tbh, I don't know how to assess if this is a correct change or not

Andarist · 2025-06-22T09:56:20Z

internal/parser/parser.go

+	if len(p.reparseList) > 0 {
+		nodes = append(nodes, p.reparseList...)
+		p.reparseList = nil
+		end = p.nodePos()


I'm not entirely sure if this dance with end is fully correct here. It seems to me that end for the statement list should get shifted if some reparsed nodes were pushed to it - otherwise some weird data mismatches would occur and the code would have to, likely, handle them in a special way. cc @sandersn

At first I thought you were wrong but after I tried replacing the mutation with a a panic I see you are right. I think it's the best option but it still might cause some weirdness when getTokenAtPosition should return an EOF. However, the reparsed nodes are marked as such and should be skipped by that code.

end should be a real location, so I would very much assume that we don't need to do anything here. The end of file token should always come after the statement list, and IIRC any reparsed nodes would have to as well, and so nothing special needs to happen?

Yeah, all of that makes sense and matches my expectations. I believe one of the previous iterations was not correct because it computed end before the reparsed nodes on the eof were handled. That's why I have changed the parseListIndex's return type - so I could get control over the assigned end position.

I've just done this because it intuitively made sense but I have not fully verified why the previous version of this code I had have not worked. And because of that, I just want the reviewers to be extra cautious here - even though I think this makes sense :)

I actually more confused as to why we need to append reparseList at all. I thought that was an internal detail of the reparser?

Hm, I guess that's where it sticks its statements, so.

In other words, I think what you've written here is actually all correct.

Or at least, at worst maybe this position should just be the old end before the EOF.

_packages/api/src/node.ts

jakebailey · 2025-06-23T16:48:05Z

testdata/baselines/reference/submodule/conformance/jsDeclarationsDefault.js.diff

+    export default _default;
+    export type default = string | number;


Hm, this looks like a syntax error. Curious whether or not this is a quick fix. What should have happened here is that the type was renamed to _default too. But, that wouldn't be in the parser, but the declaration transform.

I'll look into it. Btw. this doesn't correctly generate a unique identifier in Strada (:

// @allowJs: true // @checkJs: true // @target: es5 // @outDir: ./out // @declaration: true export const _default = class {} export default 12; /** * @typedef {string | number} default */

Playground Link is what I was testing; putting your code into that also seems to work?

In my sample, there is an explicit _default exported and the merged default export is assigned a _default name too. Both of those should refer to different identifiers/exports

jakebailey · 2025-06-23T17:37:24Z

testdata/baselines/reference/submoduleAccepted/conformance/importTag14.errors.txt.diff

+ ==== /foo.js (3 errors) ====
+     /** @import * as f from "./foo" with */
+                            ~~~~~~~
+!!! error TS2306: File '/foo.js' is not a module.


Weird, but we also have other follow-on errors anyway.

jakebailey · 2025-06-23T17:38:26Z

...eference/submoduleAccepted/conformance/jsDeclarationsFunctionPrototypeStatic.errors.txt.diff

+++ new.jsDeclarationsFunctionPrototypeStatic.errors.txt
+@@= skipped -0, +0 lines =@@
+-<no content>
+source.js(1,1): error TS2309: An export assignment cannot be used in a module with other exported elements.


This seems preexisting; the reparsed code triggers some other errors about addtional exports.

yep. I need to get back to the PR that fixes these now that declaration emit is done, and I'm done adding (most) JS features.

sandersn

Still reviewing baselines, but here are some out of order comments.

sandersn · 2025-06-23T14:03:15Z

internal/astnav/tokens.go

@@ -49,6 +49,9 @@ func getTokenAtPosition(
 	left := 0

 	testNode := func(node *ast.Node) int {
+		if node.Kind == ast.KindEndOfFile {
+			return 0


how does this translate from the code in services/utilities.ts? I don't see a specific EOF case in that code so I assume it's simplified from it in some way.

This is a good question. It's an indirect port of the EOF case checked by nodeContainsPosition. The related code in Corsa is rewritten sligthly so it's not easy to see this. There is a chance this second nodeContainsPosition call might have to be ported too... but, so far, I have not found a need for it (at least not in the case of EOF):

// first element whose start position is before the input and whose end position is after or equal to the input if (nodeContainsPosition(children[middle], start, end)) { if (children[middle - 1]) { // we want the _first_ element that contains the position, so left-recur if the prior node also contains the position if (nodeContainsPosition(children[middle - 1])) { return Comparison.GreaterThan; } } return Comparison.EqualTo; }

internal/astnav/tokens_test.go

sandersn · 2025-06-23T15:44:05Z

internal/parser/parser.go

+	if len(p.reparseList) > 0 {
+		nodes = append(nodes, p.reparseList...)
+		p.reparseList = nil
+		end = p.nodePos()


At first I thought you were wrong but after I tried replacing the mutation with a a panic I see you are right. I think it's the best option but it still might cause some weirdness when getTokenAtPosition should return an EOF. However, the reparsed nodes are marked as such and should be skipped by that code.

internal/parser/parser.go

internal/parser/reparser.go

sandersn · 2025-06-23T16:41:52Z

internal/parser/reparser.go

@@ -102,6 +106,9 @@ func (p *Parser) reparseUnhosted(tag *ast.Node, parent *ast.Node, jsDoc *ast.Nod
 	case ast.KindJSDocImportTag:
 		importTag := tag.AsJSDocImportTag()
 		importClause := importTag.ImportClause
+		if importClause == nil {


I think ModuleSpecifier is actually the critical property to require non-nil, although an import tag that imports no identifiers is pointless, so keep checking importClause for nil too.

I'm not sure if I understand correctly. Have you meant I should adjust it to this?

if importClause == nil || importTag.ModuleSpecifier == nil { break }

sandersn · 2025-06-23T18:00:08Z

testdata/baselines/reference/astnav/GetTouchingPropertyName.mapCode.ts.baseline.txt

@@ -1,3 +1,39 @@
+〚Positions: [729, 730]〛
+【TS: SourceFile [0, 9772)】
+《Go: EndOfFileToken [9770, 9772)》


@andrewbranch I don't know how to read these baselines, but this could be the difference in GetTokenAtPosition that I was worried about.

Later: no, there's no jsdoc at the end; this is just a difference in GetTokenAtPosition or something similar between Strada and Corsa. Based on my reading of the diff--still not an expert--it looks like it is incorrect and Strada is correct.

sandersn

Requesting changes based on my previous comments, since this batch is just guesses for why diffs have happened.

sandersn · 2025-06-23T18:44:32Z

testdata/baselines/reference/astnav/GetTouchingPropertyName.mapCode.ts.baseline.txt

@@ -1,3 +1,39 @@
+〚Positions: [729, 730]〛
+【TS: SourceFile [0, 9772)】
+《Go: EndOfFileToken [9770, 9772)》


Later: no, there's no jsdoc at the end; this is just a difference in GetTokenAtPosition or something similar between Strada and Corsa. Based on my reading of the diff--still not an expert--it looks like it is incorrect and Strada is correct.

sandersn · 2025-06-23T18:48:22Z

testdata/baselines/reference/submoduleAccepted/compiler/uniqueSymbolJs.errors.txt.diff

this error seems more accurate than the Strada one, although it's missing the suggestion to use a mapped type.

sandersn · 2025-06-23T18:54:47Z

testdata/baselines/reference/submoduleAccepted/conformance/callbackTag2.types.diff

this diff is because the type annotation is now a contextual type, not a type annotation that attaches to the arrow

sandersn · 2025-06-23T18:55:37Z

testdata/baselines/reference/submoduleAccepted/conformance/importTag13.errors.txt.diff

+                  ~~~~~~~~~~~~~~~~~~
+!!! error TS1141: String literal expected.


I don't know why this parse error is different, but it's not related to this PR

sandersn · 2025-06-23T19:03:00Z

...eference/submoduleAccepted/conformance/jsDeclarationsFunctionPrototypeStatic.errors.txt.diff

+++ new.jsDeclarationsFunctionPrototypeStatic.errors.txt
+@@= skipped -0, +0 lines =@@
+-<no content>
+source.js(1,1): error TS2309: An export assignment cannot be used in a module with other exported elements.


yep. I need to get back to the PR that fixes these now that declaration emit is done, and I'm done adding (most) JS features.

Bring back SourceFile.EndOfFileToken

2d49b7c

Andarist commented Jun 21, 2025

View reviewed changes

internal/astnav/tokens_test.go Outdated Show resolved Hide resolved

Andarist commented Jun 21, 2025

View reviewed changes

internal/parser/reparser.go Outdated Show resolved Hide resolved

Andarist commented Jun 21, 2025

View reviewed changes

Andarist added 5 commits June 21, 2025 22:15

update assert

2374111

regenerate fourslash test correctly

6607da5

add endOfFileToken getter

1571ba6

define childProperties for SourceFile

21b6a94

add endOfFileToken to the TS type

6210c99

Andarist mentioned this pull request Jun 22, 2025

Don't ignore completions at source file locations microsoft/TypeScript#61909

Open

inclue reparsed nodes at eof

a7b21cf

Andarist commented Jun 22, 2025

View reviewed changes

_packages/api/src/node.ts Show resolved Hide resolved

Andarist marked this pull request as ready for review June 22, 2025 10:02

sandersn self-requested a review June 23, 2025 13:54

sandersn self-assigned this Jun 23, 2025

Andrew magic

9f798cf

jakebailey reviewed Jun 23, 2025

View reviewed changes

sandersn reviewed Jun 23, 2025

View reviewed changes

sandersn requested changes Jun 23, 2025

View reviewed changes

address some CR comments

7e25056

		+ export default _default;
		+ export type default = string \| number;

		+ ~~~~~~~~~~~~~~~~~~
		+!!! error TS1141: String literal expected.

Bring back SourceFile.EndOfFileToken #1257

Are you sure you want to change the base?

Bring back SourceFile.EndOfFileToken #1257

Uh oh!

Conversation

Andarist commented Jun 21, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakebailey Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andarist Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandersn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandersn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Bring back `SourceFile.EndOfFileToken` #1257

Bring back `SourceFile.EndOfFileToken` #1257

jakebailey Jun 23, 2025 •

edited

Loading

Andarist Jun 23, 2025 •

edited

Loading