Community
Participate
Working Groups
Once we added the capability to do parameter annotation matching, the first thing people want to do is grab an annotated parameter out from 'anywhere' it appears in a parameter list at an execution/call join point. The rest of this bug poses lots of questions and has no real answers... If one your parameters is @NotNull, you might like to write something like this (we have seen people try this already, so it must be intuitive syntax): before(): execution(* *(..,@NotNull (*),..)) But we have never supported multiple uses of '..' and so the current way to achieve what you want is that you have to write multiple pointcuts for each position it might occur and each variation of surrounding parameters: before(): execution(* *(@NotNull (*),..)) before(): execution(* *(*,@NotNull (*),..)) before(): execution(* *(*,*,@NotNull (*),..)) before(): execution(* *(*,*,*,@NotNull (*),..)) etc not great and rather than doing everything in one match and advice call, you end up with a bunch of calls to the advice for a method with multiple @NotNull parameters. It would be nice to capture it in one construct. Suppose we allowed execution(* *(..,@NotNull (*),..)) and it meant as expected 'a method with an @NotNull marked parameter anywhere' (or more than one...!). What would the user do if this did match, would they use args() and expect one parameter to be pulled out, or would they use thisJoinPoint and expect to pull out just the subset of parameters that matched? Let's look at thisJoinPoint first: before(): execution(* *(..,@NotNull (*),..)) { MatchedArg[] relevantArgs = thisJoinPoint.getMatchedArgs(); } getMatchedArgs() could return the @NotNull marked arguments. The MatchedArg type is just a simple wrapper for argument position, name and actual value. Then the question would be what does getMatchedArgs() return if used at a regular match, does it just return the same as getArgs()? Probably (but arguments wrapped in MatchedArg instances). Q. Is it intuitive that the syntax (..,@NotNull (*),..) is possibly selecting multiple possible arguments and not just one? Q. Is it intuitive that 'matched args' are the ones picked out by the execution pointcut that uses double '..'? It is probably too disconnected - and becomes even less clear if multiple pointcuts are used together: execution(* *(..,@NotNull (*),..)) && execution(* (int,..)) - what is the matchedargs? To strengthen the link between was is being matched upon and what is available in the advice, perhaps we need a new pointcut designator: before(MatchedArg... args): @param(@NotNull) { } along with a variant for non annotation related matches (because there may be other cases of wanting to subset the parameters by matching on something other than annotations): before(MatchedArg... args): param(int *) { } But then the confusion is that @param() and param() use the same form as @args/args, @target/target and so they would be getting matched dynatmically rather than statically. When what we need here is a static match as you would get with execution()/call()... param-execution(@NotNull (*))? I am loathe to introduce new designators unless necessary. But I can't quite see a clean way to introduce subsetting of parameters from an execution() match.
This is hairy indeed. I will add my $0.02. If we can simplify things a bit, then we may not need a new PCD. So, why would anyone want to write this pointcut? execution(* *(..,@NotNull (*),..)) && execution(* (int,..)) Why not write this one instead? execution(* *(int,..,@NotNull (*),..)) Are they not equivalent? And if so, can the compiler just reject pointcuts that use 2 execution pcds together (or for that matter 2 call pcds)? So now, if the first pointcut is now illegal, then this statement makes more sense in the body of an advice: MatchedArg[] relevantArgs = thisJoinPoint.getMatchedArgs(); And I think this would address all of the other questions as well. Of course, this is assuming that it is possible (and a good idea) to outlaw the first kind of pointcut.
(In reply to comment #1) > I will add my $0.02. Me too. > Are they not equivalent? And if so, can the compiler just reject pointcuts > that use 2 execution pcds together (or for that matter 2 call pcds)? What I don't like about this is that this would be the first time ever (I think) that ajc would complain about combinations of pointcuts - and even ones that *are* actually valid combinations. Today you can even write get(* *) && set(* *) which makes no sense at all but ajc will happily accept it. I think that for those pointcuts you probably want to go away from a pattern-based syntax. Why not have something like anyArgs(@NotNull * *) which matches if there is an argument of any type and name, annotated with NotNull. Then one could have thisJoinPoint.getAnyArgs() return a list of those arguments that are matched. One could even bind this to a variable: before(List<Foo> a): anyArgs(a, @NotNull Foo *) {} "a" would then automatically get a list of all matched parameters. Note that this could also solve (and should!) the case where there are no annotations involved at all: before(List<Integer> intargs): anyArgs(intargs, int *) {} The problem described in this bug really has nothing to do with annotations at all. It's a general problem about matching arguments. It's just more common when people are using annotations.
> So, why would anyone want to write this pointcut? > > execution(* *(..,@NotNull (*),..)) && execution(* (int,..)) Never try and guess what the user might do - they'll do all sorts, and especially things you never planned for ;) Anyway, the pointcut may be built up in pieces from reference pointcuts defined in other files and so you might well end up with two executions() (for example) when eventually fully resolving the pointcut. > The problem described in this bug really has nothing to do with annotations at > all. It's a general problem about matching arguments. It's just more common > when people are using annotations. Indeed, as I eluded to when I mentioned @param and param in my first comment. > before(List<Foo> a): anyArgs(a, @NotNull Foo *) {} that is an interesting proposal. it has lost the positional information though, which might be considered important as your advice is going to now be dealing with a subset of the arguments at a join point. (for example, you might want to include it in a message - at least my initial use case would). My initial feeling is anyArgs has a funny name and specifying binding and matching as two separate parts of a designator is unusual - but if we chew on it a bit, maybe something will drop out.
(In reply to comment #3) > initial feeling is anyArgs has a funny name and specifying binding and > matching as two separate parts of a designator is unusual - but if we chew on > it a bit, maybe something will drop out. Sure, the name is certainly debatable ;-) Also I am aware of the fact that no other pointcuts has both a pattern and a variable binding (well, args sort of has but not in the same way). However, I think it's pretty much what you want: - you want to grab arguments from any position, - you may want to filter on annotation, name and type (also the contents of an annotation?), and - you want to bind these arguments to a variable. The point about position information is interesting, although I am not sure whether one really wants to access position information. I mean "anyArgs" explicitly says "at any position". If you were interested in the position would you then not rather specify the position by using a normal args pointcut? What use case(s) do you have? A solution to this may be to fill "null" into the list/array at positions that did not match. That way positions would be determined by the ordering of the list. (One may have to use a marker for "null" though, because one may want to match on null being passed into a method.)
My usual use case is what I'm thinking about where one or more of n-parameters supplied to a method are marked @NotNull. A piece of advice checking the marked parameters are not-null would reasonably be expected to be able to name the parameter at fault (and/or specify the position) - especially as there may be two parameters of the same type in the parameter set, both marked @NotNull. In this form: before(List<Foo> a): anyArgs(a, @NotNull Foo *) {} The Type for the match has been specified twice, that is also unusual as normally it goes from just matching: before(): somePcd(SomePattern); to binding: before(SomePattern x): somePcd(x) where the specification of the match has moved from pcd to the advice declaration. In anyArgs above we now have Foo in both places - doesn't look quite right. Although the example I'll show below has the typename in multiple places in the pointcut, hmmmmm I also thought, like Eric, about sparsely populating an array so that position can be determined that way but it seemed like a bit of a hack, making the user cope with nulls (or some kind of marker) in an array because we couldn't come up with a solution in the language. I keep thinking back to how close it is to working with most of the constructs we have. Users are writing execution(* *(..,@NotNull (*),..)) expecting it to work, which means if we could get it to work it is intuitive. I might be happy to say that the rule for now would be double dots are only allowed with one single pattern between them (generalising that later to support multiple perhaps). The binding value being Object... if no type name specified (*), or the the typename... if it is specified: before(Object... os): execution(* *(..,@NotNull (*),..)) && args(os); before(Foo... fs): execution(* *(..,Foo,..)) && args(fs); There is a slight leap there tho in that args() doesn't match the arguments of the execution() pcd, but is an argument subset because of the use of this structure (..,XXX,..) The additional information is then available from the runtime support: MatchedArg[] thisJoinPoint.getMatchedArgs() Seems to have the benefit that if you can just use the matched objects to 'do your thing' in the advice, then you don't have to mess about with the extra parameter metadata if you don't want to (parameter name, parameter position) and if you do need it you can retrieve it from the joinpoint object. (So we could store it internally in a packed form and only unpack on request) My guiding principles here are: - whats the simplest easiest and most intuitive way to support this right now, in a way that doesn't limit any future options we might want to support - can I avoid adding pcds - what will give optimal performance and certainly no new overhead for users not using this feature
Andy I very much like your latest proposal and it made me thinking whether one could not take the opportunity to fix two issues at once, the other one being bug #61568, which you may remember. That bug discusses execution of advice in the presence of another form of ambiguous bindings: after(A a, B b) returning: call(* foo(*,*)) && (args(b,a) || args(a,b)) { doIt(a,b); } The semantics that abc implements in this case (and which at least Jim Hugunin agreed on as being desirable) is that doIt(..) will be executed twice, once with the parameters in the original order and once with the order swapped. I think that it would be great if one could fix this issue and then give the other form of ambiguous bindings discussed here the same semantics, i.e. the proposed syntax would be: before(Object os): execution(* *(..,@NotNull (*),..)) && args(os); before(Foo fs): execution(* *(..,Foo,..)) && args(fs); Note the difference to your proposal: Where you had a list, I now have just a single object because the advice body is to executed multiple times, once for each match. In my opinion this would indeed follow the principle of least surprise. The only issue that then remains is to somehow expose the index to the user via thisJoinPoint. One proposal would be the following: before(Object t, Foo fs): execution(* *(..,Foo,..)) && args(fs) && target(t) { int[] thisJoinPoint.getArgIndices(); //prints "-1", meaning "undefined" because //the first argument "t" to the advice //has no argument index System.out.println(int[0]); //prints the index of whatever was //bound to fs System.out.println(int[1]); }
That is interesting. I *think* I like that: before(Object os): execution(* *(..,@NotNull (*),..)) && args(os); before(Foo fs): execution(* *(..,Foo,..)) && args(fs); I might go as far as changing args to also use the '..' notation, in this case, to make it clear. Since if you were skipping arguments with args normally you would need to indicate which you were interested in by using * and .. - and perhaps it better defines what args means when used standalone: before(Foo fs): args(..,fs,..) A Foo argument at any position at the join point. In implementation it would also mirror what you'd do if writing it by-hand without an aspect: public void someMethod(String s,@NotNull String t, @NotNull String u,String v) { checkNotNull(t); checkNotNull(u); ... } you probably wouldn't write checkNotNull(t,u); which is what we were kind of trying to get to happen by passing List<Foo> or Foo... to the advice. Mirroring what would be written by hand by making two advice calls gives me more confidence we are heading the right way. Although there will be two advice calls we can still be smart around where we invoke them to avoid duplicating anything that could be shared between the advice calls (aspect instance, etc). However, it would introduce the notion of the same advice matching twice at a join point, with different contextual information (similar to the args(a,b)||args(b,a) situation Eric mentions). I'm not sure how the infrastructure will cope with this and I'm constantly thinking about what will mean the least re-engineering to implement new features like this.
(In reply to comment #7) > I might go as far as changing args to also use the '..' notation, in this case, > to make it clear. Since if you were skipping arguments with args normally you > would need to indicate which you were interested in by using * and .. - and > perhaps it better defines what args means when used standalone: > > before(Foo fs): args(..,fs,..) That makes a lot of sense, yes. > Although > there will be two advice calls we can still be smart around where we invoke > them to avoid duplicating anything that could be shared between the advice > calls (aspect instance, etc). Yes there should just be a loop being generated to execute all possible valuations. Then common things like the return value of aspectOf() can be assigned before the loop. One would have to think about this JoinPoint object, but I guess there would have to be a different one for each valuation because of the index-retrieval issue. > However, it would introduce the notion of the same advice matching twice at a > join point, with different contextual information (similar to the > args(a,b)||args(b,a) situation Eric mentions). Conceptually I think this is very clean: You still execute the advice whenever the predicate induced by the pointcut matches. It's just that the pointcuts now got more expressive. > I'm not sure how the > infrastructure will cope with this and I'm constantly thinking about what will > mean the least re-engineering to implement new features like this. That's an issue of course. I encourage you to have a look at what abc does in the case of bug #61568 - maybe that's helpful. One idea that I also had was to "just" introduce a layer of indirection between the advice matching and the advice execution. For "normal pointcuts" this layer would just forward all the arguments from what was matched and for those variable argument pointcuts there would have to be some special dispatch logic, probably very similar to what Oege described in bug #61568.
Hello all, Is this still unsolved in the latest AspectJ? I am glad there is a workaround per https://bugs.eclipse.org/bugs/show_bug.cgi?id=233718#c0, but I am having to duplicate my 'around' advice once already, do not want to write it 2 x 6 (or whatever) times. ...Especially as each copy of the advice has to be 6 lines because the 'proceed' has to be in the advice itself, so it's (a) setup (b) proceed (c) teardown, copy-and-paste 12 times. I will probably workaround by iterating thisJoinPoint.getArgs; how optimized is that? For instance is it smart enough to just create me the getArgs object array (cheap-ish) and only do the expensive bits as a lazy operation if I want it (which I don't)?
> Is this still unsolved in the latest AspectJ? I am glad there is a workaround > per https://bugs.eclipse.org/bugs/show_bug.cgi?id=233718#c0, but I am having to > duplicate my 'around' advice once already, do not want to write it 2 x 6 (or > whatever) times. Yes, it is still unsolved. > ...Especially as each copy of the advice has to be 6 lines because the > 'proceed' has to be in the advice itself, so it's (a) setup (b) proceed (c) > teardown, copy-and-paste 12 times. > > I will probably workaround by iterating thisJoinPoint.getArgs; how optimized is > that? For instance is it smart enough to just create me the getArgs object > array (cheap-ish) and only do the expensive bits as a lazy operation if I want > it (which I don't)? getArgs() will return you the argument values - I'm not sure how that will enable you to discover which were annotated? For the annotations you need: ((MethodSignature)thisJoinPoint.getSignature()).getMethod() From the Method object you can call getParameterAnnotations().
> getArgs() will return you the argument values - I'm not sure how that will > enable you to discover which were annotated? > > For the annotations you need: > ((MethodSignature)thisJoinPoint.getSignature()).getMethod() > > From the Method object you can call getParameterAnnotations(). Thanks for the reply Andy - yeah, that's the approach I had arrived at. I mentioned getArgs just because I understood that thisJoinPoint was the slow part; I know that getSignature you can get through thisJoinPointStaticPart. I see several places warning that thisJoinPoint is slow without going into details. Does accessing getArgs alone incur the full cost? Just curious; I'll have to run my code through a profiler anyway.
> I see several places warning that thisJoinPoint is slow without going into > details. Does accessing getArgs alone incur the full cost? Just curious; I'll > have to run my code through a profiler anyway. Yes, you will incur the cost. All the state in thisJoinPointStaticPart is fixed at compile time, so built once in the static initializer for a class and then just referenced. The state in thisJoinPoint varies per join point (different argument values, different 'this', different 'target'), and so they are built each time advice is called. Currently they are implemented through a general approach that works at all join points - they are fully constructed then passed into the advice. it may be possible to make some of it lazy for some cases. Right now if you do anything that requires thisJoinPoint in your advice (as opposed to thisJoinPointStaticPart) you will suffer the cost of building them. I also looked at the code today (never actually looked at that bit before), I see each call to getArgs() will return you a new copy of the parameters too (ie. construct a new array and copy them in then return it to you) - so don't call getArgs more than once.
I understand the due diligence in making sure this gets done right, but gee whiz it has been over four years now since the discussion started. Can we still not get this in AspectJ. I'd really love this to be possible without sextupling the size of my already-crazy-long pointcut...
Hi Nicholas, I'm afraid the team just isn't finding the time to implement such a sizable change. Whilst there is a workaround (although annoying, as you have discovered), we have to spend our limited resource fixing the more serious issues that cannot be worked around. Sorry. There are few bugs in this situation.
unsetting the target field which is currently set for something already released
This would be a useful feature to have.
