typeshelper: follow type aliases, generics, to resolve underlying types by TvdW · Pull Request #422 · uber-go/nilaway

TvdW · 2026-06-15T15:25:22Z

While doing some prework for my next commit I realized that these don't resolve correctly, hiding some types from being checked for nilness.

Claude-assisted, human-reviewed.

yuxincs · 2026-07-02T01:49:18Z

 		// If we are ranging over a map slice or string with only a single lhs operand, then that
 		// operand will be int-valued.
-		if typeshelper.IsDeeplyMap(rhsType) || typeshelper.IsDeeplySlice(rhsType) || typeshelper.IsDeeplyArray(rhsType) || typeIsString(rhsType) {
+		if typeshelper.IsDeeplyMap(rhsType) || typeshelper.IsDeeplySlice(rhsType) || typeshelper.IsDeeplyArrayOrArrayPtr(rhsType) || typeIsString(rhsType) {


Can we use the UnwrapPtr helper function and just call typeshelper.IsDeeplyArray(typeshelper.UnwrapPtr(rhsType)) such that we don't have to introduce a new API in typeshelper?

Or did I miss anything here?

Not quite: UnwrapPtr only takes care of the pointer, but no type aliases, named types, parameters, etc

I have a feeling that we can make UnwrapPtr take care of type aliases, named types, parameters and it would just work.

But let's leave that to a separate journey, the blast radius is large and we may hit corner cases.

Agreed here!

That's #431 😁

yuxincs · 2026-07-02T01:49:27Z

 		// regardless of the indices, so we must check it before the `b[_:0:_]` case below (which
 		// would otherwise wrongly treat `a[:0]` as a nilable empty slice).
-		if typeshelper.IsDeeplyArray(r.Pass().TypesInfo.Types[expr.X].Type) {
+		if typeshelper.IsDeeplyArrayOrArrayPtr(r.Pass().TypesInfo.TypeOf(expr.X)) {


yuxincs · 2026-07-02T01:50:18Z

+// IsDeeplyArray returns true if `t` is of array type, including transitively through named
+// types and aliases, as well as type parameters whose type sets contain only array types.
 func IsDeeplyArray(t types.Type) bool {
-	switch tt := UnwrapPtr(t).(type) {
-	case *types.Array:
-		return true
-	case *types.Named:
-		return IsDeeplyArray(tt.Underlying())
-	}
-	return false
+	return underlyingIs[*types.Array](t)
 }


Now that we have underlyingIs, can we remove these APIs from the good old times and just use the generic version instead?

Of course, we should do that in a separate PR to avoid bloating this one :)

Happy to submit a follow-up PR! func IsType[T](t types.Type) bool

Yes exactly! Please do!

yuxincs · 2026-07-02T01:51:46Z

+// interface intersect, this is conservative: the actual type set can only be smaller than the
+// enumerated terms, and type sets we cannot enumerate (such as method-only constraints) yield
+// false.
+func underlyingAlwaysSatisfies(t types.Type, pred func(types.Type) bool) bool {


It seems that we only ever have a single pred, do we foresee that we will add more predicates in the future? if not I'd actually prefer we inline it

hm, not trivially. this function has two callers with different predicates (IsDeeplyArrayOrArrayPtr and underlyingIs), and the predicate is used to check type parameters so it needs to loop. I could create a new function getUnderlyingTypes but it would need to return a freshly allocated slice which seems worse?

Huh, OK then let's keep this as-is for now :)

yuxincs · 2026-07-02T01:53:33Z

+func constraintTerms(iface *types.Interface) []types.Type {
+	var terms []types.Type
+	for i := 0; i < iface.NumEmbeddeds(); i++ {
+		switch e := types.Unalias(iface.EmbeddedType(i)).(type) {
+		case *types.Union:
+			for j := 0; j < e.Len(); j++ {
+				if emb, isIface := e.Term(j).Type().Underlying().(*types.Interface); isIface {
+					terms = append(terms, constraintTerms(emb)...)
+				} else {
+					terms = append(terms, e.Term(j).Type())
+				}
+			}
+		default:
+			if emb, isIface := e.Underlying().(*types.Interface); isIface {
+				terms = append(terms, constraintTerms(emb)...)
+			} else {
+				terms = append(terms, e)
+			}
+		}
 	}
-	return false
+	return terms
+}


Do we know how much of a performance penalty this would bring?

We'll be calling this function over and over again (and perhaps for the same type), if this makes it slow we should probably at least add a cache.

Or, we can also move this part out to a separate PR and I'm happy to merge the rest first while we discuss on this.

fwiw I can remove this function entirely if you're ok with me adding a new dependency on https://pkg.go.dev/golang.org/x/exp/typeparams#NormalTerms (which does a slightly better job as well, I just wasn't sure if you'd be ok with a new dependency.)

perf-wise, this code only runs on structs with type parameters. it doesn't seem to hurt the golden test- but I admittedly haven't measured the performance impact on code that heavily uses generics (like Rust-style code)

I ran a benchmark using the uncached version vs map[types.Type]... vs sync.Map. the uncached underlyingIs outperformed its cached version (respectively 5ns/op, 9ns/op, 7ns/op) for non-parameterized types. With parameters this quickly jumped to 100ns and even upwards of 500ns for more complex parameters. Still: are those common enough to warrant it?

Yes, having dependencies from x/exp is totally OK (I really wish NormalTerms can be included in stdlib, maybe in the future -- then we can drop that dependency)

Still: are those common enough to warrant it?

Yeah I agree that generics in Go aren't that popular (yet?) and this should be OK. Let's swap to NormalTerms and merge.

I can run this on our internal benchmarks too, if needed we can always revert this part

yuxincs · 2026-07-02T01:53:59Z

@TvdW This is an awesome PR! I left some comments, let me know if they don't make sense :)

codecov · 2026-07-02T02:01:55Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.24%. Comparing base (a4b74d4) to head (1f30814).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #422      +/-   ##
==========================================
- Coverage   87.26%   87.24%   -0.03%     
==========================================
  Files          74       74              
  Lines        8451     8434      -17     
==========================================
- Hits         7375     7358      -17     
- Misses        877      878       +1     
+ Partials      199      198       -1

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

github-actions · 2026-07-02T02:07:05Z

Golden Test

Note

✅ NilAway errors reported on standard libraries are identical.

2187 errors on base branch (main, a4b74d4)
2187 errors on test branch (e63dabf)

While doing some prework for my next commit I realized that these don't resolve correctly, hiding some types from being checked for nilness.

…e[T] Replace the six near-identical IsDeeplyArray/Slice/Map/Ptr/Chan/Interface wrappers with a single exported generic IsDeeplyType[T], built on the underlyingAlwaysSatisfies resolution (named types, aliases, and type parameters via normalized type sets).

yuxincs · 2026-07-02T20:03:07Z

@TvdW Replied to all comments, please ping me once this PR is updated and I'm happy to merge :)

TvdW · 2026-07-02T20:06:24Z

@TvdW Replied to all comments, please ping me once this PR is updated and I'm happy to merge :)

@yuxincs Done!

TvdW force-pushed the pointer-to-array-prework branch 2 times, most recently from 3774a14 to 63d8d5e Compare June 18, 2026 12:46

TvdW mentioned this pull request Jul 1, 2026

feat(struct-init-v2): collect boundary field read effects #429

Merged

yuxincs reviewed Jul 2, 2026

View reviewed changes

TvdW added 2 commits July 2, 2026 11:48

typeshelper: follow type aliases, generics, to resolve underlying types

81b90e1

While doing some prework for my next commit I realized that these don't resolve correctly, hiding some types from being checked for nilness.

TvdW mentioned this pull request Jul 2, 2026

typeshelper: be even more thorough in resolving types #431

Draft

TvdW force-pushed the pointer-to-array-prework branch from 63d8d5e to 1f30814 Compare July 2, 2026 20:04

yuxincs approved these changes Jul 2, 2026

View reviewed changes

yuxincs enabled auto-merge (squash) July 2, 2026 20:08

yuxincs merged commit 5d1c427 into uber-go:main Jul 2, 2026
9 checks passed

Uh oh!

Conversation

TvdW commented Jun 15, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TvdW Jul 2, 2026 • 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!

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!

TvdW Jul 2, 2026 • 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!

yuxincs commented Jul 2, 2026

Uh oh!

codecov Bot commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions Bot commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Golden Test

Uh oh!

yuxincs commented Jul 2, 2026

Uh oh!

TvdW commented Jul 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

TvdW Jul 2, 2026 •

edited

Loading

TvdW Jul 2, 2026 •

edited

Loading

codecov Bot commented Jul 2, 2026 •

edited

Loading

github-actions Bot commented Jul 2, 2026 •

edited

Loading