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.

Sign up for GitHub

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

Jump to bottom

AJC core dump when mixing type name wildcards with generics #211

Closed
kriegaex opened this issue Jan 15, 2023 · 2 comments
Closed

AJC core dump when mixing type name wildcards with generics #211

kriegaex opened this issue Jan 15, 2023 · 2 comments
Assignees
@kriegaex
Labels
bug Something isn't working
Milestone
1.9.22.1

Comments

@kriegaex
Copy link
Contributor

kriegaex commented Jan 15, 2023 •
edited

Commit 5383736 contains an inactive test case reproducing a new problem I found by chance when fixing a failing test after the code changes for #24:

https://github.com/eclipse/org.aspectj/blob/53837367b0f04a20aab530741ea7a24a42e309e5/tests/bugs165/pr272233/Iffy2.java#L44-L45

Further up in the same class you see working variations of that syntax.

The corresponding ajcore.*.txt file says:

malformed org.aspectj.weaver.PointcutDeclaration attribute (length:219)
org.aspectj.weaver.BCException: Bad type signature *

org.aspectj.weaver.BCException: malformed org.aspectj.weaver.PointcutDeclaration attribute (length:219)org.aspectj.weaver.BCException: Bad type signature *
  at org.aspectj.weaver.AjAttribute.read(AjAttribute.java:137)
  at org.aspectj.weaver.bcel.Utility.readAjAttributes(Utility.java:102)
  at org.aspectj.weaver.bcel.BcelMethod.unpackAjAttributes(BcelMethod.java:197)
  at org.aspectj.weaver.bcel.BcelMethod.<init>(BcelMethod.java:91)
  at org.aspectj.weaver.bcel.BcelObjectType.getDeclaredMethods(BcelObjectType.java:290)
  at org.aspectj.weaver.ReferenceType.getDeclaredMethods(ReferenceType.java:870)
  at org.aspectj.weaver.ResolvedType.getDeclaredAdvice(ResolvedType.java:1028)
  at org.aspectj.weaver.ResolvedType.getDeclaredShadowMungers(ResolvedType.java:1068)
  at org.aspectj.weaver.ResolvedType.collectShadowMungers(ResolvedType.java:868)
  at org.aspectj.weaver.ResolvedType.collectCrosscuttingMembers(ResolvedType.java:794)
  at org.aspectj.weaver.CrosscuttingMembersSet.addOrReplaceAspect(CrosscuttingMembersSet.java:112)
  at org.aspectj.weaver.CrosscuttingMembersSet.addOrReplaceAspect(CrosscuttingMembersSet.java:67)
  at org.aspectj.weaver.bcel.BcelWeaver.prepareForWeave(BcelWeaver.java:512)
  at org.aspectj.ajdt.internal.compiler.AjPipeliningCompilerAdapter.ensureWeaverInitialized(AjPipeliningCompilerAdapter.java:525)
  at org.aspectj.ajdt.internal.compiler.AjPipeliningCompilerAdapter.weaveQueuedEntries(AjPipeliningCompilerAdapter.java:505)
  at org.aspectj.ajdt.internal.compiler.AjPipeliningCompilerAdapter.queueForWeaving(AjPipeliningCompilerAdapter.java:446)
  at org.aspectj.ajdt.internal.compiler.AjPipeliningCompilerAdapter.afterProcessing(AjPipeliningCompilerAdapter.java:431)
  at org.aspectj.ajdt.internal.compiler.CompilerAdapter.ajc$after$org_aspectj_ajdt_internal_compiler_CompilerAdapter$5$6b855184(CompilerAdapter.aj:104)
  at org.aspectj.org.eclipse.jdt.internal.compiler.Compiler.process(Compiler.java:946)
  at org.aspectj.org.eclipse.jdt.internal.compiler.Compiler.processCompiledUnits(Compiler.java:576)
  at org.aspectj.org.eclipse.jdt.internal.compiler.Compiler.compile(Compiler.java:476)
  at org.aspectj.org.eclipse.jdt.internal.compiler.Compiler.compile(Compiler.java:427)
  at org.aspectj.ajdt.internal.core.builder.AjBuildManager.performCompilation(AjBuildManager.java:1101)
  at org.aspectj.ajdt.internal.core.builder.AjBuildManager.performBuild(AjBuildManager.java:275)
  at org.aspectj.ajdt.internal.core.builder.AjBuildManager.batchBuild(AjBuildManager.java:188)
  at org.aspectj.ajdt.ajc.AjdtCommand.doCommand(AjdtCommand.java:103)
  at org.aspectj.ajdt.ajc.AjdtCommand.runCommand(AjdtCommand.java:47)
  at org.aspectj.tools.ajc.Main.run(Main.java:374)
  at org.aspectj.tools.ajc.Main.runMain(Main.java:253)
  at org.aspectj.tools.ajc.Ajc.compile(Ajc.java:202)
  at org.aspectj.tools.ajc.Ajc.compile(Ajc.java:163)
  at org.aspectj.tools.ajc.AjcTestCase.ajc(AjcTestCase.java:534)
  at org.aspectj.testing.CompileSpec.execute(CompileSpec.java:52)
  at org.aspectj.testing.AjcTest.runTest(AjcTest.java:60)
  at org.aspectj.testing.XMLBasedAjcTestCase.runTest(XMLBasedAjcTestCase.java:157)
  at org.aspectj.testing.XMLBasedAjcTestCase.runTest(XMLBasedAjcTestCase.java:171)
  at org.aspectj.systemtest.ajc165.Ajc165Tests.testFunkyPointcut_pr272233_2(Ajc165Tests.java:80)
@kriegaex kriegaex added the bug Something isn't working label Jan 15, 2023
@kriegaex
Copy link
Contributor Author

