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

Array type reference not working when aspect is compiled separately from referenced type #24

Closed
kriegaex opened this issue Oct 19, 2020 · 8 comments · Fixed by #210
Closed
Assignees
Labels
bug Something isn't working
Milestone

Comments

@kriegaex
Copy link
Contributor

I have found StackOverflow question #64416863 and can reproduce the author's problem with AspectJ 1.9.6. I further simplified the code a bit to now read:

public aspect MyAspect {
  after() : execution(public MaybeMissingClass MaybeMissingClass.*()) {
    System.out.println(thisJoinPoint);
  }

  after() : execution(public MaybeMissingClass[] MaybeMissingClass.*()) {
    System.out.println(thisJoinPoint);
  }
}
public class MaybeMissingClass {
  public static void main(String[] args) {
    f1();
    f2();
  }

  public static MaybeMissingClass f1() {
    System.out.println("MaybeMissingClass.f1");
    return null;
  }

  public static MaybeMissingClass[] f2() {
    System.out.println("MaybeMissingClass.f2");
    return null;
  }
}

When compiling everything together, it works as expected:

foo> ajc -cp aspectjrt.jar -1.8 MyAspect.aj MaybeMissingClass.java

foo> java -cp .;aspectjrt.jar MaybeMissingClass
MaybeMissingClass.f1
execution(MaybeMissingClass MaybeMissingClass.f1())
MaybeMissingClass.f2
execution(MaybeMissingClass[] MaybeMissingClass.f2())

When compiling the aspect separately however, the second advice unexpectedly intercepts target method f1:

foo> ajc -cp aspectjrt.jar -1.8 -outjar aspect.jar MyAspect.aj
foo\MyAspect.aj:11 [warning] no match for this type name: MaybeMissingClass [Xlint:invalidAbsoluteTypeName]
after() : execution(public MaybeMissingClass MaybeMissingClass.*()) {
                           ^^^^^^^^^^^^^^
        [Xlint:invalidAbsoluteTypeName]
foo\MyAspect.aj:11 [warning] no match for this type name: MaybeMissingClass [Xlint:invalidAbsoluteTypeName]
after() : execution(public MaybeMissingClass MaybeMissingClass.*()) {
                                             ^^^^^^^^^^^^^^^^
        [Xlint:invalidAbsoluteTypeName]
foo\MyAspect.aj:15 [warning] no match for this type name: MaybeMissingClass [Xlint:invalidAbsoluteTypeName]
after() : execution(public MaybeMissingClass[] MaybeMissingClass.*()) {
                           ^^^^^^^^^^^^^^^^
        [Xlint:invalidAbsoluteTypeName]
foo\MyAspect.aj:15 [warning] no match for this type name: MaybeMissingClass [Xlint:invalidAbsoluteTypeName]
after() : execution(public MaybeMissingClass[] MaybeMissingClass.*()) {
                                               ^^^^^^^^^^^^^^^^
        [Xlint:invalidAbsoluteTypeName]
foo\MyAspect.aj:15 [warning] advice defined in MyAspect has not been applied [Xlint:adviceDidNotMatch]

foo\MyAspect.aj:11 [warning] advice defined in MyAspect has not been applied [Xlint:adviceDidNotMatch]


6 warnings

foo> ajc -cp aspectjrt.jar -1.8 -aspectpath aspect.jar MaybeMissingClass.java

foo> java -cp .;aspect.jar;aspectjrt.jar MaybeMissingClass
MaybeMissingClass.f1
execution(MaybeMissingClass MaybeMissingClass.f1())
execution(MaybeMissingClass MaybeMissingClass.f1())
MaybeMissingClass.f2
@kriegaex
Copy link
Contributor Author

kriegaex commented Oct 19, 2020

BTW, when modifying the aspect like this experimentally

public aspect MyAspect {
  after() : execution(public MaybeMissing* MaybeMissing*.*()) {
    System.out.println(thisJoinPoint);
  }

  after() : execution(public MaybeMissing*[] MaybeMissing*.*()) {
    System.out.println(thisJoinPoint);
  }
}

now both advices match both methods, i.e. the absolute type name as such does not seem to be the (only) problem. This is true for both compiling the aspect separately or together with the target class. The program output will be:

MaybeMissingClass.f1
execution(MaybeMissingClass MaybeMissingClass.f1())
execution(MaybeMissingClass MaybeMissingClass.f1())
MaybeMissingClass.f2
execution(MaybeMissingClass[] MaybeMissingClass.f2())
execution(MaybeMissingClass[] MaybeMissingClass.f2())

There seems to be some imprecision with pointcut parsing in this case.

@kriegaex
Copy link
Contributor Author

Some feedback from a maintainer would be welcome. Thank you.

@aclement
Copy link
Contributor

Not sure what I can say, looks like a bug as you say. Maybe in parsing of point cuts (PatternParser), maybe in how array information is remembered (TypePattern hierarchy), maybe how matching of type patterns is done (TypePattern.matches*).

@kriegaex kriegaex added the bug Something isn't working label Mar 21, 2022
@kriegaex
Copy link
Contributor Author

kriegaex commented Jan 8, 2023

I started adding tests reproducing both the currently working and failing cases to the code base in a branch. For the second case with the fuzzy pointcuts - return types MaybeMissing* and MaybeMissing*[] - I noticed that matching is also wrong (too broad) when aspect and target class are compiled together, not just in the separate compilation case. I.e., there are two distinct types of bugs, even though they might be related.

kriegaex added a commit that referenced this issue Jan 8, 2023
Relates to #24.

Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
@kriegaex
Copy link
Contributor Author

kriegaex commented Jan 8, 2023

For the first case with the exact type names, I compared javap -v output for the aspects generated when compiled together with the target class or separately. FWIW,

  • the executable byte code looks exactly identical,
  • the separately compiled aspect's embedded binary org.aspectj.weaver.Advice and org.aspectj.weaver.WeaverState data are larger than the other one's,
  • the aspect compiled together with the target class contains some constant table data which the other aspect does not:
    #34 = Utf8               LMaybeMissingClass;
    ...
    #51 = Utf8               [LMaybeMissingClass;
    
    It is not a big surprise that this type information is not available in the separately compiled aspect, I am just collecting data at thios point.

@kriegaex
Copy link
Contributor Author

kriegaex commented Jan 8, 2023

Not sure what I can say, looks like a bug as you say. Maybe in parsing of point cuts (PatternParser), maybe in how array information is remembered (TypePattern hierarchy), maybe how matching of type patterns is done (TypePattern.matches*).

Pointcut parsing and array information storage seem to be OK. In both cases (exact and fuzzy/wildcard type patterns), TypePattern.dim equals 1 after parsing the pointcuts. I need to debug more, but it looks as if it might actually be a matching rather than a parsing or storage problem.

@kriegaex
Copy link
Contributor Author

kriegaex commented Jan 8, 2023

I managed to locally fix the matcher for one-dimensional array return types. For multi-dimensional ones like MaybeMissingClass[][] however, currently it only works when compiling aspect and target class together. When creating a separate aspect library, the resulting aspect somehow only contains 2 instead of 3 advice methods for this configuration:

public aspect ExactlyMatchingAspect {
  after() : execution(public MaybeMissingClass MaybeMissingClass.*()) {
    System.out.println(thisJoinPoint);
  }

  after() : execution(public MaybeMissingClass[] MaybeMissingClass.*()) {
    System.out.println(thisJoinPoint);
  }

  after() : execution(public MaybeMissingClass[][] MaybeMissingClass.*()) {
    System.out.println(thisJoinPoint);
  }
}

Maybe there is something wrong in equals and/or hashCode methods. I am documenting this for myself, because I will be very busy during the next few weeks starting tomorrow. Maybe I will not get around to continuing the debug session in a while, so I am documenting the status quo for now.


Update: I have a local prototype working with multi-dimensional array return types, but it needs to be verified, tidied up and checked. E.g., so far I only tried with arrays of reference types, not of primitive types yet. While I am at it, I also want to look into array type matching for method parameter and field types, not just return types.

kriegaex added a commit that referenced this issue Jan 9, 2023
More test cases were added for
  - multi-dimensional arrays,
  - primitive type arrays.

Relates to #24.

Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
kriegaex added a commit that referenced this issue Jan 9, 2023
The method falsely determined that a one-dimensional array was not an
array due to a one-off bug.

Relates to #24.

Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
kriegaex added a commit that referenced this issue Jan 9, 2023
A simple boolean condition is enough.

Loosely relates to #24, but
actually it is just drive-by cosmetics.

Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
kriegaex added a commit that referenced this issue Jan 9, 2023
Fixes #24, both the array
return type matching as such as well as matching dimensionality patterns
correctly. E.g., 'Foo*[]' is not the same as 'Foo*[][]'. This also works
correctly in combination with asterisks, even for primitive types, i.e.
'in*[][]' correctly matches a 2-dimensional array of 'int'.

Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
kriegaex added a commit that referenced this issue Jan 15, 2023
Relates to #24.

Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
kriegaex added a commit that referenced this issue Jan 15, 2023
Especially 'hashCode' did not correspond to 'equals', disregarding
several fields, array dimension information being only one of them. This
led to parts of pointcuts being ignored, because they were regarded as
duplicates. Example:

execution(Foo* *(..)) && !execution(Foo*[] *(..))

Here, the negated pattern was falsely regarded as equal to the first
pattern, leading to an "A && !A" situation, i.e. no match at all.

Furthermore, 'toString' did not print array strings, i.e. instead of
"Foo*[][]" something like "Foo*" was printed. This false information was
also present in annotations generated by the weaver.

FuzzilyMatchingAspect was adjusted to actually match exactly once, as
expected, for the "Foo*" return types, i.e. exclusions for the array
return types have been added.

Relates to #24.

Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
kriegaex added a commit that referenced this issue Jan 15, 2023
After WildTypePattern.hashCode was fixed in the previous commit,
PointcutRewriterTest started failing, because in many places it was
falsely relying on a specific order of hash codes, which cannot be
guaranteed, especially since more instance fields are part of the hash
code now in accordance with 'equals'.

The new test helper class LogicalPointcutStructure is able to recognise
chained '&&' and '||' pointcuts of the same logical nesting level,
un-nesting them from the actual pointcut structure and making them
comparable, disregarding their order. I.e., something like
  ((A && B) && C) && D
is actually recognised to logically be
  A && B && C && D
and equivalent to e.g. either of
  D && B && A && C
  A && B && D && C
  C && A && D && B

This helps to compare rewritten pointcuts, as long as their logical
structure has not been altered.

Relates to #24.

Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
kriegaex added a commit that referenced this issue Jan 15, 2023
Relates to #24.

Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
kriegaex added a commit that referenced this issue Jan 15, 2023
More test cases were added for
  - multi-dimensional arrays,
  - primitive type arrays.

Relates to #24.

Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
kriegaex added a commit that referenced this issue Jan 15, 2023
The method falsely determined that a one-dimensional array was not an
array due to a one-off bug.

Relates to #24.

Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
kriegaex added a commit that referenced this issue Jan 15, 2023
A simple boolean condition is enough.

Loosely relates to #24, but
actually it is just drive-by cosmetics.

Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
kriegaex added a commit that referenced this issue Jan 15, 2023
Fixes #24, both the array
return type matching as such as well as matching dimensionality patterns
correctly. E.g., 'Foo*[]' is not the same as 'Foo*[][]'. This also works
correctly in combination with asterisks, even for primitive types, i.e.
'in*[][]' correctly matches a 2-dimensional array of 'int'.

Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
kriegaex added a commit that referenced this issue Jan 15, 2023
Relates to #24.

Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
kriegaex added a commit that referenced this issue Jan 15, 2023
Especially 'hashCode' did not correspond to 'equals', disregarding
several fields, array dimension information being only one of them. This
led to parts of pointcuts being ignored, because they were regarded as
duplicates. Example:

execution(Foo* *(..)) && !execution(Foo*[] *(..))

Here, the negated pattern was falsely regarded as equal to the first
pattern, leading to an "A && !A" situation, i.e. no match at all.

Furthermore, 'toString' did not print array strings, i.e. instead of
"Foo*[][]" something like "Foo*" was printed. This false information was
also present in annotations generated by the weaver.

FuzzilyMatchingAspect was adjusted to actually match exactly once, as
expected, for the "Foo*" return types, i.e. exclusions for the array
return types have been added.

Relates to #24.

Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
kriegaex added a commit that referenced this issue Jan 15, 2023
After WildTypePattern.hashCode was fixed in the previous commit,
PointcutRewriterTest started failing, because in many places it was
falsely relying on a specific order of hash codes, which cannot be
guaranteed, especially since more instance fields are part of the hash
code now in accordance with 'equals'.

The new test helper class LogicalPointcutStructure is able to recognise
chained '&&' and '||' pointcuts of the same logical nesting level,
un-nesting them from the actual pointcut structure and making them
comparable, disregarding their order. I.e., something like
  ((A && B) && C) && D
is actually recognised to logically be
  A && B && C && D
and equivalent to e.g. either of
  D && B && A && C
  A && B && D && C
  C && A && D && B

This helps to compare rewritten pointcuts, as long as their logical
structure has not been altered.

Relates to #24.

Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
@kriegaex kriegaex added this to the 1.9.20 milestone Jan 15, 2023
@kriegaex kriegaex self-assigned this Jan 15, 2023
@kriegaex
Copy link
Contributor Author

If you look at the merged PR, this issue became considerably bigger than I thought, because along the way I also improved the syntax for array matching. So, this was not just a simple bugfix.

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 a pull request may close this issue.

2 participants