`NullPointerException` on `BcelWorld.java#L135`

[dagyu]

Hi I'm writing a Language Server for AspectJ and one of the features that I have implemented are the codeLenses that I exploit to show all the weaved points by an advice and viceversa. This GIF is an example on VSCode (I also implemented tthe client for Vim).

In order to retrieve all the weaved points I set an ICrossReferenceHandler on the BcelWorld instance, in this way when the reportMatch method is called I can retrieve all the weaved points. But there are some cases where this solution crash because the method determineRealKind can return null and on BcelWorld.java#L135 is never checked the null case and a NullPointerException is thrown.

I also created a repo to reproduce this bug programmatically https://github.com/dagyu/bcel-world-bug-poc.

This bug is really easy to fix but I didn't do it just because I don't know if the way that I choose to retrieve the weaved points is the best one. If there is a better way could you give me some suggestion?

If you don't have any suggestion in any case the bug is present, so if you will fix it I'll be really grateful.

[kriegaex]

@aclement, a quick fix would be to return "" instead of null here: https://github.com/eclipse/org.aspectj/blob/ea43aa5ed8e64ad20672f38864e0d53a5e855e2a/weaver/src/main/java/org/aspectj/weaver/bcel/BcelWorld.java#L119-L125

Another easy alternative would be determineRelKind(munger) == null ? "" : determineRelKind(munger).getName() (or using a variable to avoid the duplicate method call) here: https://github.com/eclipse/org.aspectj/blob/ea43aa5ed8e64ad20672f38864e0d53a5e855e2a/weaver/src/main/java/org/aspectj/weaver/bcel/BcelWorld.java#L133-L137

The call graph for BcelWorld.determineRelKind looks like this:

BTW, I found no references to ICrossReferenceHandler in either AspectJ or AJDT, i.e. no implementing classes relying on a null value in String kind. If you are not expecting any negative side effects, I could simply add more entries to https://github.com/eclipse/org.aspectj/blob/ea43aa5ed8e64ad20672f38864e0d53a5e855e2a/asm/src/main/java/org/aspectj/asm/IRelationship.java#L44-L58 which could then be used in BcelWorld in order to set the corresponding ICrossReferenceHandler.kind to something meaningful. WDYT, @aclement?

[kriegaex]

For now and until further feedback, I chose the cheapest fix. Actually, the other kinds, existing or imagined new ones, are not really advice types. Therefore, setting kind to null is OK, even though not super informative.

[kriegaex]

@dagyu, please give it a spin with the 1.9.10-SNAPSHOT in https://oss.sonatype.org/content/repositories/snapshots and report back. Thanks.

[dagyu]

BTW, I found no references to ICrossReferenceHandler in either AspectJ or AJDT, i.e. no implementing classes relying on a null value in String kind

If ICrossReferenceHandler actually isn't used I think that changing the signature of ICrossReferenceHandler in particular replacing String kind in AdviceKind kind could be more informative because in the null case I lose some information.

[dagyu]

@dagyu, please give it a spin with the 1.9.10-SNAPSHOT in https://oss.sonatype.org/content/repositories/snapshots and report back. Thanks.

I have no permission on the webpage that you shared but I build the package from the source code and this fix is good, but if you could change the signature of ICrossReferenceHandler.addCrossReference and passing AdviceKind instead of a tring I'll be really grateful.

Thanks for the answet.

[kriegaex]

Of course you have permission. It is the public Maven Central snapshot repository. If you use Maven or Gradle to build, simply add it as a snapshot repo. You can google how to do that, is is super easy. No need to build AspectJ from source, I did that for you already.

if you could change the signature of ICrossReferenceHandler.addCrossReference and passing AdviceKind instead of a String I'll be really grateful.

Sorry, I am not going to do a breaking change in an existing interface for you at this point. There might be other users, relying on the current behaviour. I do agree that it might have made more sense to pass a type-safe object instead of a string to begin with, but now the interface is like this and has been for many years. Maybe overloading it incl. a default implementation might be an option. But for now, I simply want to fix the bug you were kind enough to raise here.

[dagyu]

Ok thank you

[kriegaex]

BTW, I found no references to ICrossReferenceHandler in either AspectJ or AJDT, i.e. no implementing classes relying on a null value in String kind

If ICrossReferenceHandler actually isn't used I think that changing the signature of ICrossReferenceHandler in particular replacing String kind in AdviceKind kind could be more informative because in the null case I lose some information.

I want to be a bit more precise about what I said before: When I talked about not having found any implementing classes for ICrossReferenceHandler in AspectJ or AJDT, I was trying to find out if anyone might care about something is null or we could introduce new IRelationship values for all existing AdviceKind values without a counterpart. But actually, all three things describe different things, albeit partly overlapping ones:

• org.aspectj.weaver.AdviceKind has a bad name, actually, because it contains enum values for many things which are not advice types, e.g. per* instantiation type descriptors, cflow* pointcut designators and others. This is something the weaver uses internally. I am not even sure myself what exactly it does with those values.
• org.aspectj.lang.reflect.AdviceKind is actually the closest thing to what we need for the cross references here, because this enum actually describes the actual 5 advice types in AspectJ. This is however not the one used in BcelWorld, but the first one is.
• org.aspectj.asm.IRelationship describes whatever kind of "relationships" are needed in the given context. This class is also a bit fuzzy to me, because it contains a mixture of things: declare * language constructs, again the 5 advice types, but also generic "advice" and "declare" items plus a relationship kind "uses pointcut". OMG! (I am also doing some project archeology here, I am unfamiliar with that stuff, not being an AspectJ veteran.)
• org.aspectj.weaver.ICrossReferenceHandler is what you are using for your tool. It describes, AFAIK, relationships between aspects and woven code, specifically relationships which you would use in IDE-type or xref reporting tools. I think the best data type for kind would org.aspectj.lang.reflect.AdviceKind, but not the weaver AdviceKind. Probably this is what you wanted. But actually, at least since 14 years ago when the file was created, the field type was String, i.e. something you can directly use as a label and do not need to map. If now we would add more string labels for other types of cross-references, which ones would that be? Let us look at them again: https://github.com/eclipse/org.aspectj/blob/ea43aa5ed8e64ad20672f38864e0d53a5e855e2a/weaver/src/main/java/org/aspectj/weaver/bcel/BcelWorld.java#L119-L125 None of these are things you can cross-reference to places in the application source code. Arguably, maybe you could jump from ITDs to target classes and vice versa, but those are not even listed here. So actually, extending something here would not help you with your tool. If you disagree, please explain.