A related wrinkle is that recordActivity(), which is called not only to record an activity but also to association an entity with it, currently also assumes that the relevant target is stored in the tracked method's return value. I think sorting this out properly probably requires a redesign, or more time for me to understand what the intended semantics actually are, and how they differ from the current implementation, as the Right Thing to do isn't clear to me:
Add target and targetIndex fields to ActivityMappings, just like for EntityMappings? But this appears to (a) duplicate most of what's in EntityMapping (so why have separate ActivityMappings at all?) and also (b) it's tricky to inject bytecode to handle EntityRef.ARGat the callsite (since TTBOMK there's no random access into the stack, and calling a special utility method beforehand to make arguments them accessible as local variables erases them from the stack).
Get the right location of the target by looking at corresponding EntityMapping entries, which is possible now that EntityCreations also keep track of target and targetIndex? This seems nice, and avoids duplication. But it again calls into question why ActivityMappings exist at all, since everything can now be done with just EntityMappings.
Something else?
However, provided we only want to deal with activities that either return their associated entity (as is already handled) or create it using a constructor (i.e., if we don't care about EntityRef.ARG), this is moot: the JVM happens to handles both cases in such a way that the stack looks the same after the method or constructor returns, i.e., the desired target object is on the top of the stack either way. IOW, we get lucky here and don't need to fix this properly just yet 🤪
A related wrinkle is that
recordActivity(), which is called not only to record an activity but also to association an entity with it, currently also assumes that the relevant target is stored in the tracked method's return value. I think sorting this out properly probably requires a redesign, or more time for me to understand what the intended semantics actually are, and how they differ from the current implementation, as the Right Thing to do isn't clear to me:targetandtargetIndexfields toActivityMappings, just like forEntityMappings? But this appears to (a) duplicate most of what's inEntityMapping(so why have separateActivityMappings at all?) and also (b) it's tricky to inject bytecode to handleEntityRef.ARGat the callsite (since TTBOMK there's no random access into the stack, and calling a special utility method beforehand to make arguments them accessible as local variables erases them from the stack).EntityMappingentries, which is possible now thatEntityCreations also keep track oftargetandtargetIndex? This seems nice, and avoids duplication. But it again calls into question whyActivityMappings exist at all, since everything can now be done with justEntityMappings.However, provided we only want to deal with activities that either return their associated entity (as is already handled) or create it using a constructor (i.e., if we don't care about
EntityRef.ARG), this is moot: the JVM happens to handles both cases in such a way that the stack looks the same after the method or constructor returns, i.e., the desired target object is on the top of the stack either way. IOW, we get lucky here and don't need to fix this properly just yet 🤪Originally posted by @wtwhite in https://github.com/veracitylab/provenance-injector/issues/28#issuecomment-2136555466