Refine the handling of pointers in TypeAsDeepType

[sonalmahajan15]

This PR refines the handling of *types.Pointer in TypeAsDeepType to only consider a pointer type deep if its underlying elements are of a deep type. The implication of always considering a pointer deep was that for type like var x *int; foo(x), unnecessary deep triggers were being created. The change in this PR reduces the creation of these redundant triggers.

[sonalmahajan15]

Earlier, total 5 triggers were created across these two functions, now only the required 3 are being created.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.60%. Comparing base (4f33d8c) to head (0204bd5).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #251      +/-   ##
==========================================
- Coverage   87.63%   87.60%   -0.03%     
==========================================
  Files          63       63              
  Lines        7926     7916      -10     
==========================================
- Hits         6946     6935      -11     
+ Misses        800      799       -1     
- Partials      180      182       +2     

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

[sonalmahajan15]

This change is not particularly related to the PR, but it is a small enough code cleanup, hence included in the same PR. The change is as following: since we have made our consumers as pointers, we can simply go ahead and modify its GuardMatched field, instead of having to override the object.

[github-actions]

Golden Test

Note

✅ NilAway errors reported on standard libraries are identical.

3271 errors on base branch (main, 4f33d8c)
3271 errors on test branch (b2560db)

[yuxincs]

Nice find!! I just have a quick question :)

[yuxincs]

Would this actually work for cases where it's a pointer to struct?

Do we actually want to call TypeAsDeepType recursively here? return TypeAsDeepType(t.Elem())?

[sonalmahajan15]

Ah yes, good point. Now added support for pointer to struct. Added more test cases too, to cover this and a named deep type case.

I actually did do it the recursive way first, but quickly realized that if there exists a circular named type dependency then this goes into an infinite loop. But moreover, we were missing deep type cases like pointer of pointer (**x), causing a change in number of errors as well. The hypothesis of this PR was to only remove the redundant triggers without affecting the number of errors, hence backtracked from the recursive approach.

[yuxincs]

exists a circular named type dependency then this goes into an infinite loop

Hmmm, is it something like this?

type A *B
type B *A

It's definitely weird, and the compiler doesn't complain (it complains if you do type A B; type B A though...).

(it would probably be good to add the pattern to the test suite to avoid accidental regressions)

But moreover, we were missing deep type cases like pointer of pointer (**x), causing a change in number of errors as well.

Are you suggesting pointer of a named type, where the named type is also a pointer (e.g., type A *B; type B *int)? I think by default double pointers aren't allowed. I think other helper functions probably suffer from the same double-ptr problem, and we definitely don't need to handle them in this PR :) Also, are we missing cases like named types, i.e., type A []int where A should be considered deep as well, right?

Another proposal for simpler handling:

Instead of recursions, should we unwrap the ptr first, and then check it's type (like other utility functions in this package), e.g.,

switch UnwrapPtr(t).(type) {
case ....
// no case for `*types.Pointer`
}

This definitely would miss the cases discussed above:
(1) double-ptrs: we're only unwrapping the ptr once, but it avoids infinite loops (and handling double ptrs should be more complex and in separate PRs).
(2) named types: well, if we do UnwrapPtr(t.Underlying()), we should be handling it fine? we can put those in separate PRs as well (with more test cases perhaps)

[sonalmahajan15]

We had discussed this point extensively internally. Updating the logic accordingly, incorporating your suggestion and internal discussion.