Fix several bugs #212
Merged
Fix several bugs #212
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kriegaex
commented
Jan 17, 2023
It is unnecessary to represent a pattern list 'A,B,C' as '(A,B,C)'. Not only does it look ugly in a type signature like 'org.acme.Foo<(A,B,C)>', but also is it not valid Java syntax. While the latter might not be strictly necessary in a String representation, it certainly is desirable, if such representations are ever used to generate code or @AspectJ pointcut annotations. Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
For array reference types, match type parameters on component type, not on array type itself. Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
In generic type lists, 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[]>>[]'
This is now fixed.
Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
Because void arrays are illegal (and nonsensical), now there is a new Xlint warning whenever World.resolve resolves a new 'void[]'. Because in the World class we do not have any source context, no path + line number are logged. The user only sees something like: [warning] arrays cannot have a void type, but found 'void[]' in pointcut [Xlint:arrayCannotBeVoid] Then later, if due to the returned MissingResolvedTypeWithKnownSignature type a joinpoint does not match, there is an additional my/path/MyAspect.aj:42 [warning] advice defined in MyAspect has not been applied [Xlint:adviceDidNotMatch] log line, but not necessarily anywhere near the former one. On the one hand, this is better than nothing. OTOH, comparing the situation with no logging message other than Xlint:adviceDidNotMatch in case of something equally illegal like 'Foo<int>' (primitive generic type parameter), this is actually more than we have in several other situations and might even be regarded as superfluous. In case of multiple 'void[]' cases within a big number of aspects, the same aspect or even the same pointcut, the user would have no clue where exactly to search for it. He would just see multiple log messages without source context. One option would be to set 'arrayCannotBeVoid=ignore' in XlintDefault.properties, so the user would have to explicitly activate it. But IMO, this message should be visible by default. Another option would be to find out how to defer logging the messages until later similarly to BcelWeaver.warnOnUnmatchedAdvice and then to bulk-print them. But in order to achieve that, the information about the existence of any 'void[]' occurrences would have to be stored in a flag similar to BcelAdvice.hasMatchedAtLeastOnce, bloating BcelAdvice for that rare case. Alternatively, each advice pointcut could be heuristically scanned for the literal substring 'void[]', logging the Xlint message if it is found anywhere. Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
The test needs to expect the lately introduced Xlint:arrayCannotBeVoid warning to be thrown, because one of the pointcuts in tests/bugs165/pr272233/Iffy2.java contains a 'void[]' return type in a method signature. Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
- Bugfix: Flush stream. - Adjust format to more closely resemble JVM format. E.g., do not print the causing exception name twice. - Add TODO, because this whole custom stack trace printing can just go away. The JVM format should do just fine. This commit is merely meant to document the decision to remove the cruft in the next commit. Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
- Improvement: Also add CompilationAndWeavingContext for constructor with causing exception - Remove home-brew stack trace printing, just call super constructors Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
Add more funky pointcuts concerning 'void[]' and pointcuts matching arrays of generic types. Remove TODO after previously committed bugfix. Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR should, step by step, fix all problems mentioned in #211, not just the original one, but also the other ones mentioned in comments. The plan is to push and build commit by commit in order to find out if the bugfixes actually break existing tests which falsely expect faulty behaviour.
I might force-push commits, adding other tests or editing commit messages, referring to future issues I might open in order to split the catch-all issue #211 into distinct issues per topic, which later we can also refer to from release notes.