Re: Re: [Findbugs-core] toward ASM 4.0?

I wonder if it's not better to create a special package "compat"
with the abstract classes implementing the already existing interfaces.
This will let users decide if they want the ability to use several 
version of visitors or not.

Anyway, as Eric says, it will take times before having an implementation 
of that
and we should not delay the release of ASM4.
The package compat can be added in a following version, by ex. 4.1.

Rémi

On 08/19/2011 06:36 PM, Eric Bruneton wrote:
> Eugene Kuleshov wrote:
> >    A great write up. The only thing I'd change is to replace
> > interfaces with an abstract classes and not concrete classes.
>
> yes, although no method in FooVisitor will be abstract, we can make 
> the class itself abstract to prevent users from instantiating it 
> directly.
>
> Brian Goetz wrote:
> > Here's two reasons to postpone this change to ASM[JDK 8]:
> > - JDK 8 will have extension methods, which allow you to add methods
> > to interfaces and provide a "default" implementation in case old
> > moldy classes don't implement them.
>
> after some discussion with Rémi, it appears that this will not suffice 
> (even with the addition of modularity) to provide the same properties 
> than the approach I propose (e.g. having a FindBugs library working 
> with several 3rd party detectors at the same time, when these 
> detectors have been written for different ASM versions)
>
> Eliot Moss wrote:
> > An interesting approach. It seems reasonable to me.
> > Perhaps an abstract class with certain key "checking
> > and redirecting" method already implemented (maybe
> > even "final"?) would be even better, as previously
> > suggested.
> >
> > Meanwhile ...
>
> In the approach I propose, the FooVisitor code I presented is provided 
> by ASM, so yes, the "checking and redirecting" code will be already 
> implemented. However we can't make it final, because users must of 
> course be able to override any visitXxx method.
>
> thanks for your other comments. We will examine them later on.
>
> Andrey Loskutov wrote:
> > I do not see much trouble for the FB detector providers, as the only
> > *really* affected FB interface is FBMethodVisitor, and I'm not sure
> > if any client ever used this interface directly (and not via
> > extending the AbstractFBMethodVisitor class).
> >
> > The rest of the FB detectors code could be probably simply recompiled.
> > In the FB code itself we would need to change AbstractMethodVisitor
> > which would then *extend* and not implement MethodVisitor, and
> > refactor some code in and around the
> > AbstractFBMethodVisitor/FBMethodVisitor,  which should be also
> > doable with not so much effort.
>
> besides the change from an interface to an (abstract) class, users 
> will now also have to specify the maximum bytecode version they 
> considered when they wrote their code (cf. the FooVisitor constructors 
> in my previous email, which take a "version" argument). More on this 
> below.
>
> Zeeb, Jeffrey wrote:
> > One question that I have is: when will the final 4.0 be available
> > with option d?  You need time to make the changes, then you need time
> > to thoroughly test them.  If someone is using ASM and wants to ship
> > their tools with support for Java 7, I expect that option d will
> > cause some additional delays.
>
> I would say at most one month.
>
> Bill Pugh wrote:
> > old code analyzers that examine code with invoke dynamic instructions
> > may have problems. But code that is simply compiled for Java 7 but
> > don't use  invoke dynamic should probably be fine. It would be nice
> > if this compatibility checking was done based on the features
> > actually in the code being processed, rather than the features
> > supported by the class file version the code is compiled to.
>
> with the approach and code I proposed there are two cases: if you want 
> to read a recent class with an old ClassReader, you will get an 
> exception right at the beginning, without examining the class content. 
> If you read a recent class with an up to date ClassReader but with an 
> old analyzer or transformer, then you will get errors only if a new 
> feature is actually used, as you propose. It would be possible to make 
> the first case less restrictive, but this would require more dynamic 
> checks in ClassReader.accept(), and we always chose speed vs checks 
> (moved in CheckXxxxAdapters)
>
> > For using ASM within FindBugs, we already provide abstract classes
> > for many of the visitor interfaces. So switching to abstract classes
> > in ASM should be transparent.
>
> same comment as above.
>
> Eric Bruneton wrote:
> > factor). All this should be checked by trying to reimplement the
> > examples in cases 2 and 3 with this method. Another good test is to
> > see what happens if visitBar becomes visitBar(int,int,int) in a new
> > version Z.
>
> the code I proposed in FooVisitor can be generalized with the 
> following patterns, in pseudo code:
>
> adding a new method:
>
> visitBaz(...) {
>     if (version < V_Y)
>         error
>     else
>         if (next != null) next.visitBaz(...);
> }
>
> changing a method signature:
>
> visitBar(old signature) {
>     if (version < V_Y) {
>         // copy code from visitBar(old signature) in version X
>     } else {
>        call visitBar(new signature)
>     }
> }
>
> visitBar(new signature) {
>     if (version < V_Y) {
>         if (lossless conversion to old signature possible) {
>             call visitBar(old signature)
>         } else {
>             error
>         }
>     } else {
>         if (next != null) next.visitBar(new signature);
>     }
> }
>
> Then, applying this pattern to a new signature change for visitBar in 
> version Z > Y, we get (as said above, this "checking and redirecting" 
> code will be provided by ASM):
>
> // unchanged
> void visitBar(int i) {
>     if (version < V_Y) {
>         if (next != null) next.visitBar(i,0);
>     } else {
>         visitBar(i,0);
>     }
> }
>
> void visitBar(int i, int j) {
>     if (version < V_Z) {
>         // code from visitBar(int,int) in version Y
>         if (version < V_Y) {
>             if (j == 0) visitBar(i) else throw new RuntimeException();
>         } else
>             if (next != null) next.visitBar(i,j);
>     } else
>         visitBar(i,j,0);
> }
>
> void visitBar(int i, int j, int k) {
>     if (version < V_Z) {
>         if (k == 0) visitBar(i,j); else throw new RuntimeException();
>     } else
>         if (next != null) next.visitBar(i,j,k);
> }
>
> now, considering three possible subclasses of this abstract class, for 
> versions X, Y and Z (each subclass overriding exactly one of 3 above 
> methods), it is easy to see that, no matter which method is called on 
> a FooVisitor instance, we finally end up in the overriden method (one 
> example: a version Z subclass overrides visitBar(int,int,int); even if 
> we call visitBar(int) or visitBar(int,int), we endup in the overriden 
> visitBar(int,int,int). The same is true for versions X and Y).
>
> Note also that up to date code (ie. a version Z FooVisitor preceded 
> and followed by version Z components in the adapter chain) does not 
> pay any indirection cost, no matter how many times the visitBar 
> signature has been changed.
>
> Adding an int to a method is a simple change. A more complex change is 
> the following:
>
> visitField(Attribute[] a) // old, version X
>
> to
>
> FieldVisitor visitField(String signature) // new, version Y
>
> with:
>
> FieldVisitor {
>     void visitAttribute(Attribute a);
>     void visitEnd();
> }
>
> even in this case, the general pattern proposed above still works:
>
> ASM X:
>
> visitField(Attribute[] a) {
>     if (next != null) visitField(a);
> }
>
> ASM Y:
>
> visitField(Attribute[] a) {
>     if (version < V_Y) {
>         if (next != null) visitField(a);
>     } else {
>         FieldVisitor fv = visitField(null);
>         if (fv != null) {
>             for (int i = 0; i < a.length; ++i) {
>                 fv.visitAttribute(a[i]);
>             }
>             fv.visitEnd();
>         }
>     }
> }
>
> FieldVisitor visitField(String signature) {
>     if (version < V_Y) {
>         if (signature == null) {
>             return new FieldVisitor() {
>                 List attrs = new ArrayList();
>                 void visitAttribute(Attribute a) {
>                     attrs.add(a);
>                 }
>                 void visitEnd() {
>                     visitField((Attribute[]) attrs.toArray());
>                 }
>             };
>         } else
>             throw new RuntimeException();
>     } else {
>         if (next != null) return next.visitField(signature);
>     }
> }
>
> ----------------------
>
> So far, I assumed that when a method signature is changed, i.e. when 
> we have several versions of a method in FooVisitor, users will 
> override *at most one* of these methods, the one corresponding to the 
> version declared in the FooVisitor constructor. It is easy to see 
> that, if this hypothesis is not respected, unexpected behavior can occur.
>
> And it is easy to arrive in such a situation without noticing it. It 
> suffice to have a subclass B of a class A itself extending FooVisitor:
>
> in version X we have:
>
> class A extends FooVisitor {
>     A(FooVisitor next) { super(V_X, next); }
>     void visitBar(int i) { ... behavior A ... }
>     ...
> }
>
> class B extends A {
>     B(FooVisitor next) { super(next); }
>     void visitBar(int i) { ... behavior B ... }
> }
>
> now suppose that A is provided by some library, and B by another (for 
> instance A is provided by ASM itself, e.g, it is a asm.commons 
> component, while B is provided by the user; or A is provided by 
> FindBugs, and B is provided by a 3rd party detector). At some time, A 
> is upgraded to version Y, but not B. Then we get:
>
> class A extends FooVisitor {
>     A(FooVisitor next) { super(V_Y, next); }
>     void visitBar(int i, int j) { ... behavior A ... }
>     ...
> }
>
> and now B overrides both versions of visitBar, with different 
> behaviors in each version! Additionally, its version is wrongly 
> upgraded to Y (while it should stay to X). This second problem can 
> easily be fixed as follows:
>
> in version X:
>
> class A extends FooVisitor {
>     A(FooVisitor next) { super(V_X, next); }
>     protected A(int version, FooVisitor next) {
>         super(Math.min(version, V_X), next);
>     }
>     ...
> }
>
> class B extends A {
>     B(FooVisitor next) { super(V_X, next); }
>     protected B(int version, FooVisitor next) {
>         super(Math.min(version, V_X), next);
>     }
>     ...
> }
>
> Now when A is changed to:
>
> class A extends FooVisitor {
>     A(FooVisitor next) { super(V_Y, next); }
>     protected A(int version, FooVisitor next) {
>         super(Math.min(version, V_Y), next);
>     }
>     ...
> }
>
> in version Y, the version for B instances will remain V_X, as desired. 
> Another way of doing the same thing is to transform the "version" 
> constructor argument in an abstract "getVersion" method:
>
> ASM code:
> abstract class FooVisitor {
>     private int version;
>     public FooVisitor(...) { this.version = getVersion(); ... }
>     public abstract int getVersion();
>     ...
> }
>
> user code (version X):
>
> class A extends FooVisitor {
>     public int getVersion() { return V_X; }
>     ...
> }
>
> class B extends A {
>     public int getVersion() { return Math.min(V_X, super.getVersion()); }
>     ...
> }
>
> this approach is probably better than the first from a user point of 
> view.
>
> It remains to solve the first problem: how can we enforce ASM 
> components to override at most one variant of each method (and not an 
> arbitrary one, but the one corresponding to "version")?
>
> The only solution I see is to check this in the FooVisitor 
> constructor, provided by ASM, by using introspection. For instance, in 
> FooVisitor version Y, we would have (pseudo code):
>
> abstract class FooVisitor {
>     FooVisitor(int version, FooVisitor next) {
>         if (version < V_Y) {
>             // visitBar(int,int) must not be overriden
>             if 
> (!getClass().getMethod("visitBar(int,int)").getDeclaringClass().equals(FooVisitor.class)) 
> throw new RuntimeException();
>         } else {
>             // visitBar(int) must not be overriden
>             if 
> (!getClass().getMethod("visitBar(int)").getDeclaringClass().equals(FooVisitor.class)) 
> throw new RuntimeException();
>         }
>     }
>     ...
> }
>
> the "if" part would correctly detect an error in the above example (B 
> of version X, overriding visitBar(int,int) via inheritance). I don't 
> see yet how the second case could occur in a similar way (i.e. without 
> users noticing it -- of course this case can happen if someone 
> overrides both versions in its code, but then it will most probably 
> just implement visitBar(int) with a call to visitBar(i,0), which is 
> OK). So I would be tempted to remove the "else" part, i.e. the checks 
> when FooVisitor is instantiated with the most recent version. This way 
> the cost of these checks (which increases each time we add a new 
> version of a method) would only be paid by old components.
>
> Eric
>

-- 
You receive this message as a subscriber of the [email protected] mailing list.
To unsubscribe: mailto:[email protected]
For general help: mailto:[email protected]?subject=help
OW2 mailing lists service home page: http://www.ow2.org/wws