BTW, despite not causing any exceptions, patterns like

  • execution(java.util.Collection<*>[] *(..)),
  • execution(*..Collection<java.lang.String>[] *(..))

are not matching either. I.e., even beyond the problem above there is something fundamentally wrong with generic type parameter matching in connection with array types. For non-array types, pointcuts such as

  • execution(*..Collection<String> *(..)),
  • execution(*..Collection<*> *(..))

would match a method like Collection<String> foo(). But even without the array,

  • execution(*..Collection<?> *(..))

would cause the same org.aspectj.weaver.BCException as above.

@kriegaex
Copy link
Contributor Author

kriegaex commented Jan 17, 2023 •
edited

This is like a can of worms. I think I am understanding it better now, but there are several issues here:

  • In contrast to exact types, wildcard types store ? type parameters for generics as *, but when reading them again, this is unexpected and cannot be parsed correctly. I.e., *..Foo<?> or a.b.C*<?, String> cannot be parsed correctly. I fixed this by making the parser in for unresolved types more lenient here, allowing *, too. Maybe I can find out how to store them in a different way, but for now I am focusing on the next problem.

  • Wildcard types which are at the same time array reference types and have generic type parameters are not supported at all, e.g. *..Foo<Integer>[] or a.b.C*<String>[][] are handled incorrectly: The matcher, when trying to handle type paramaters, erroneously looks at the array reference type, but actually it needs to check the component type, because the latter contains the type parameters. I also fixed that.

  • An edge case I found in https://github.com/eclipse/org.aspectj/blob/53837367b0f04a20aab530741ea7a24a42e309e5/tests/bugs165/pr272233/Iffy2.java#L10 actually would match void myMethod(). In native syntax, this would not compile, because void arrays void[] are not allowed in Java for obvious reasons. I am still pondering how to handle that case. Either we can

    • log a warning and let it match anyway,
    • throw an error which would need to be handled downstream so as not to cause an AJ core dump, or
    • log a lint warning and make sure the pointcut doe not match anything.

    Locally, I have the first solution working. I am still trying to get my bearings and figure out how to implement the third one, which is my favourite.

  • UnresolvedType.signatureToName does a bad job displaying generic wildcard types parameters correctly. I also fixed that locally, i.e.

    • instead of <(A,B)> we now have <A,B>,
    • after a * in any type parameter list, sometimes the * (which should be converted to ?) itself and always the subsequent parameters would be missing from the signature:
      • [Pjava/util/Collection<*>; yielded java.util.Collection<>[], but should be java.util.Collection<?>[]
      • [Pjava/util/Map<*Pjava/util/List<[Ljava/lang/Integer;>;>; yielded java.util.Map<?>[], but should be java.util.Map<?,java.util.List<java.lang.Integer[]>>[]

kriegaex added a commit that referenced this issue Jan 17, 2023
@kriegaex
UnresolvedType: fix JVM signature conversion
a5aadb4 
Fixes #211. Previously, '?' was not converted to '*' in
UnresolvedType.nameToSignature, but kept as-is. That is why - falsely -
it was necessary to handle the '?' case in UnresolvedType.forSignature
at all, reading this kind of bogus signature and creating a type for it
in TypeFactory.createTypeFromSignature. This, ironically, led to correct
JVM generic type signatures containing '*' not being handled at all.

The conversion should now work correctly both ways.

Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
@kriegaex kriegaex mentioned this issue Jan 17, 2023
Merged
kriegaex added a commit that referenced this issue Jan 30, 2023
@kriegaex
UnresolvedType: fix JVM signature conversion
fce0cf2 
Fixes #211. Previously, '?' was not converted to '*' in
UnresolvedType.nameToSignature, but kept as-is. That is why - falsely -
it was necessary to handle the '?' case in UnresolvedType.forSignature
at all, reading this kind of bogus signature and creating a type for it
in TypeFactory.createTypeFromSignature. This, ironically, led to correct
JVM generic type signatures containing '*' not being handled at all.

The conversion should now work correctly both ways.

Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
kriegaex added a commit that referenced this issue Dec 2, 2023
@kriegaex
Merge branch 'master' into gh-211
9712436 
# Conflicts:
#	pom.xml
#	tests/src/test/resources/org/aspectj/systemtest/ajc1919/ajc1919.xml
kriegaex added a commit that referenced this issue Apr 11, 2024
@kriegaex
UnresolvedType: fix JVM signature conversion
88b766f 
Fixes #211. Previously, '?' was not converted to '*' in
UnresolvedType.nameToSignature, but kept as-is. That is why - falsely -
it was necessary to handle the '?' case in UnresolvedType.forSignature
at all, reading this kind of bogus signature and creating a type for it
in TypeFactory.createTypeFromSignature. This, ironically, led to correct
JVM generic type signatures containing '*' not being handled at all.

The conversion should now work correctly both ways.

Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
kriegaex added a commit that referenced this issue Apr 11, 2024
@kriegaex
UnresolvedType: fix JVM signature conversion
e88b2cf 
Fixes #211. Previously, '?' was not converted to '*' in
UnresolvedType.nameToSignature, but kept as-is. That is why - falsely -
it was necessary to handle the '?' case in UnresolvedType.forSignature
at all, reading this kind of bogus signature and creating a type for it
in TypeFactory.createTypeFromSignature. This, ironically, led to correct
JVM generic type signatures containing '*' not being handled at all.

The conversion should now work correctly both ways.

Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
@kriegaex kriegaex self-assigned this Apr 12, 2024
@kriegaex kriegaex added this to the 1.9.22.1 milestone Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kriegaex kriegaex

Labels
bug Something isn't working
Projects
None yet
Milestone
1.9.22.1
Development

No branches or pull requests
1 participant
@kriegaex