microsoft
feat: move nil check to call sites
This addresses
// TODO(rbuckton): Move node != niltest to call sites
@rbuckton Can you please review this
I can undo the stylistic whitespace changes in the internal/ast/utilities.g
but I think they're for the better, LMK what you think
@SoulPancake Sorry for jumping on here for a slightly tangential issue, but I noticed that nil checks are inconsistent throughout the codebase. For example,
https://github.com/microsoft/typescript-go/blob/main/internal%2Fast%2Fast.go#L15-L31
type Visitor func(*Node) bool
func visit(v Visitor, node *Node) bool {
if node != nil {
return v(node)
}
return false
}
func visitNodes(v Visitor, nodes []*Node) bool {
for _, node := range nodes {
if v(node) {
return true
}
}
return false
}
visit checks for nils, but the for loop doesn't check if node is nil. Do you have clear documentation on whether Visitors are supposed to handle nils? Because visit assumes they don't, and visitNodes assumes they do, which are mutually incompatible assumptions.
This PR has gotten out of date as main changed. Could you update it, or close it if you don't plan on working on this anymore?
I feel like most of these are redundant; I would think we'd only want ones that correspond to undefined checks in the original code.
@jakebailey I guess these aren't needed or helpful as you mentioned, do you suggest I remove the TODO comments and remove the changes?