beanshell / beanshell

Beanshell scripting language
Apache License 2.0
819 stars 183 forks source link

Parent for class generation and reload issues #694

Open nickl- opened 1 year ago

nickl- commented 1 year ago

EDIT: Turned this into a discussion and parent task, as it turns out there are multiple defects, with class generation. To manage these the parent task will group the separated issues.

Original post titled: Late binding on class generation now #696

Currently BeanShell requires bindings, like parent classes, to be in place already when classes are declared (and generated). This should not be a terminating exception, only at instantiation would binding be required, if we can defer the generation until then late binding for class generation will be accomplished.

As an example this is how jshell deals with it:

jshell> class A extends B {}
|  created class A, however, it cannot be referenced until class B is declared

jshell> A.class;
|  Error:
|  cannot find symbol
|    symbol:   class A
|  A.class;
|  ^

jshell> class B {}
|  created class B

jshell> A.class;
$3 ==> class A

As we can see, even though it claims that class A was created, it was not. Only once the dependencies were available could the class be generated and made accessible.

While solving the issues with super I also discovered that parent classes are instantiated in order to generate (or instantiate need to make sure) a child class. Perhaps this is required but would be preferable to avoid since these instances are not what is referenced and used in the end, they simply become garbage.

nickl- commented 1 year ago

Related to late binding but slightly more complicated.

At #335 we discovered a valid java class definition which BeanShell cannot generate.

public abstract class MyParent {
    public static final MyParent INSTANCE = new MyClass();
    public abstract int getValue(Number number);

    private static class MyClass extends MyParent {
        public int getValue(Number number) {
            return 1;
        }
    }
}
// Error: Evaluation Error: Class: MyParent not found in namespace : at Line: 9 : in file: <unknown file> : MyParent

A chicken or egg problem, members of the class definition references itself, and an inner class extends its parent. In this case the class itself is missing and required and cannot be made available later (late binding).

However this can be accomplished if we first create only the empty class and then recreate the class with its cyclic redundancy.

BeanShell 3.0.0-SNAPSHOT.3578
public abstract class MyParent {}
--> $0 = class MyParent :Class
public abstract class MyParent {
    public static final MyParent INSTANCE = new MyClass();
    public abstract int getValue(Number number);

    private static class MyClass extends MyParent {
        public int getValue(Number number) {
            return 1;
        }
    }
}
--> $1 = class MyParent :Class

However because of the lack of late binding support we cannot do it the other way around because BeanShell will refuse any further mention of the problem class if an exception occurred.

After the first example trying to redefine the empty class results in the following error:

public abstract class MyParent {}
// Error: Internal Error: Defining class problem: MyParent: BeanShell cannot yet simultaneously define two or more dependent classes of the same name.  Attempt to define: MyParent while defining: MyParent
nickl- commented 1 year ago

The "Defining class problem" is actually not related, a cache of the currently defined class name is kept because, if I understand it correctly, BeanShell had an issue creating two clåsses with the same name from different packages. I think because the class generation was aborted our class was never removed from the currently defined list.

This is the code:

https://github.com/beanshell/beanshell/blob/e2477aa6d06a5b3c66472151581825ce9e1d3154/src/main/java/bsh/BshClassManager.java#L632-L655

Commenting out the exception allows us to recreate the offending class after the failed generation. Not entirely sure what the comment refers to about "...namespace imports in an analogous..." but I don't see any problem with generating two classes of the same name from different packages with the exception commented.

package a;
--> void
class A { };
--> $2 = class a.A :Class
a.A.class;
--> $3 = class a.A :Class
package b;
--> void
class A { };
--> $4 = class b.A :Class
a.A.class;
--> $5 = class a.A :Class
b.A.class;
--> $6 = class b.A :Class

Will investigate more maybe this is not the exact issue that is being prevented but if this is really not an issue anymore we should remove the definingClasses cache as a whole and not just remove the exception, as throwing this exception is the whole purpose of this cache and would otherwise have no purpose.

nickl- commented 1 year ago

Also tested dependency, extending a class with the same name from a different package to see if that was the issue:

