Add more test cases for calling no-return functions #252
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #252 +/- ##
=======================================
Coverage 87.55% 87.55%
=======================================
Files 63 63
Lines 7828 7828
=======================================
Hits 6854 6854
Misses 796 796
Partials 178 178 ☔ View full report in Codecov by Sentry. |
| } | ||
| print(*ptr) | ||
| } | ||
| } |
There was a problem hiding this comment.
Appreciate the detailed tests! 👍
assertion/function/preprocess/cfg.go
Outdated
| continue | ||
| } | ||
| if trustedfunc.TrimSuccsOn(p.pass, call) { | ||
| block.Succs = nil |
There was a problem hiding this comment.
I was just wondering if we should set successors to nil or point to the return block... What is the behavior of the drivers where this behavior is handled correctly? Perhaps we can simulate that behavior here.
There was a problem hiding this comment.
I compared the correct behaviors and it seems that block.Succs == nil is the behavior, so we're following that practice.
This is also documented: https://pkg.go.dev/golang.org/x/tools/go/cfg
A block may have 0-2 successors: zero for a return block or a block that calls a function such as panic that never returns; one for a normal (jump) block; and two for a conditional (if) block.
Anyway, I think this comment is no longer applicable since we decided to just fix the upstream ctrlflow analyzer instead of doing hacky postfixes in NilAway.
There was a problem hiding this comment.
Got it. Makes sense. Thanks for clarifying :)
|
Actually, instead of doing hacky postfixes in NilAway, we should just fix Will repurpose this PR to just introduce those valuable test cases instead :) |
yuxincs
changed the title
There was a problem hiding this comment.
Actually, instead of doing hacky postfixes in NilAway, we should just fix ctrlflow analyzer: golang/tools#497
Will repurpose this PR to just introduce those valuable test cases instead :)
Oh yeah, makes complete sense! :)
This PR adds more test cases to test NilAway's ability to handle no-return functions (such as
os.Exit,runtime.Goexit,testing.T.SkipNowetc.).