Community
Participate
Working Groups
*** to be discussed on aspectj-dev / users before implementing *** We have found it useful to be able to include contextual information in the messages produced by declare warning /error. We want to be able to write something like: declare warning: call(* com.ibm.ws..*(..)) : "Call to protected WAS API: %s"; and get the compiler message: "Call to protected WAS API: void com.ibm.ws.SomeClass.someMethod(boolean)" We prototyped a solution using substitution variables as follows: %% inserts a % character %k inserts the joinpoint kind %s inserts the signature at the joinpoint %t inserts the name of the declaring type at the joinpoint %i inserts the name of the identifier at the joinpoint %j inserts a string representation of the joinpoint %f inserts the filename where the match was found %l inserts the line number where the match was found %a inserts the file and line number of the declare error/warning definition (this may not be the perfect set...) Here's a complete mini-example showing all the subsitutions in action : Hello.java ========= public class Hello { public static void main(String[] args) { System.out.println("Hello"); new Hello().sayItToo(); } private void sayItToo() { System.out.println("me too"); } } Warn.java ========= public aspect Warn { declare warning : execution(* Hello.*(..)) : "My warning:\n%%k=%k\n%%s=%s\n%%t=%t\n%%i=%i\n%%j=%j\n%%f=%f\n%%l=% l\n%%a=%a"; declare warning : call(* Hello.*(..)) : "Illegal call to %t.%i(..)"; } To get the following output: C:\ColyerRoot\Data\AspectJDev\eclipse\workspace\Notes\decwtest\Hello.java:4 Illegal call to Hello.sayItToo(..) C:\ColyerRoot\Data\AspectJDev\eclipse\workspace\Notes\decwtest\Hello.java:3 My warning: %k=method-execution %s=void Hello.main(java.lang.String[]) %t=Hello %i=main %j=method-execution(void Hello.main(java.lang.String[])) %f=C:\ColyerRoot\Data\AspectJDev\eclipse\workspace\Notes\decwtest\Hello.java %l=3 %a=C:\ColyerRoot\Data\AspectJDev\eclipse\workspace\Notes\decwtest\Warn.java:3 C:\ColyerRoot\Data\AspectJDev\eclipse\workspace\Notes\decwtest\Hello.java:8 My warning: %k=method-execution %s=void Hello.sayItToo() %t=Hello %i=sayItToo %j=method-execution(void Hello.sayItToo()) %f=C:\ColyerRoot\Data\AspectJDev\eclipse\workspace\Notes\decwtest\Hello.java %l=8 %a=C:\ColyerRoot\Data\AspectJDev\eclipse\workspace\Notes\decwtest\Warn.java:3 3 warnings The following patch implements the extension (warning - based on a version of Checker.java from a while back). Index: Checker.java =================================================================== RCS file: /home/technology/org. aspectj/modules/weaver/src/org/aspectj/weaver/Checker.java,v retrieving revision 1.5 diff -u -r1.5 Checker.java --- Checker.java 12 Mar 2003 19:51:43 -0000 1.5 +++ Checker.java 19 Jul 2003 12:34:12 -0000 @@ -17,6 +17,7 @@ import java.util.Collections; import org.aspectj.bridge.IMessage; +import org.aspectj.bridge.ISourceLocation; import org.aspectj.bridge.Message; import org.aspectj.weaver.patterns.DeclareErrorOrWarning; import org.aspectj.weaver.patterns.PerClause; @@ -49,7 +50,7 @@ public boolean match(Shadow shadow, World world) { if (super.match(shadow, world)) { world.getMessageHandler().handleMessage( - new Message(msg, + new Message(format(msg,shadow), isError ? IMessage.ERROR : IMessage.WARNING, null, shadow.getSourceLocation())); @@ -63,5 +64,85 @@ } public Collection getThrownExceptions() { return Collections.EMPTY_LIST; } + +// %% inserts a % character +// %k inserts the joinpoint kind +// %s inserts the signature at the joinpoint +// %t inserts the name of the declaring type at the joinpoint +// %i inserts the name of the identifier at the joinpoint +// %j inserts a string representation of the joinpoint +// %J inserts an extended string representation of the joinpoint +// %f inserts the filename where the match was found +// %l inserts the line number where the match was found +// %a inserts the name of the aspect where the declare was defined + private String format(String msg, Shadow shadow) { + StringBuffer ret = new StringBuffer(); + for(int i = 0; i < msg.length(); i++) { + if (msg.charAt(i) != '%') { + ret.append(msg.charAt(i)); + } else { + // its a substitution character + int subCharPos = i+1; + if (subCharPos < msg.length()) { + i++; // consume it + substitute(ret,shadow,msg.charAt(i)); + } + } + } + return ret.toString(); + } + + /** + * @param buf the buffer in which to insert the substitution + * @param shadow shadow from which to draw context info + * @param c the substitution character + */ + private void substitute(StringBuffer ret, Shadow shadow, char c) { + ISourceLocation loc; + switch(c) { + case '%': + ret.append('%'); + break; + case 'k': // kind + ret.append(shadow.getKind().getName()); + break; + case 's': + ret.append(shadow.getSignature()); + break; + case 't': + ret.append(shadow.getEnclosingType()); + break; + case 'i': + ret.append(shadow.getSignature().getName()); + break; + case 'j': + case 'J': + ret.append(shadow.toString()); + break; + case 'f': // file name + loc = shadow.getSourceLocation(); + if ((loc != null) && (loc.getSourceFile() != null)) { + ret.append(loc.getSourceFile().toString()); + } + break; + case 'l': // line number + loc = shadow.getSourceLocation(); + if (loc != null) { + ret.append(loc.getLine()); + } + break; + case 'a': // aspect file and line number + loc = getSourceLocation(); + if ((loc != null) && (loc.getSourceFile() != null)) { + ret.append(loc.getSourceFile().toString()); + ret.append(':'); + ret.append(loc.getLine()); + } + break; + default: // unknown substitution character, leave alone + ret.append('%'); + ret.append(c); + } + } }
A small variation on this proposal would be to use the style of java.text.MessageFormat and longer names that closely match our existing thisJoinPoint-based reflection API, something like: declare warning : call(* Hello.*(..)) : "Illegal call to {signature.declaringType}.{signature.name}(..)"; or declare warning : call(* Hello.*(..)) : "Illegal call to {signature.shortString}";
more radical design that would directly use the thisJoinPoint reflection API, something like this: declare warning: call(* Hello.*(..)) { Signature sig = thisJoinPointStaticPart.getSignature(); return "Illegal call to " + sig.getDeclaringType().getName() + "." + sig.getName() + "(..)"; } As you can easily tell from the example, the purpose of this change is to make this kind of simple example smaller and easier to read <smile>. Okay, the real goal is to provide a lot more flexibility to the programmer. The open question is whether or not that flexibility is worth the increased complexity both of implementation and use of the feature. Here are two examples that show interesting things that could be done if the code can decide whether or not to show an error, not just how to format the message. declare warning: execution(* *(..)) { Signature sig = thisJoinPointStaticPart.getSignature(); if (Character.isUpper(sig.getName().charAt(0))) { return "Java method names should begin with a lower case letter"; } else { return NO_MESSAGE; } } declare warning: execution(boolean equals(Object)) { Class c = thisJoinPointStaticPart.getSignature().getDeclaringType(); if (c.getDeclaredMethod("hashCode", new Class[0]) == null) { return "should override hashCode whenever overriding equals"; } else { return NO_MESSAGE; } } There's a convincing case to be made that this design would greatly increase the power of declare error/warning. I think that for the user this additional power is easily worth the added complexity for the simple cases. I also think that we could make some minor tweaks to the design to improve these simple cases a lot. The big question is what are the consequences of having user code run at compile/weave time? How hard will it be for programmers to understand this code? How much more complicated will the implementation be? I'm going to address the issue of implementation complexity below by sketching what an implementation might look like. The block of code for the declare warning will be compiled into a special class. At weave time that class will be loaded by a custom ClassLoader. That ClassLoader will provide a sandbox for the code to run in to limit its capabilities. The exact contents of this sandbox will take some thought. Should be visible: java.lang: String, StringBuffer java.util.regex: * java.text: * org.aspectj.lang: JoinPoint.StaticPart, Signature, SourceLocation Must not be visible: Anything that can interact with the outside world Need to be faked: Class, Field, Method and Constructor These four classes would need to be replaced with fakes, maybe in org.aspectj.reflect. The fakes would have almost all of the methods of the originals, but methods like invoke and newInstance would be removed. All references to the original classes would be replaced with these fakes. Minor Issues: How can we make this less cumbersome for simple cases? Can we solve the thisJoinPointStaticPart is much too long problem? What’s the best way to signal no message? Is there a good way to label this as an experimental feature in 1.2 to reduce the pressure to get the design just right the first time? Will people get more confused by if? Note: There is an even more powerful proposal along these lines from Pengcheng Wu and Karl Lieberherr. I don't currently have a good reference to their work, but it extends both the complexity and the power of the proposal above.
This is an interesting area to pursue for the tool AspectJ. However, all of the compelling examples are about duplicating the abilities of tools like CheckStyle (http://checkstyle.sourceforge.net). They are not about AspectJ's rasion d'etre of providing better support for cross-cutting modularity. With that in mind, we will consider a simple solution that will satisfy the greatest need for this feature. The weaver will be modified very slightly to display the string representation of the join point shadow that is matched in addition to the string specified by the user. This may be more verbose than a programmer specified string might be but it will provide all of the available information without requiring any changes to the language or standard library. In fact, this information is already being passed to the Message object created by org.aspectj.weaver.Checker as "details". The only issue seems to be that these details are not being printed. This distinction between message and details is useful as it will allow IDE's to choose not to display this information all the time when their navigation features make it much less useful. However, these details need to be enabled by default for command-line uses of the compiler/weaver. This is being raised to a P2 enhancement request, where the feature to add is simply displaying the details for Messages when running on the command-line. We will revisit this proposal in the future if this small change doesn't satisfy users.
*** Bug 49328 has been marked as a duplicate of this bug. ***
*** Bug 48796 has been marked as a duplicate of this bug. ***
More on four bugs/discussions to incorporate into this: Two are marked as duplicates. Typically, duplicates can be ignored as redundant, but in this case duplicates are being incorporated for assessment into a master bug. (That can also be done by using a bug dependency and optionally marking the depending bugs as LATER.) - Bug 49328 is Pengcheng Wu's proposal for staticly-executable advice, available from http://www.ccs.neu.edu/research/demeter/sea - Bug 48796 is to permit compile-time string concatenation in declare error/warning strings. Two others are also relevant: - Bug 31724 notes that we don't supply the source context for these messages (like compiler-generated messages) like we used to in 1.0. That issue is deferred pending some implementation providing available source context to the weaver. - An RFE (I can't find) says the binary or source location of the declare error/warning should be emitted along with the location of the offending join point shadow, so that users can trace back to (and fix) the declare statement.
Fixed in tree. Sample output for a declare error (this one against binary source in a jar file cdd.jar) is as follows: D.java:6 error can only call bad from C (no source information available) method-call(void C.bad()) see also: C:\ColyerRoot\...\tests\errors\amctemp\A.java:5 see also: C:\ColyerRoot\...\tests\errors\amctemp\cdd.jar