package c;
--> void
class A extends b.A { };
--> $7 = class c.A :Class
c.A.class;
--> $8 = class c.A :Class
c.A.class.getSuperclass();
--> $9 = class b.A :Class

Nothing the matter there... perhaps we can remove this defining classes workaround/hack? removed at d6d3746facc25aa6ad8a81951401cfe6a5e3c49c

nickl- commented 1 year ago

Turns out BSHType class reload event listeners are required after all: tracked with #699

BeanShell 3.0.0-SNAPSHOT.3577
class A {}
--> $0 = class A :Class
class A {
    static A INSTANCE = new A();
}
--> $1 = class A :Class
A.INSTANCE;
// Error: Target Exception: Exception in static init block <clinit> for class A. With message: Typed variable declaration : Can't assign A to A : at Line: 3 : in file: <unknown file> : static A INSTANCE = new A ( )
: at Line: 5 : in file: <unknown file> : A .INSTANCE
Caused by: bsh.UtilEvalError: Exception in static init block <clinit> for class A. With message: Typed variable declaration : Can't assign A to A : at Line: 3 : in file: <unknown file> : static A INSTANCE = new A ( )

bsh.EvalError: Typed variable declaration : Can't assign A to A : at Line: 3 : in file: <unknown file> : static A INSTANCE = new A ( )

bsh.UtilEvalError: Can't assign A to A

With the listener enabled and resetting the instance cache on BSHType:

BeanShell 3.0.0-SNAPSHOT.3665
class A {}
--> $0 = class A :Class
class A {
    static A INSTANCE = new A();
}
--> $1 = class A :Class
A.INSTANCE;
--> $2 = A@5a39699c :A
A.class == A.INSTANCE.getClass();
--> $3 = true :boolean

Because the type is reload aware the two classes are now the same.

nickl- commented 1 year ago

TODO: Inner class extends outer still requires outer class to be defined first, but then when reloaded we have two different outer classes. tracked with #698

BeanShell 3.0.0-SNAPSHOT.3665
class A {}
--> $0 = class A :Class
class A {
    class B extends A {}
}
--> $1 = class A :Class
A.class == A$B.class.getSuperclass(); // this should be true
--> $2 = false :boolean
nickl- commented 1 year ago

TODO: parent class reload does not change child class parent tracked with #697

BeanShell 3.0.0-SNAPSHOT.3665
class A {
    int get() {
        return 1;
    }
}
--> $0 = class A :Class
new A().get();
--> $1 = 1I :int
class B extends A { }
--> $2 = class B :Class
new B().get();
--> $3 = 1I :int
class A {
    int get() {
        return 2;
    }
}
--> $4 = class A :Class
new A().get();
--> $5 = 2I :int
new B().get();
--> $6 = 1I :int
A.class == B.class.getSuperclass(); // this should be true
--> $7 = false :boolean
nickl- commented 1 year ago

While solving the issues with super I also discovered that parent classes are instantiated in order to generate (or instantiate need to make sure) a child class. Perhaps this is required but would be preferable to avoid since these instances are not what is referenced and used in the end, they simply become garbage.

This has to do with modifier validation, in order to get the modifiers from the non static generated methods and/or variables, it appears we need an instance. The generated class only has a subset of the declared modifiers baked into byte code to allow us more flexibility and freedom to use reflection on these classes, the additional modifier validation is applied manually.

Will look and see if there is an alternative...

nickl- commented 1 year ago

Not sure how we are supposed to reload or reinitialize classes. #698 #697

The class generator uses defineClass to register byte code with the class loader and produce a class. It then calls a method reloadClasses which is written to cater for multiple classes but only gets used per single generated class.

https://github.com/beanshell/beanshell/blob/57c851821b2aa7a5327170076a7e2f7937c3f216/src/main/java/bsh/classpath/ClassManagerImpl.java#L587-L596

Even though we have all the class sources stored reloading them does not work because the static name space is not configured. There are also additional context stored in the static name space as per the comments on the initStaticNameSpace method from the class generator.

https://github.com/beanshell/beanshell/blob/57c851821b2aa7a5327170076a7e2f7937c3f216/src/main/java/bsh/ClassGeneratorUtil.java#L167-L185

Bottom line, it isn't a simple reload of generated classes in the class loader.