Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bytecode modifications for object versioning #3091

Open
maxonfjvipon opened this issue Apr 14, 2024 · 10 comments
Open

Bytecode modifications for object versioning #3091

maxonfjvipon opened this issue Apr 14, 2024 · 10 comments

Comments

@maxonfjvipon
Copy link
Member

maxonfjvipon commented Apr 14, 2024

One of the parts of the #1602 implementation is bytecode modifications.

We need separated standalone mojo in eo-maven-plugin.

The parameters of the mojo:

  1. input directory - path/file
  2. output directory - path/file
  3. hash - string

Algorithm of the mojo:

  1. Scan input directory, which contains .class files
  2. Make a list of objects/names in which .class files @Versionized annotation is used. Let's call this list L.
  3. For every object in L:
    3.1. modify corresponding .class file by appending hash (see parameters of the mojo) in front of every object usage from the list L (if A.class contains the usage of object org.eolang.B and object B in org.eolang.B.class has @Versionized annotation, in A.class this usage should be replaced with $hash.org.eolang.B, see example)
    3.2. save modified .class file to output directory appending hash in front of relative path (if we modified org/eolang/A.class, it should be saved to output directory as $hash/org/eolang/A.class)
    3.3 if object usage is not in the list L - it should not be modified in the .class file

Example

input directory: target/classes
output directory: target/modified-classes
hash: qwerty

Before

target
|-classes
  |-org
    |-eolang
      |-A.class
      |-B.class
      |-C.class

A.class (decompiled to java)

package org.eolang;
@Versionized
class A {
  B b;
  C c;
  // ...
}

B.class (decompiled to java)

package org.eolang;
@Versionized
class B {
  C c;
  // ...
}

C.class (decompiled to java)

package org.eolang;
class C {
  A a;
  // ...
}

After:

target
target
|-classes
  |-org
    |-eolang
      |-A.class
      |-B.class
      |-C.class
|-modified-classes
  |-org
    |-eolang
      |-C.class
  |-qwerty
    |-org
      |-eolang
        |-A.class
        |-B.class
  

A.class (decompiled to java)

package qwerty.org.eolang;
@Versionized
class A {
  org.eolang.C c;
  B b;
  // ...
}

B.class (decompiled to java)

package qwerty.org.eolang;
@Versionized
class B {
  org.eolang.C
  // ...
}

C.class (decompiled to java)

package org.eolang;
class C {
  qwerty.org.eolang.A a;
  // ...
}

Implemented mojo + integration test that confirms that mojo works = resolved issue.

The issue may be done in several steps (you may use less or more):

  1. integration test that expects the final result but does not work, with @Disabled annotation
  2. template of mojo + just copying files from input directory to output directory + test for it
  3. some classes for bytecode modifications + tests for them
  4. full mojo implementation + enabled integration test
@SevyConst
Copy link

SevyConst commented May 6, 2024

Hi! I think I've already done most part of the feature. But I've just noticed that I don't understand why annotation @Versionized should be removed in B.class?

A.class (decompiled to java)

package qwerty.org.eolang;
@Versionized
class A {
  org.eolang.C c;
  B b;
  // ...
}

B.class (decompiled to java)

package qwerty.org.eolang;
class B {
  org.eolang.C
  // ...
}

@maxonfjvipon
Copy link
Member Author

maxonfjvipon commented May 6, 2024

@SevyConst it's an our mistake, thank you. It the result B class annotation should not be removed. But actually after modification this annotation does not really matter.

@maxonfjvipon
Copy link
Member Author

maxonfjvipon commented May 6, 2024

@SevyConst if you took this task, it would be better to assign you to the ticket so nobody else touches it.
So should I assign you?

@SevyConst
Copy link

SevyConst commented May 6, 2024

@maxonfjvipon yes, it would be great☺️ I think, I will try to do a pull request in two days. I didn't break the issue down into the steps - my fault

@maxonfjvipon
Copy link
Member Author

maxonfjvipon commented May 6, 2024

@SevyConst the larger pull request - the harder it would be for review. I strongly recommend you to break it down into at least two steps. It would be easer for you to get your changes merged and for us to review it.

@SevyConst
Copy link

Ok, I will do it

@SevyConst
Copy link

SevyConst commented May 12, 2024

Hi, @maxonfjvipon I wrote the integration test. It contains more than 1k lines, because the testing is very important for such core thing as versioning. I got stuck with qulice - there are 926 violations only in the test, though I had tried to write code in your style. Could you please recommend some tools for reformatting?

@maxonfjvipon
Copy link
Member Author

@SevyConst that's the reason I recommended to split the task into the small peaces)
Unfortunately there's no a specific tool for such reformatting. We do it by our hands.
Could you please at least make a pull request so we could at least look at the code

@yegor256
Copy link
Member

@SevyConst sometimes IntelliJ IDEA auto-formatting helps

SevyConst added a commit to SevyConst/Huawei-Pre-Employment-Test that referenced this issue May 13, 2024
…mployment Test (Konstantin Lopatko)

Preview - what has been done at the current moment.

- Add ASM library. You are already using it inside ByteBuddy (Mockito dependency)
- Integration test is ready except code style.
- Bytecode modification works as described in the issue
@SevyConst
Copy link

@yegor256 @maxonfjvipon thank you for answering. I have just made a pull-request so you can see what have been done at the current moment. That’s why I added not only the integration test but also a Mojo class. Please see the description of the pull request for more details. I also just have made some auto-formatting by IDEA but I don’t got into the details due to lack of time. Unfortunately, I will not have much time in the next 10 days

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants