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

Fix to Ensure All Rules Satisfy not isInterface #695

Merged
merged 3 commits into from
Jun 3, 2024

Conversation

wengyingjian
Copy link
Contributor

Fix

  • Add a unit test to verify that the fix works.

  • Explain briefly why the bug exists and how to fix it.
    According to the code principles of net.bytebuddy.matcher.ElementMatcher.Junction:
    In the current code, the running effect can only ensure that the first NamedMatch satisfies not isInterface:
    (((NamedMatch) and (not(isInterface))) or IndirectMatch) or...IndirectMatch.

    After moving not isInterface to the end, the running effect can ensure that all rules satisfy not isInterface:
    (((NamedMatch) or (IndirectMatch)) or...IndirectMatch) and not(isInterface).

  • If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #.

  • Update the CHANGES log.

@CodePrometheus
Copy link

In the current code, the running effect can only ensure that the first NamedMatch satisfies not isInterface

@wengyingjian I think the current Junction expression is

(A and not(isInterface())) or (B and not(isInterface())) or ...

Is my understanding right?

@wu-sheng wu-sheng added this to the 9.3.0 milestone May 31, 2024
@wu-sheng
Copy link
Member

wu-sheng commented Jun 1, 2024

I think original request is all things should be not an interface.

First or last are not accurate.

It should use and with all other condition or before all other checks run.

@wengyingjian
Copy link
Contributor Author

Yes, because I didn't find a place that required modifying the bytecode for interfaces, I think all matching rules should be non-interface based.

If that's the case, adding and (not isInterface) in the for loop is indeed a safer approach.

Additionally, I've noticed that some custom IndirectMatch instances include not interface, while some do not.
I'm not sure if this means there is already a place that requires modifying the interface bytecode, or if it's just to maintain flexibility, allowing subclasses to implement it themselves.

@wu-sheng
Copy link
Member

wu-sheng commented Jun 3, 2024

In theory, there is no point to intrument an interface, especially we require to add new field to the target.

@wu-sheng wu-sheng added the bug Something isn't working label Jun 3, 2024
@wu-sheng
Copy link
Member

wu-sheng commented Jun 3, 2024

I've noticed that some custom IndirectMatch instances include not interface, while some do not.

I think those are not necessary, but as the bug you are fixing, they may be added as it is.

@wu-sheng
Copy link
Member

wu-sheng commented Jun 3, 2024

In the current code, the running effect can only ensure that the first NamedMatch satisfies not isInterface

@wengyingjian I think the current Junction expression is

(A and not(isInterface())) or (B and not(isInterface())) or ...

Is my understanding right?

I think the logic error is, that the is not interface could be overridden by the or from the 2nd condition, as it is flat.

@wu-sheng wu-sheng merged commit 313d7d5 into apache:main Jun 3, 2024
191 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants