Set parents during parse, not bind (#1251)

Author: jakebailey

internal/ast/utilities.go

@@ -904,8 +904,7 @@ func newParentInChildrenSetter() func(node *Node) bool {
 	}
 
 	state.visit = func(node *Node) bool {
-		if state.parent != nil && node.Parent != state.parent {
-			// Avoid data races on no-ops
+		if state.parent != nil {
 			node.Parent = state.parent
 		}
 		saveParent := state.parent

internal/binder/binder.go

@@ -46,7 +46,6 @@ type Binder struct {
 	unreachableFlow         *ast.FlowNode
 	reportedUnreachableFlow *ast.FlowNode
 
-	parent                 *ast.Node
 	container              *ast.Node
 	thisContainer          *ast.Node
 	blockScopeContainer    *ast.Node
@@ -211,9 +210,6 @@ func (b *Binder) declareSymbolEx(symbolTable ast.SymbolTable, parent *ast.Symbol
 			} else if !(includes&ast.SymbolFlagsVariable != 0 && symbol.Flags&ast.SymbolFlagsAssignment != 0 ||
 				includes&ast.SymbolFlagsAssignment != 0 && symbol.Flags&ast.SymbolFlagsVariable != 0) {
 				// Assignment declarations are allowed to merge with variables, no matter what other flags they have.
-				if node.Name() != nil {
-					setParent(node.Name(), node)
-				}
 				// Report errors every position with duplicate declaration
 				// Report errors on previous encountered declarations
 				var message *diagnostics.Message
@@ -584,9 +580,6 @@ func (b *Binder) bind(node *ast.Node) bool {
 	if node == nil {
 		return false
 	}
-	if node.Parent == nil || node.Parent.Flags&ast.NodeFlagsReparsed != 0 {
-		node.Parent = b.parent
-	}
 	saveInStrictMode := b.inStrictMode
 	// Even though in the AST the jsdoc @typedef node belongs to the current node,
 	// its symbol might be in the same scope with the current node's symbol. Consider:
@@ -731,9 +724,7 @@ func (b *Binder) bind(node *ast.Node) bool {
 	// children, as an optimization we don't process those.
 	thisNodeOrAnySubnodesHasError := node.Flags&ast.NodeFlagsThisNodeHasError != 0
 	if node.Kind > ast.KindLastToken {
-		saveParent := b.parent
 		saveSeenParseError := b.seenParseError
-		b.parent = node
 		b.seenParseError = false
 		containerFlags := GetContainerFlags(node)
 		if containerFlags == ContainerFlagsNone {
@@ -744,15 +735,7 @@ func (b *Binder) bind(node *ast.Node) bool {
 		if b.seenParseError {
 			thisNodeOrAnySubnodesHasError = true
 		}
-		b.parent = saveParent
 		b.seenParseError = saveSeenParseError
-	} else {
-		saveParent := b.parent
-		if node.Kind == ast.KindEndOfFile {
-			b.parent = node
-		}
-		b.setJSDocParents(node)
-		b.parent = saveParent
 	}
 	if thisNodeOrAnySubnodesHasError {
 		node.Flags |= ast.NodeFlagsThisNodeOrAnySubNodesHasError
@@ -762,16 +745,6 @@ func (b *Binder) bind(node *ast.Node) bool {
 	return false
 }
 
-func (b *Binder) setJSDocParents(node *ast.Node) {
-	for _, jsdoc := range node.JSDoc(b.file) {
-		setParent(jsdoc, node)
-		if jsdoc.Kind != ast.KindJSDocImportTag {
-			// JSDocImportTag children have parents set during parsing for module resolution purposes.
-			ast.SetParentInChildren(jsdoc)
-		}
-	}
-}
-
 func (b *Binder) bindPropertyWorker(node *ast.Node) {
 	isAutoAccessor := ast.IsAutoAccessorPropertyDeclaration(node)
 	includes := core.IfElse(isAutoAccessor, ast.SymbolFlagsAccessor, ast.SymbolFlagsProperty)
@@ -868,9 +841,6 @@ func (b *Binder) bindExportDeclaration(node *ast.Node) {
 		// All export * declarations are collected in an __export symbol
 		b.declareSymbol(ast.GetExports(b.container.Symbol()), b.container.Symbol(), node, ast.SymbolFlagsExportStar, ast.SymbolFlagsNone)
 	} else if ast.IsNamespaceExport(decl.ExportClause) {
-		// declareSymbol walks up parents to find name text, parent _must_ be set
-		// but won't be set by the normal binder walk until `bindChildren` later on.
-		setParent(decl.ExportClause, node)
 		b.declareSymbol(ast.GetExports(b.container.Symbol()), b.container.Symbol(), decl.ExportClause, ast.SymbolFlagsAlias, ast.SymbolFlagsAliasExcludes)
 	}
 }
@@ -981,7 +951,6 @@ func (b *Binder) bindClassLikeDeclaration(node *ast.Node) {
 	prototypeSymbol := b.newSymbol(ast.SymbolFlagsProperty|ast.SymbolFlagsPrototype, "prototype")
 	symbolExport := ast.GetExports(symbol)[prototypeSymbol.Name]
 	if symbolExport != nil {
-		setParent(name, node)
 		b.errorOnNode(symbolExport.Declarations[0], diagnostics.Duplicate_identifier_0, ast.SymbolName(prototypeSymbol))
 	}
 	ast.GetExports(symbol)[prototypeSymbol.Name] = prototypeSymbol
@@ -1026,9 +995,6 @@ func (b *Binder) bindExpandoPropertyAssignment(node *ast.Node) {
 		symbol = b.lookupEntity(parent, b.container)
 	}
 	if symbol = getInitializerSymbol(symbol); symbol != nil {
-		// Fix up parent pointers since we're going to use these nodes before we bind into them
-		setParent(expr.Left, node)
-		setParent(expr.Right, node)
 		if ast.HasDynamicName(node) {
 			b.bindAnonymousDeclaration(node, ast.SymbolFlagsProperty|ast.SymbolFlagsAssignment, ast.InternalSymbolNameComputed)
 			addLateBoundAssignmentDeclarationToSymbol(node, symbol)
@@ -1599,7 +1565,6 @@ func (b *Binder) bindChildren(node *ast.Node) {
 	// and set it before we descend into nodes that could actually be part of an assignment pattern.
 	b.inAssignmentPattern = false
 	if b.checkUnreachable(node) {
-		b.setJSDocParents(node)
 		b.bindEachChild(node)
 		b.inAssignmentPattern = saveInAssignmentPattern
 		return
@@ -1611,7 +1576,6 @@ func (b *Binder) bindChildren(node *ast.Node) {
 			hasFlowNodeData.FlowNode = b.currentFlow
 		}
 	}
-	b.setJSDocParents(node)
 	switch node.Kind {
 	case ast.KindWhileStatement:
 		b.bindWhileStatement(node)
@@ -2800,12 +2764,6 @@ func (b *Binder) addDiagnostic(diagnostic *ast.Diagnostic) {
 	b.file.SetBindDiagnostics(append(b.file.BindDiagnostics(), diagnostic))
 }
 
-func setParent(child *ast.Node, parent *ast.Node) {
-	if child != nil {
-		child.Parent = parent
-	}
-}
-
 func isSignedNumericLiteral(node *ast.Node) bool {
 	if node.Kind == ast.KindPrefixUnaryExpression {
 		node := node.AsPrefixUnaryExpression()

internal/execute/tsc.go

@@ -78,7 +78,6 @@ func fmtMain(sys System, input, output string) ExitStatus {
 		Path:             pathified,
 		JSDocParsingMode: ast.JSDocParsingModeParseAll,
 	}, text, core.GetScriptKindFromFileName(string(pathified)))
-	ast.SetParentInChildren(sourceFile.AsNode())
 	edits := format.FormatDocument(ctx, sourceFile)
 	newText := applyBulkEdits(text, edits)
 

internal/format/api_test.go

@@ -59,7 +59,6 @@ func TestFormat(t *testing.T) {
 			FileName: "/checker.ts",
 			Path:     "/checker.ts",
 		}, text, core.ScriptKindTS)
-		ast.SetParentInChildren(sourceFile.AsNode())
 		edits := format.FormatDocument(ctx, sourceFile)
 		newText := applyBulkEdits(text, edits)
 		assert.Assert(t, len(newText) > 0)
@@ -89,7 +88,6 @@ func BenchmarkFormat(b *testing.B) {
 		FileName: "/checker.ts",
 		Path:     "/checker.ts",
 	}, text, core.ScriptKindTS)
-	ast.SetParentInChildren(sourceFile.AsNode())
 
 	b.Run("format checker.ts", func(b *testing.B) {
 		for b.Loop() {

internal/parser/jsdoc.go

@@ -82,11 +82,6 @@ func (p *Parser) parseJSDocTypeExpression(mayOmitBraces bool) *ast.Node {
 	}
 
 	result := p.factory.NewJSDocTypeExpression(t)
-	// normally parent references are set during binding. However, for clients that only need
-	// a syntax tree, and no semantic features, then the binding process is an unnecessary
-	// overhead.  This functions allows us to set all the parents, without all the expense of
-	// binding.
-	ast.SetParentInChildren(result)
 	p.finishNode(result, pos)
 	return result
 }
@@ -107,7 +102,6 @@ func (p *Parser) parseJSDocNameReference() *ast.Node {
 	}
 
 	result := p.factory.NewJSDocNameReference(entityName)
-	ast.SetParentInChildren(result)
 	p.finishNode(result, pos)
 	return result
 }
@@ -145,7 +139,6 @@ func (p *Parser) parseJSDocComment(parent *ast.Node, start int, end int, fullSta
 	p.parsingContexts = p.parsingContexts | ParsingContexts(PCJSDocComment)
 
 	comment := p.parseJSDocCommentWorker(start, end, fullStart, initialIndent)
-	comment.Parent = parent
 	// move jsdoc diagnostics to jsdocDiagnostics -- for JS files only
 	if p.contextFlags&ast.NodeFlagsJavaScriptFile != 0 {
 		p.jsdocDiagnostics = append(p.jsdocDiagnostics, p.diagnostics[saveDiagnosticsLength:]...)
@@ -457,7 +450,6 @@ func (p *Parser) parseTag(tags []*ast.Node, margin int) *ast.Node {
 		tag = p.parseSeeTag(start, tagName, margin, indentText)
 	case "import":
 		tag = p.parseImportTag(start, tagName, margin, indentText)
-		ast.SetParentInChildren(tag)
 	default:
 		tag = p.parseUnknownTag(start, tagName, margin, indentText)
 	}

internal/parser/parser.go

@@ -352,6 +352,15 @@ func (p *Parser) finishSourceFile(result *ast.SourceFile, isDeclarationFile bool
 	result.TextCount = p.factory.TextCount()
 	result.IdentifierCount = p.identifierCount
 	result.SetJSDocCache(p.jsdocCache)
+
+	ast.SetParentInChildren(result.AsNode())
+	for parent, children := range p.jsdocCache {
+		for _, child := range children {
+			child.Parent = parent
+			ast.SetParentInChildren(child)
+		}
+	}
+
 	ast.SetExternalModuleIndicator(result, p.opts.ExternalModuleIndicatorOptions)
 }
 

internal/parser/references.go

@@ -15,7 +15,6 @@ func collectExternalModuleReferences(file *ast.SourceFile) {
 
 	if file.Flags&ast.NodeFlagsPossiblyContainsDynamicImport != 0 || ast.IsInJSFile(file.AsNode()) {
 		ast.ForEachDynamicImportOrRequireCall(file /*includeTypeSpaceImports*/, true /*requireStringLiteralLikeArgument*/, true, func(node *ast.Node, moduleSpecifier *ast.Expression) bool {
-			ast.SetParentInChildren(node) // we need parent data on imports before the program is fully bound, so we ensure it's set here
 			ast.SetImportsOfSourceFile(file, append(file.Imports(), moduleSpecifier))
 			return false
 		})
@@ -31,9 +30,6 @@ func collectModuleReferences(file *ast.SourceFile, node *ast.Statement, inAmbien
 		if moduleNameExpr != nil && ast.IsStringLiteral(moduleNameExpr) {
 			moduleName := moduleNameExpr.AsStringLiteral().Text
 			if moduleName != "" && (!inAmbientModule || !tspath.IsExternalModuleNameRelative(moduleName)) {
-				if node.Kind != ast.KindJSImportDeclaration {
-					ast.SetParentInChildren(node) // we need parent data on imports before the program is fully bound, so we ensure it's set here
-				}
 				ast.SetImportsOfSourceFile(file, append(file.Imports(), moduleNameExpr))
 				// !!! removed `&& p.currentNodeModulesDepth == 0`
 				if file.UsesUriStyleNodeCoreModules != core.TSTrue && !file.IsDeclarationFile {
@@ -50,7 +46,6 @@ func collectModuleReferences(file *ast.SourceFile, node *ast.Statement, inAmbien
 		return
 	}
 	if ast.IsModuleDeclaration(node) && ast.IsAmbientModule(node) && (inAmbientModule || ast.HasSyntacticModifier(node, ast.ModifierFlagsAmbient) || file.IsDeclarationFile) {
-		ast.SetParentInChildren(node)
 		nameText := node.AsModuleDeclaration().Name().Text()
 		// Ambient module declarations can be interpreted as augmentations for some existing external modules.
 		// This will happen in two cases: