Skip to content
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

Support for "always safe" for error returning and ok-form functions #264

Merged
merged 8 commits into from
Jul 26, 2024

Conversation

sonalmahajan15
Copy link
Contributor

@sonalmahajan15 sonalmahajan15 commented Jul 3, 2024

This PR adds support for "always safe" case for rich check effect functions, namely error returning functions and ok-form functions.

The support for always safe tracking is currently limited to:

  1. directly determinable cases of "non-nil" return
      (a) return &S{}, errors.New(...)  // supported
      (b) return new(S), errors.New(...)  // supported
      (c) if s != nil {
               return s, errors.New(...)  // supported
          }
  
      (d) return getS(), errors.New(...)  // not supported
      (e) return x.y.z.f, errors.New(...)   // not supported
  1. immediate function call only
func errRetFunc() (*S, error) {
      return &S{}, errors.New(...)
}

func wrapErrRetFunc() (*S, error) {
      return errRetFunc()
}

func test1() {
       // supported
       v, _ := errRetFunc()    // immediate function call, supported
       _ = *v

       // not supported
       v, _ := wrapErrRetFunc()  // multiple hops, not supported
       _ = *v
}

[closes #151 , #199 ]

Copy link

codecov bot commented Jul 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.64%. Comparing base (28b542b) to head (5ec951b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #264      +/-   ##
==========================================
+ Coverage   87.51%   87.64%   +0.13%     
==========================================
  Files          63       63              
  Lines        7842     7927      +85     
==========================================
+ Hits         6863     6948      +85     
  Misses        799      799              
  Partials      180      180              

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

Copy link

github-actions bot commented Jul 3, 2024

Golden Test

Warning

❌ NilAway errors reported on stdlib are different 📉.

3283 errors on base branch (main, 28b542b)
3275 errors on test branch (d830871)

Diffs
- /opt/hostedtoolcache/go/1.22.5/x64/src/encoding/base32/base32_test.go:114:41: Potential nil panic detected. Observed nil flow from source to dereference point: 
- 	- base32/base32_test.go:114:41: result 0 of `AppendDecode()` lacking guarding; sliced into via the assignment(s):
- 		- `StdEncoding.AppendDecode(...)` to `dst` at base32/base32_test.go:110:3
- /opt/hostedtoolcache/go/1.22.5/x64/src/encoding/base64/base64_test.go:173:37: Potential nil panic detected. Observed nil flow from source to dereference point: 
- 	- base64/base64_test.go:173:37: result 0 of `AppendDecode()` lacking guarding; sliced into via the assignment(s):
- 		- `tt.enc.AppendDecode(...)` to `dst` at base64/base64_test.go:169:4
- /opt/hostedtoolcache/go/1.22.5/x64/src/go/parser/parser.go:2195:29: Potential nil panic detected. Observed nil flow from source to dereference point: 
- 	- parser/parser.go:2249:36: result 0 of `parseSimpleStmt()` lacking guarding; passed as arg `s` to `isTypeSwitchGuard()` via the assignment(s):
- 		- `p.parseSimpleStmt(...)` to `s2` at parser/parser.go:2243:5
- 	- parser/parser.go:2195:29: function parameter `s` accessed field `X`
- /opt/hostedtoolcache/go/1.22.5/x64/src/net/http/cookiejar/jar.go:204:11: Potential nil panic detected. Observed nil flow from source to dereference point: 
- 	- cookiejar/jar_test.go:1347:13: result 0 of `New()` lacking guarding; used as receiver to call `Cookies()` via the assignment(s):
- 		- `New(nil)` to `jar` at cookiejar/jar_test.go:1345:3
- 	- cookiejar/jar.go:158:9: read by method receiver `j` used as receiver to call `cookies()`
- 	- cookiejar/jar.go:204:11: read by method receiver `j` accessed field `entries`
- /opt/hostedtoolcache/go/1.22.5/x64/src/net/http/cookiejar/jar.go:204:11: Potential nil panic detected. Observed nil flow from source to dereference point: 
- 	- cookiejar/jar_test.go:1351:13: result 0 of `New()` lacking guarding; used as receiver to call `Cookies()` via the assignment(s):
- 		- `New(nil)` to `jar` at cookiejar/jar_test.go:1345:3
- 	- cookiejar/jar.go:158:9: read by method receiver `j` used as receiver to call `cookies()`
- 	- cookiejar/jar.go:204:11: read by method receiver `j` accessed field `entries`
- /opt/hostedtoolcache/go/1.22.5/x64/src/net/http/cookiejar/jar.go:291:11: Potential nil panic detected. Observed nil flow from source to dereference point: 
- 	- cookiejar/jar_test.go:1350:3: result 0 of `New()` lacking guarding; used as receiver to call `SetCookies()` via the assignment(s):
- 		- `New(nil)` to `jar` at cookiejar/jar_test.go:1345:3
- 	- cookiejar/jar.go:233:2: read by method receiver `j` used as receiver to call `setCookies()`
- 	- cookiejar/jar.go:291:11: read by method receiver `j` accessed field `entries`
- /opt/hostedtoolcache/go/1.22.5/x64/src/net/url/example_test.go:157:2: Potential nil panic detected. Observed nil flow from source to dereference point: 
- 	- url/url.go:1123:9: result 0 of `ParseQuery()` lacking guarding; returned from `Query()` in position 0 via the assignment(s):
- 		- `ParseQuery(...)` to `v` at url/url.go:1122:2
- 	- url/example_test.go:157:2: result 0 of `Query()` called `Set()` via the assignment(s):
- 		- `url.Parse(...)` to `u` at url/example_test.go:150:2,
- 		- `u.Query()` to `q` at url/example_test.go:156:2
- 
- (Same nil source could also cause potential nil panic(s) at 3 other place(s): "url/example_test.go:158:15", "url/example_test.go:308:14", and "url/example_test.go:309:14".)
- /opt/hostedtoolcache/go/1.22.5/x64/src/net/url/url_test.go:1307:13: Potential nil panic detected. Observed nil flow from source to dereference point: 
- 	- url/url.go:1123:9: result 0 of `ParseQuery()` lacking guarding; returned from `Query()` in position 0 via the assignment(s):
- 		- `ParseQuery(...)` to `v` at url/url.go:1122:2
- 	- url/url_test.go:1307:13: result 0 of `Query()` called `Get()` via the assignment(s):
- 		- `Parse(...)` to `u` at url/url_test.go:1302:2,
- 		- `u.Query()` to `v` at url/url_test.go:1303:2
- 
- (Same nil source could also cause potential nil panic(s) at 9 other place(s): "url/url_test.go:1311:13", "url/url_test.go:1314:13", "url/url_test.go:1317:13", "url/url_test.go:1320:13", "url/url_test.go:1323:13", "url/url_test.go:1326:13", "url/url_test.go:1329:13", "url/url_test.go:1332:2", and "url/url_test.go:1333:13".)

@sonalmahajan15 sonalmahajan15 changed the title Support for "always safe" Support for "always safe" for error returning and ok-form functions Jul 9, 2024
Comment on lines 219 to 227
// Filter out the triggers that are to be deleted.
var filteredPkgFullTriggers []annotation.FullTrigger
for i, t := range pkgFullTriggers {
if triggersToBeDeleted[i] {
continue
}
filteredPkgFullTriggers = append(filteredPkgFullTriggers, t)
}
pkgFullTriggers = filteredPkgFullTriggers
Copy link
Contributor

Choose a reason for hiding this comment

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

Use slices.DeleteFunc to modify the slice in-place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Done :)

Consumer: consumer,
})
}

return // expr is not trackable, but cannot be nil, so do nothing
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this comment?

Copy link
Contributor Author

@sonalmahajan15 sonalmahajan15 Jul 26, 2024

Choose a reason for hiding this comment

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

Yeah, makes sense, it has become redundant now. Done.

@sonalmahajan15 sonalmahajan15 force-pushed the sonalmahajan15/support-always-safe branch from d51f9d4 to 5ec951b Compare July 26, 2024 22:47
@sonalmahajan15 sonalmahajan15 merged commit 9012b93 into main Jul 26, 2024
8 checks passed
@sonalmahajan15 sonalmahajan15 deleted the sonalmahajan15/support-always-safe branch July 26, 2024 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants