Optimistic default for nil receivers of out-of-scope (e.g., generated code) methods #146
Conversation
sonalmahajan15
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #146 +/- ##
==========================================
- Coverage 88.97% 88.82% -0.15%
==========================================
Files 54 54
Lines 8254 8260 +6
==========================================
- Hits 7344 7337 -7
- Misses 753 766 +13
Partials 157 157 ☔ View full report in Codecov by Sentry. |
| var a *A | ||
| err := a.retErr() | ||
| print(err.Error()) //want "returned as error result 0 of `retErr.*`" | ||
| print(err.Error()) // false negative, since `Error()` is nil-unsafe |
There was a problem hiding this comment.
So, I'm a bit concerned about this given the nil-safe methods are kinda rare even in stdlib, and this could possibly lead to many false negatives.
Can we have a functionality regression check internally to see if the errors being removed by this PR are all valid false positives?
I feel it might be better to be a little conservative here to model thrift-generated code first (it would have a package path prefixed with thriftw/), and possibly some stdlib methods as we go (e.g., the os.File.Stat here, which is an interesting example).
Would it be possible to extend our trusted function framework to handle this? If not I'm OK with a hack here to stop the bleed and refactor once the trusted function framework can handle this.
Thinking on the longer-term (we have discussed internally on this as well), we probably want to extend the trusted function framework to be a plugin system that can hook into multiple points in NilAway's logic flow to easily model libraries (even for end-users)
Also, we probably also want to model thrift (and other IDL languages like protobuf) better since they kinda have nilability annotations builtin (e.g., thrift has this optional keyword to mark a field such that the return value is nilable, otherwise it's guaranteed to be nonnil). If we can parse those, we should be a lot more precise for generated code.
Thoughts?
There was a problem hiding this comment.
Thank you for your insightful feedback. I acknowledge your concern regarding false negatives and would like to address specific points related to extending NilAway's design for nil receivers and the potential utilization of the trusted function framework.
To provide context, NilAway is designed with optimistic defaults for external code, assuming non-nil returns and nil-safe parameters. For example, atomic.AddUint64(nil, 1) would lead to a nil panic upon execution due to the nil parameter, however, goes unreported (false negative) by NilAway due to being outside its scope. Coming to the nil receivers change in this PR, I'd like to emphasize that this adjustment aligns with this existing paradigm of NilAway. Receivers can be viewed as specialized parameters, and this PR simply extends the established design bounds of NilAway within this paradigm. I believe treating receivers in a manner consistent with parameters is both logical and in line with NilAway's core principles.
Can we have a functionality regression check internally to see if the errors being removed by this PR are all valid false positives?
I agree and have already conducted spot checks internally, all of which confirmed the instances as false positives. However, I can perform a more thorough and systematic check to ensure the accuracy of the errors removed by this PR.
Would it be possible to extend our trusted function framework to handle this?
While technically possible, it would involve modeling each external function individually, which could be a potentially tedious process.
extend the trusted function framework to be a plugin system
I concur; this aligns with our internal discussions on enhancing NilAway's capabilities in the long-term.
we probably also want to model thrift (and other IDL languages like protobuf) better
I do share your perspective on the importance of improving NilAway's handling of IDL languages like Thrift and Protobuf, leveraging their built-in nilability annotations for more precise code analysis. This would be a great long-term task to improve NilAway.
I hope this clarifies my stance on these matters. Please feel free to further discuss or provide additional insights. Happy to discuss this offline too! :)
There was a problem hiding this comment.
Thanks for the explanations!
Hmmmm, I agree that this actually aligns with the optimistic defaults in NilAway in other places, so let's have that. 😃
The example here was from stdlib and I had a thought of actually analyzing stdlib and caching the results, so I confused myself here.
For context, since Go 1.20 the stdlib is no longer shipped as precompiled packages but built on demand and cached in the build cache like any other packages (meaning NilAway has a chance to "see" the stdlib code), I had a thought that since stdlib code does not change it might be good to actually analyze it (and then cache it).
But that's something for another discussion later, I think we should go for optimistic defaults as a fallback mechanism, and we can have some handlings as special cases (stdlib, thrift-generated code etc.).
Stamping the changes!
There was a problem hiding this comment.
Thanks for the context. I understand and agree with your position :) We can revisit and revamp the handling of external code in the future, but for now resort to optimistic defaults and make the handling of receivers consistent with parameters.
sonalmahajan15
deleted the
sonalmahajan15/nil-receiver-optimistic-default
branch
This PR adds optimistic default for nil receivers of out-of-scope (e.g., generated code) methods. Without this support, NilAway was observed to report false positives for nil-safe methods that were outside of NilAway's analysis scope.