Skip to content

Misc customlint updates#2643

Open
jakebailey wants to merge 10 commits intomainfrom
jabaile/tools-refactor
Open

Misc customlint updates#2643
jakebailey wants to merge 10 commits intomainfrom
jabaile/tools-refactor

Conversation

@jakebailey
Copy link
Member

These are some refactors I've had on the mind for a while but never wrote down / had copilot do. Might as well send them...

Copilot AI review requested due to automatic review settings February 2, 2026 19:01
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the customlint analyzers with the following changes: updates to use newer inspector APIs, removes defensive nil checks and type assertions, improves panic messages, adds test coverage, and fixes the plugin load mode.

Changes:

  • Refactored all analyzers to use inspector.Root().Preorder() instead of older inspect.Preorder() or inspect.PreorderSeq() APIs
  • Removed defensive nil checks and type assertions throughout the codebase, replacing them with fail-fast panics
  • Added comprehensive test cases to improve code coverage for unexported API and shadow checkers
  • Changed plugin load mode from LoadModeSyntax to LoadModeTypesInfo to match analyzer requirements

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
_tools/customlint/unexportedapi.go Refactored to use new inspector API, renamed functions for clarity, removed nil checks, improved SelectorExpr handling, added types.Alias support, and removed unused expression cases
_tools/customlint/shadow.go Refactored to use new inspector API, removed defensive type assertions and nil checks, removed unused functions, added map capacity hints
_tools/customlint/emptycase.go Refactored to use pass struct pattern and new inspector API, changed default case to panic
_tools/customlint/plugin.go Changed GetLoadMode from LoadModeSyntax to LoadModeTypesInfo
_tools/customlint/testdata/unexportedapi/* Added extensive test cases for edge cases and code coverage, including new otherpkg test package
_tools/customlint/testdata/shadow/* Added comprehensive shadow detection test cases
Comments suppressed due to low confidence (2)

_tools/customlint/unexportedapi.go:241

  • The removal of the nil check for obj is unsafe. At line 231, if both u.pass.TypesInfo.Defs[expr] and u.pass.TypesInfo.Uses[expr] are nil, then obj will be nil. Accessing obj.Parent() on the next line will cause a nil pointer dereference panic. The original nil check should be restored.
	case *ast.Ident:
		// First check Defs (for defining occurrences), then Uses (for referring occurrences)
		obj := u.pass.TypesInfo.Defs[expr]
		if obj == nil {
			obj = u.pass.TypesInfo.Uses[expr]
		}
		if !expr.IsExported() {
			if obj.Parent() == types.Universe {
				return false
			}
			// Only report if the unexported identifier is from the same package
			if obj.Pkg() != nil && obj.Pkg() == u.pass.Pkg {
				u.pass.Reportf(expr.Pos(), "exported API references unexported identifier %s", expr.Name)
				return true
			}
		}
		return u.checkType(obj.Type())

_tools/customlint/unexportedapi.go:191

  • The removal of the nil check for typ is unsafe. At line 162, u.pass.TypesInfo.TypeOf(field.Type) can return nil if type information is unavailable. If typ is nil, the call to typ.Underlying() at line 173 will cause a nil pointer dereference panic. The original nil check in checkEmbeddedField should be restored.
func (u *unexportedAPIPass) checkEmbeddedField(field *ast.Field) (stop bool) {
	// Get the type of the embedded field
	typ := u.pass.TypesInfo.TypeOf(field.Type)

	// For embedded fields, walk through all exported members and check them.
	// Use the checked map to avoid re-checking members we've already seen.

	// Dereference pointers
	if ptr, ok := typ.(*types.Pointer); ok {
		typ = ptr.Elem()
	}

	// Check exported fields in structs
	if structType, ok := typ.Underlying().(*types.Struct); ok {
		for field := range structType.Fields() {
			if field.Exported() && u.checkObjectType(field) {
				return true
			}
		}
	}

	// Check exported methods on the type
	if named, ok := typ.(*types.Named); ok {
		for method := range named.Methods() {
			if method.Exported() && u.checkObjectType(method) {
				return true
			}
		}
	}

	return false
}

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.

2 participants