-
Notifications
You must be signed in to change notification settings - Fork 53
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
Revamp producers and consumers to be used as pointers #80
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #80 +/- ##
==========================================
+ Coverage 88.92% 89.06% +0.14%
==========================================
Files 54 55 +1
Lines 8260 9156 +896
==========================================
+ Hits 7345 8155 +810
- Misses 758 836 +78
- Partials 157 165 +8 ☔ View full report in Codecov by Sentry. |
79ec162
to
4cf1263
Compare
bc95a95
to
9b0b5d3
Compare
9b0b5d3
to
daae825
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this LGTM, but I have a few nits and one change request for a type assertion typo along with requests for unit tests to catch such typos 😃
annotation/consume_trigger.go
Outdated
|
||
// equals returns true if the passed ConsumingAnnotationTrigger is equal to this one | ||
func (i *MapAccess) equals(other ConsumingAnnotationTrigger) bool { | ||
if other, ok := other.(*PtrLoad); ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if other, ok := other.(*PtrLoad); ok { | |
if other, ok := other.(*MapAccess); ok { |
Right?
I would suggest we add some unit tests to guard against these (we can definitely use reflections in test code since performance usually isn't a concern)
That is, we can use reflect
to find all structs that satisfies the ConsumingAnnotationTrigger
interface and then (1) create two structs for each type and call equals
on them (which should return true), and (2) create two structs of different types and equals
should return false. (3) for even better coverage, we can use reflection to assign different values to the fields and call equals
, but I think having (1) and (2) is good enough and is relatively easy to implement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! I agree that we should add unit tests for catching such inadvertent mistakes. Added unit tests for checking implementations of equals
method of ConsumingAnnotationTrigger
, ProducingAnnotationTrigger
, and Key
interface. The unit tests check for (1) and (2), since (3) would prove to be quite tedious to write the test cases.
Also, as discussed offline, reflection could not be used for this purpose. Hence, for ensuring that we have unit tests for all the structs implementing the given interface, I resorted to the following:
- expected: extract all structs implementing the interface using
types
package - actual: extract all structs initialized in the test case
- compare
expected
andactual
to find and report missing structs, if any
@@ -50,13 +50,16 @@ type ConsumingAnnotationTrigger interface { | |||
UnderlyingSite() Key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(how do you add comments on collapsed content?)
This is actually meant for L34-L36: since we are kinda doing refactors already, could we remove the expired comments there? not a must, obviously 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
annotation/consume_trigger.go
Outdated
func (*ConsumeTriggerTautology) CheckConsume(Map) bool { | ||
return true | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor nit: put this in a single line for consistency with the methods above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
annotation/consume_trigger.go
Outdated
TriggerIfNonNil | ||
AffiliationPair | ||
*TriggerIfNonNil | ||
*AffiliationPair |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need to make AffiliationPair
a pointer, i.e., do we really have the need to modify AffiliationPair
?
I'm OK with making it a pointer, but I have a feeling that it is actually immutable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. It is actually OK if we don't make it a pointer. I just followed the same convention for consistency, but you make a good point about immutability. Removed pointer for AffiliationPair
@@ -961,15 +1272,15 @@ type UseAsNonErrorRetDependentOnErrorRetNilabilityPrestring struct { | |||
func (u UseAsNonErrorRetDependentOnErrorRetNilabilityPrestring) String() string { | |||
via := "" | |||
if u.IsNamedReturn { | |||
via = fmt.Sprintf(" via the named return value `%s`", u.RetName) | |||
via = fmt.Sprintf(" via named return `%s`", u.RetName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small refactor included?
Totally onboarded with the change, but just want to make sure this isn't actually meant for another PR ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I just came across it while testing this PR, and hence included the error message refinement in this PR itself :)
annotation/consume_trigger.go
Outdated
@@ -1074,8 +1395,8 @@ type ConsumeTrigger struct { | |||
} | |||
|
|||
// Eq compares two ConsumeTrigger pointers for equality |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Eq compares two ConsumeTrigger pointers for equality | |
// equals compares two ConsumeTrigger pointers for equality |
Side note: as we discussed, we should probably re-architect the trigger structs, but of course let's do that in future PRs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
+1 about re-architecting the trigger structs.
} | ||
|
||
// TriggerIfNilable is a general trigger indicating that the bad case occurs when a certain Annotation | ||
// key is nilable | ||
type TriggerIfNilable struct { | ||
Ann Key | ||
Ann Key | ||
NeedsGuard bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we have exposed method to access it, should we un-export this field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In principle, we should. However, it would require refactors where this field is being directly set (e.g., annotation/util.go#L189). I feel such refactors are out of scope for this PR. I'll create an issue to track this and refactor in follow-up PRs.
annotation/produce_trigger.go
Outdated
// SetNeedsGuard for a `TriggerIfNilable` is, by default, a noop, as guarding | ||
// applies mostly to deep reads, but this behavior is overriden | ||
// for `VariadicFuncParamDeep`s, which have the semantics of | ||
// deep reads despite consulting shallow annotations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should update the comments here, right? ;)
(and a few places below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes! :) Done
|
||
// ProduceTriggerTautology is used for trigger producers that will always result in nil | ||
type ProduceTriggerTautology struct{} | ||
type ProduceTriggerTautology struct { | ||
NeedsGuard bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here as well, do we need to export this field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(same as discussed above)
annotation/produce_trigger.go
Outdated
TriggerIfNilable | ||
AffiliationPair | ||
*TriggerIfNilable | ||
*AffiliationPair |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here: do we want to make this a ptr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(as discussed above, removed the pointer)
337055f
to
019ced5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this LGTM, thanks!
I just have a few suggestions and a question, but no real blockers.
annotation/equals_test.go
Outdated
func getImplementedMethods(t *types.Named) []*types.Func { | ||
visitedMethods := make(map[string]*types.Func) // helps in only storing the latest overridden implementation of a method | ||
visitedStructs := make(map[*types.Struct]bool) // helps in avoiding infinite recursion if there is a cycle in the struct embedding | ||
collectMethods(t, visitedMethods, visitedStructs) | ||
return maps.Values(visitedMethods) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry it's been a while 😅 can I get a quick reminder on why the corresponding functions in types
doesn't work here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, likely because we are parsing ASTs for files in a given package, it was not able to extract complete type information, particularly, methods of embedded fields.
@@ -15,6 +15,7 @@ require ( | |||
github.com/davecgh/go-spew v1.1.1 // indirect | |||
github.com/kr/text v0.2.0 // indirect | |||
github.com/pmezard/go-difflib v1.0.0 // indirect | |||
github.com/stretchr/objx v0.5.0 // indirect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: is this used anywhere? or does testify/mock
requires it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use it, but testify/mock
requires it
} | ||
|
||
// EqualsTestSuite defines the test suite for the `equals` method. | ||
type EqualsTestSuite struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit tempted to move this to a new package internal/nilawaytest
(internal
prevents other people from depending on it, and nilawaytest
follows the convention in Go that test utility packages are named pkgtest
).
This might make this more general to be shared for checking "a particular method for all structs defined in a package"
Obviously this can be a separate PR :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed :) Created issue #172 to perform this refactoring in a follow-up PR.
// EqualsTestSuite defines the test suite for the `equals` method. | ||
type EqualsTestSuite struct { | ||
suite.Suite | ||
initStructs []any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this a generic struct instead of using any
(with a constraint on having equals
)?
type interface equaler { equals() }
type EqualsTestSuite[T equaler] struct {
...
initStructs[T]
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created issue #172 to perform this refactoring in a follow-up PR.
switch t := initStruct.(type) { | ||
case ConsumingAnnotationTrigger: | ||
s.Truef(t.equals(t), msg, t) | ||
case ProducingAnnotationTrigger: | ||
s.Truef(t.equals(t), msg, t) | ||
case Key: | ||
s.Truef(t.equals(t), msg, t) | ||
default: | ||
s.Failf("unknown type", "unknown type `%T`", t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we define a generic struct, then here we probably won't need a type switch anymore (we can just call .equals
due to our type constraint).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created issue #172 to perform this refactoring in a follow-up PR.
case ConsumingAnnotationTrigger: | ||
if t2, ok := s2.(ConsumingAnnotationTrigger); ok { | ||
s.Falsef(t1.equals(t2), msg, t1, t2) | ||
} | ||
case ProducingAnnotationTrigger: | ||
if t2, ok := s2.(ProducingAnnotationTrigger); ok { | ||
s.Falsef(t1.equals(t2), msg, t1, t2) | ||
} | ||
case Key: | ||
if t2, ok := s2.(Key); ok { | ||
s.Falsef(t1.equals(t2), msg, t1, t2) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created issue #172 to perform this refactoring in a follow-up PR.
annotation/consume_trigger_test.go
Outdated
type ConsumingAnnotationTriggerTestSuite struct { | ||
EqualsTestSuite | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can just have a NewEqualsTestSuite
that takes the interfaceName
and initStructs
parameters, instead of having to inherit EqualsTestSuite
?
Inheritance is second-class in Go, so I would try to avoid it when we can (plus having a constructor makes the code here simpler) :)
No strong preferences, of course.
(If we do change it here, we should also update the tests for produce trigger and key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created issue #172 to perform this refactoring in a follow-up PR.
9534584
to
c7b4d03
Compare
c7b4d03
to
35156bd
Compare
35156bd
to
6814686
Compare
This PR modifies the consumers to add support for deep copy when the consumers are being copied in backpropagation. [depends on #80 ]
Currently, NilAway initializes and passes producers and consumers as struct objects. This PR revamps that design to make producers and consumers as pointers. For this, the PR makes the following changes:
produce_trigger.go
andconsume_trigger.go
, respectively, to pointersProduceTrigger
andConsumeTrigger
equals
method to compare producer/consumerequals
[includes changes from the PR stack: #86 , #87 , #153 ]
[closes #79 ]