Skip to content

feat: move nil check to call sites #658

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

SoulPancake
Copy link

@SoulPancake SoulPancake commented Mar 18, 2025

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 SoulPancake changed the title feat: moving nil check to call sites of IsFunctionLikeDeclaration feat: move nil check to call sites - IsFunctionLikeDeclaration Mar 18, 2025
@SoulPancake SoulPancake changed the title feat: move nil check to call sites - IsFunctionLikeDeclaration feat: move nil check to call sites Mar 18, 2025
@jakebailey jakebailey requested a review from rbuckton March 18, 2025 18:31
@SoulPancake
Copy link
Author

@rbuckton Can you take a look at this?

@SaadiSave
Copy link

SaadiSave commented May 7, 2025

@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.

@SoulPancake
Copy link
Author

@SaadiSave Agreed, I will add the nil check in the loop

@SaadiSave
Copy link

@SoulPancake is there a lint to enforce nil checks?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants