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

Eric,

  A great write up. The only thing I'd change is to replace interfaces
with an abstract classes and not concrete classes. This is pretty much
in line with old Eclipse API document.
http://wiki.eclipse.org/index.php/Evolving_Java-based_APIs

  Also agree with you on no reasons to postpone this change. So,
option d) is the best choice now. The ASM4 already introduced backward
incompatible changes and developers will have to update. May as well
do this now.

  I am not familiar with FindBugs detectors, so it is unclear if
detectors will be impacted by this change, ie if they have to be
recompiled or not.

  regards,
  Eugene


On Wed, Aug 17, 2011 at 8:39 AM,  <[email protected]> wrote:
> I have an idea to solve the binary compatibility issues once and for all in the future, but it requires one last (big) non-backward compatible change (see below). Then we have several options:
> a) discard this idea, and accept to have new incompatible changes in the future, with all the problems we currently have
> b) implement this idea in one or two years, in "ASM for Java 8"
> c) implement this idea in an "ASM 5" version released at the same time as ASM 4
> d) implement this idea in ASM 4
> We also have the choice to rename the package to "ow2" at the same time or not. Also, if we implement this idea, it will require some effort to port existing code, sooner or later, but I don't see any advantage to postpone it. In other words, I prefer option d), or c). I think this non backward compatible change is worth the effort, knowing that it will be the last one.
>
> Sorry, this email is quite long...
>
> Eric
>
> -----------------
>
> New elements have been introduced in the past in the class file format, and new elements will continue to be added in the future (e.g. for modularity, annotations on Java types, etc). These changes impact code generators, analyzers and transformers differently:
> - code generators are not impacted: they generate code with some fixed class version, and these generated classes will remain valid with future JVM versions, because the JVM ensures backward binary compatibility.
> - code analyzers may or may not be impacted. For instance, a code that analyzes the bytecode instructions, written for Java 4, will probably still work with Java 5 classes, despite the introduction of annotations. But this same code will probably no longer work with Java 7 classes, due to the introduction of the new invokedynamic instruction (even if there were no backward compatibility issues due to ASM itself; I suppose here that new elements are simply ignored by older code).
> - likewise, code transformers may or may not be impacted. A dead code removal tool is not impacted by the introduction of annotations, or even by the new invokedynamic instruction (I suppose here that an old user code simply pass the new elements unmodified to the next adapter). On the other hand, a class renaming tool is impacted by both.
>
> Because new class elements can have an unpredictable impact on existing user code, I propose the following binary compatibility contract:
> - code written for ASM version X (itself written for Java classes versions <= x) should continue to work, unmodified, for Java classes up to version x, with any future version Y > X of ASM.
> - code written for ASM version X (ie for Java classes up to version x) should *fail* with an "unsupported class version" exception if given as input a class whose version is > x, with ASM X or any other future version (this does not concern class generators, which do not have class inputs). The failure signals that this code may or may not work with the new class format, and that its author must analyze the situation to update it if necessary. I think a failure is better than silently ignoring the new elements since, as shown above, this may break the semantics of the user code without notice, which may lead to hard to find bugs.
>
> Unfortunately, this contract cannot be implemented with the current ASM core API. This is because this API is based on interfaces. Indeed, when new elements are added to the class file format, we cannot avoid the introduction of new methods or new method arguments in these interfaces (due to their visitor-based nature). This is not a problem for code generators (that simply call the interfaces), but it is a problem for class analyzers and class transformers, that must implement these interfaces. If the user implements the interfaces directly, the new methods will not be implemented by old code, leading to errors (this is what we want for completely new methods, like visitInvokeDynamicInsn, but it is *not* what we want for new versions an existing method, like when we added a "signature" argument to some method). Likewise, if the user extends a class or method adapter, old code will simply ignore the new elements, which is not what we want either.
>
> Since we cannot implement the above contract with interfaces, I propose to replace them with classes. It is a big change, but I don't see any other solution (if you find one, you are welcome). Before presenting this solution, let's review the non-backward compatible changes we made to the ASM API in the past, and which are likely to occur again in the future:
> 1) some changes were done without being motivated by a class file format change. For instance, we renamed CodeVisitor in 1.5.3 to MethodVisitor in 2.x. We also changed the boolean argument in ClassReader.accept to an int (from 2.x to 3.x). The first kind of change must be forbidden in the future. The second kind of change can be made backward compatible by keeping both versions of the method.
> 2) some completely new methods (or even interfaces) were introduced, like visitAnnotation, visitFrame, visitInvokeDynamicInsn, FieldVisitor, AnnotationVisitor, etc.
> 3) some methods were changed with new or removed arguments: the sourceFile argument in ClassVisitor.visit was moved to the new visitDebug method, the Attribute[] argument in visitField was moved to the visitAttribute method in the new FieldVisitor interface, the signature argument was added to the visitField method, etc.
> 4) the semantics of some methods changed: for instance, the Object argument of the visitLdcInsn method was initially either a String, an Integer, a Float, a Long or a Double. Then we added Type, and then Handle.
>
> Case 1) concerns changes to classes other than the XxxVisitor interfaces, and is easy to solve by forbiding us to do incompatible changes in the future (we can do that since we are not constrained here by the class file format evolution). The remaining cases can be implemented in a backward compatible way (as defined in the above contract) by replacing the XxxVisitor interfaces with classes, as follows.
>
> Let's consider an imaginary ASM interface FooVisitor in version X (for class versions <= V_X):
>
> interface FooVisitor {
>    void visitBar(int i);
> }
>
> which, in a later version Y (for class versions <= V_Y > V_X) must be changed, to
>
> interface FooVisitor {
>    void visitBar(int j, int j);
>    void visitBaz(int k);
> }
>
> in order to support new class file format elements (if we keep using the same strategy to update ASM, as we did in the past).
>
> Instead, I propose to replace FooVisitor with the following class:
>
> class FooVisitor {
>    // maximum class version supported by this visitor
>    protected int version;
>    protected FooVisitor next;
>    FooVisitor(int version) { ... }
>    FooVisitor(int version, FooVisitor next) { ... }
>
>    void visitBar(int i) { if (next != null) next.visitBar(i); }
> }
>
> in version X. In version Y, we upgrade it to (assuming that 0 is the default value of 'j'):
>
> class FooVisitor {
>    protected int version;
>    protected FooVisitor next;
>    FooVisitor(int version) { ... }
>    FooVisitor(int version, FooVisitor next) { ... }
>
>    @Deprecated
>    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_Y)
>            if (j == 0)
>                visitBar(i);
>            else
>                throw new RuntimeException();
>        if (next != null) next.visitBar(i, j);
>    }
>
>    void visitBaz(int k) {
>        if (version < V_Y) throw new RuntimeException();
>        if (next != null) next.visitBaz(k);
>    }
> }
>
> In ClassReader version X we call visitBar(int), while in version Y we call visitBar(int,int) and visitBaz(int) (in both cases we throw an exception in accept() if the class version is larger than X or Y, respectively). Similarly, ClassWriter version X implements visitBar(int), while version Y implements the 3 methods, with:
>
> ClassWriter {
>    ...
>    void visitBar(int i) { visitBar(i,0); }
>    void visitBar(int i, int j) { ... }
>    void visitBaz(int k) { ... }
> }
>
> Thus, a class generator written for version X will still work with version Y. Now a class analyzer written for version X must be implemented like this:
>
> class MyFooAnalyzer extends FooVisitor {
>    MyFooAnalyzer(...) { super(V_X); ... }
>    visitBar(int i) { ... } // optional
> }
>
> and one for version Y like this:
>
> class MyFooAnalyzer extends FooVisitor {
>    MyFooAnalyzer(...) { super(V_Y); ... }
>    visitBar(int i, int j) { ... } // optional
>    visitBaz(int k) { ... } // optional
> }
>
> If CX is a class with version X and CY a class with version Y, we have 8 cases:
> - analyzing CX with MyFooAnalyzer X and ASM X is ok (ClassReader calls visitBar(i) directly in MyFooAnalyzer X, without indirections), as desired.
> - analyzing CX with MyFooAnalyzer X and ASM Y is ok (ClassReader calls visitBar(i,0), that FooVisitor Y delegates to visitBar(i), overriden in MyFooAnalyzer X -- or not, but both cases work). This follows clause 1 of the contract, as desired.
> - analyzing CX with MyFooAnalyzer Y and ASM Y is ok (ClassReader calls visitBar(i,0) directly in MyFooAnalyzer Y, without indirections), as desired.
> - analyzing CY with MyFooAnalyzer X and ASM X will throw an exception in ClassReader X, as desired (contract clause 2).
> - analyzing CY with MyFooAnalyzer X and ASM Y will throw an exception in FooVisitor.visitBaz Y, as desired (contract clause 2). Likewise, we get an exception if ClassReader Y calls visitBar(i,j) with j != 0.
> - analyzing CY with MyFooAnalyzer Y and ASM Y is ok (ClassReader calls visitBar(i,j) and visitBaz(k) directly in MyFooAnalyzer, without indirections)
> The two remaining cases concern the analysis of CX or CY with MyFooAnalyzer Y and ASM X, i.e. a case of *forward* binary compatibility (we are trying to use a component written for ASM Y with ASM X). Analyzing CY will give an exception in ClassReader X, as desired, but analyzing CX with MyFooAnalyzer Y and ASM X will not work (ClassReader X will call visitBar(i), but MyFooAnalyzer Y implements visitBar(i,j)). But we never said that ASM should also enfore a forward binary compatibility, so these two cases can be safely ignored.
>
> Note that the absence of visitBaz in MyFooAnalyzer X leads to an error with class versions > V_X, but its potential absence in MyFooAnalyzer Y does not. This is what we want: in MyFooAnalyzer X we had no idea of the possibility of 'baz' elements, so we want a failure when we encouter one. In MyFooAnalyzer Y we know these elements, but we may not need to analyze them.
>
> Note also that indirections occur only when using old code with new ASM versions, which should provide an additional motivation, besides the 'deprecated' warnings, to upgrade it as soon as possible.
>
> Unless there is a flaw somewhere that I don't see, I think that this method can be used to implement the changes of type 2 or 3 above in a backward compatible way, both for class generators, analyzers and transformers (this is trivial for generators; the example is based on a code analyzer, but it also works for a code adapter -- indeed I proposed in the FooVisitor classes above to merge the XxxAdapter classes in the interfaces as well, to avoid having both XxxVisitor and XxxAdapter classes; but both choices are possible). I think this method also works if several analyzers or adapters written for different ASM versions are mixed together in complex analysis or transformation chains (the oldest one being then the limiting 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.
>
> For case 4, I think the contract can only be enforced by users, by checking in their code that the received arguments have the possible values declared in the ASM version they use (and if not throw an exception).
>
> Finally, it remains the case of the tree API. Adding fields to the XxxNode classes does not break the binary compatibility, and so old code can still "work" (i.e. compile) with new ASM versions in this case. Howhever, as shown above, this old code may in fact need to take these new fields into account to preserve its semantics. But I don't see here a way to automatically throw a "unsupported class version" exception in such cases (to follow clause 2 of the contract). The only solution I see is, again, that users explicitly test that the input class version is less than or equal to the version for which they have been implemented (and if not throw an error). Then, this test will also cover the addition of new XxxNode classes (that old code may find unexpectedly in instructions lists for instance).
>
>
> --
> 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
>
>

-- 
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