I'm +1 for adding whatever we need to make coverage work. Having said that, we shouldn't need to add it for methods (classes?) already marked synthetic. I also noticed that the kotlin support looks for a lack of line number information. Could we do something similar? We leave line number info as -1 in many cases and could do that check just for classes implementing GroovyObject. Or was this approach ruled out earlier? The line number approach is more of a hack I guess but we could leave that as the fall back. That could enable things to almost totally work now and we can improve incrementally by adding synthetic or @Generated in more places over time.
Thoughts? Paul. On Thu, Aug 16, 2018 at 6:53 AM Andres Almiray <aalmi...@gmail.com> wrote: > There’s a difference between synthetic and having the @Generated > annotation, they are not equivalent. Synthetic signals tools to ignore the > method altogether (JaCoCo honors this behavior). @Generated should be > applied to non-synthetic methods/classes; what a particular tool decides to > do with that information is up to the tool itself, in the case of JaCoCo > it’ll skip the method/class from coverage. > > The reason I raises this issue is to see if there are any objections in > adding @Generated to most (if not all) compiler generated methods (property > getter/setter, core AST xforms) as this change touches lots of files, > however it has no impact of Groovy behavior, rather it impacts tools that > may parse Groovy bytecode (like JaCoCo). > > Cheers > Andres (with no extra a) > > Sent from my primitive Tricorder > > On 15 Aug 2018, at 22:43, Milles, Eric (TR Technology & Ops) < > eric.mil...@thomsonreuters.com> wrote: > > Andreas, > > > One place to start is everywhere that "AnnotatedNode.setSynthetic(true)" > is currently called. I think this misses the methods added for a > property. But it does cover several AST transforms. And maybe the > transforms that add methods and don't call this method could do so in > addition to the modification to add "@Generated". Maybe the call to > setSynthetic could actually add the annotation. Or you could create a > utility method that sets synthetic and adds "@Generated". > > > ------------------------------ > *From:* Andres Almiray <aalmi...@gmail.com> > *Sent:* Tuesday, August 14, 2018 3:42 PM > *To:* dev@groovy.apache.org > *Subject:* Updates on JaCoCo support > > Hello everyone, > > I've spent a couple of hours with JaCoCo team members at the Hackergarten > Bern this evening. > The goal of the session was to get started with an integration test for > the @Generated feature > added in Groovy 2.5.0. > > You can see the outcome at https://github.com/jacoco/jacoco/pull/733 > <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_jacoco_jacoco_pull_733&d=DwMFaQ&c=4ZIZThykDLcoWk-GVjSLmy8-1Cr1I4FWIvbLFebwKgY&r=tPJuIuL_GkTEazjQW7vvl7mNWVGXn3yJD5LGBHYYHww&m=hAfmXOD5R2AP8VF69wcL-UqYZEv2tXAbyBRfDJh0lfg&s=kxj4hAyP-_6cuc12J6CSMgl_xJRqfEjVcnMHfxzID8w&e=> > > The good news is that Groovy applies @Generated on constructors added by > @Cannonical as well > as methods defined by the GroovyObject interface. The bad news is that the > test still fails > because the expectation is that *every* method generated by the compiler > that does not map > to a particular source line *should* be annotated with @Generated. The > following source > > ---- > // This annotation generates the following > // - a constructor that takes an int as argument > // - a suitable implementation of toString() > // - a suitable implementation of hashCode() > // - a suitable implementation of equals(Object) > // - a public method named canEqual(Object) > // - a getter & setter for the valRead property > @groovy.transform.Canonical > class GroovyDataClassTarget { // assertFullyCovered() > > int valRead // assertNotCovered() > > static void main(String[] args) { > new GroovyDataClassTarget() // assertFullyCovered() > } > } > ---- > > Generates bytecode equivalent to (decompiled with IntelliJ) > > ---- > // > // Source code recreated from a .class file by IntelliJ IDEA > // (powered by Fernflower decompiler) > // > > package org.jacoco.core.test.validation.groovy.targets; > > import groovy.lang.GroovyObject; > import groovy.lang.MetaClass; > import groovy.transform.EqualsAndHashCode; > import groovy.transform.Generated; > import groovy.transform.ToString; > import org.codehaus.groovy.runtime.InvokerHelper; > import org.codehaus.groovy.runtime.ScriptBytecodeAdapter; > import org.codehaus.groovy.runtime.callsite.CallSite; > import org.codehaus.groovy.runtime.typehandling.DefaultTypeTransformation; > import org.codehaus.groovy.runtime.typehandling.ShortTypeHandling; > import org.codehaus.groovy.util.HashCodeHelper; > > @ToString > @EqualsAndHashCode > public class GroovyDataClassTarget implements GroovyObject { > private int valRead; > > @Generated > public GroovyDataClassTarget(int valRead) { > CallSite[] var2 = $getCallSiteArray(); > super(); > MetaClass var3 = this.$getStaticMetaClass(); > this.metaClass = var3; > this.valRead = DefaultTypeTransformation.intUnbox(valRead); > } > > public GroovyDataClassTarget() { > CallSite[] var1 = $getCallSiteArray(); > this(Integer.valueOf(0)); > } > > public static void main(String... args) { > CallSite[] var1 = $getCallSiteArray(); > var1[0].callConstructor(GroovyDataClassTarget.class); > } > > public String toString() { > CallSite[] var1 = $getCallSiteArray(); > Object _result = var1[1].callConstructor(StringBuilder.class); > Object $toStringFirst = Boolean.TRUE; > var1[2].call(_result, > "org.jacoco.core.test.validation.groovy.targets.GroovyDataClassTarget("); > if (DefaultTypeTransformation.booleanUnbox($toStringFirst)) { > Boolean var4 = Boolean.FALSE; > } else { > var1[3].call(_result, ", "); > } > > var1[4].call(_result, var1[5].callStatic(InvokerHelper.class, > var1[6].callCurrent(this))); > var1[7].call(_result, ")"); > return > (String)ShortTypeHandling.castToString(var1[8].call(_result)); > } > > public int hashCode() { > CallSite[] var1 = $getCallSiteArray(); > Object _result = var1[9].callStatic(HashCodeHelper.class); > if > (!DefaultTypeTransformation.booleanUnbox(var1[10].call(var1[11].callCurrent(this), > this))) { > Object var3 = var1[12].callStatic(HashCodeHelper.class, > _result, var1[13].callCurrent(this)); > _result = var3; > } > > return DefaultTypeTransformation.intUnbox(_result); > } > > public boolean canEqual(Object other) { > CallSite[] var2 = $getCallSiteArray(); > return other instanceof GroovyDataClassTarget; > } > > public boolean equals(Object other) { > CallSite[] var2 = $getCallSiteArray(); > if (ScriptBytecodeAdapter.compareEqual(other, (Object)null)) { > return false; > } else if > (DefaultTypeTransformation.booleanUnbox(var2[14].callCurrent(this, other))) > { > return true; > } else if (!(other instanceof GroovyDataClassTarget)) { > return false; > } else { > GroovyDataClassTarget otherTyped = > (GroovyDataClassTarget)other; > if > (!DefaultTypeTransformation.booleanUnbox(var2[15].call(otherTyped, this))) { > return false; > } else { > return > ScriptBytecodeAdapter.compareEqual(var2[16].callCurrent(this), > var2[17].call(otherTyped)); > } > } > } > > public int getValRead() { > return this.valRead; > } > > public void setValRead(int var1) { > this.valRead = var1; > } > } > ---- > > We can appreciate that the methods added by @ToString, @EqualsAndHashcode, > and the property getter/setter are not > annotated with @Generated, which will prompt JaCoCo to mark them as not > covered. The rationale from the JaCoCo team > is that these methods should be annotated as the compiler is "trusted", > only those methods explicitly added to the > source should be covered. > > Thus, here comes the call to action and the reason why I wanted to start > this conversation in the first place: > - modify the Groovy compiler to add @Generated on property getters and > setters. > - modify core AST xforms to add @Generated where it makes sense. > > Related to the original @Generated issue (as commented by Evgeny at > https://github.com/jacoco/jacoco/pull/610 > <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_jacoco_jacoco_pull_610&d=DwMFaQ&c=4ZIZThykDLcoWk-GVjSLmy8-1Cr1I4FWIvbLFebwKgY&r=tPJuIuL_GkTEazjQW7vvl7mNWVGXn3yJD5LGBHYYHww&m=hAfmXOD5R2AP8VF69wcL-UqYZEv2tXAbyBRfDJh0lfg&s=44Nu_515VJUtJ8wIVcQJF2ow_axTotUprpo9KwRmQMY&e=>) > fields > do not have line numbers, would be good to have them. > > What do you think? > > Cheers, > Andres > > ------------------------------------------- > Java Champion; Groovy Enthusiast > JCP EC Associate Seat > http://andresalmiray.com > <https://urldefense.proofpoint.com/v2/url?u=http-3A__andresalmiray.com&d=DwMFaQ&c=4ZIZThykDLcoWk-GVjSLmy8-1Cr1I4FWIvbLFebwKgY&r=tPJuIuL_GkTEazjQW7vvl7mNWVGXn3yJD5LGBHYYHww&m=hAfmXOD5R2AP8VF69wcL-UqYZEv2tXAbyBRfDJh0lfg&s=ynGizu7RDzXxq8Mp65SEUYPVyKHTo7kc14uNurHMLRM&e=> > http://www.linkedin.com/in/aalmiray > <https://urldefense.proofpoint.com/v2/url?u=http-3A__www.linkedin.com_in_aalmiray&d=DwMFaQ&c=4ZIZThykDLcoWk-GVjSLmy8-1Cr1I4FWIvbLFebwKgY&r=tPJuIuL_GkTEazjQW7vvl7mNWVGXn3yJD5LGBHYYHww&m=hAfmXOD5R2AP8VF69wcL-UqYZEv2tXAbyBRfDJh0lfg&s=jMgNE8ncoIZmIOY8tu6FwSSs_8KA4Vh6-7GpRqDNEd4&e=> > -- > What goes up, must come down. Ask any system administrator. > There are 10 types of people in the world: Those who understand binary, > and those who don't. > To understand recursion, we must first understand recursion. > >
