Generate classes even after validation errors #1978
Conversation
7f5eab9 to
ce4702a
Compare
|
Hi, it seems the reason for the build fail it is that there's some Tests that requires that the process failed. Right now as explained in #1978 the process is failing in the last round... it seems this is not being detected... I've run several tests in projects and it seams to be working fine. Could you anyway please check why they are failing exactly?.. maybe it is something else. Thanks |
|
Thank you for testing and enhancing my PR. I'll try to test this on my project and check whats wrong with the tests as soon as possible. But currently I have a lot of projects for my job and in my free time. I hope I can get into it this weekend! :) |
|
I just closed my version of this PR as we should not have two PR for the same issue. Please ensure that you have checked the |
|
Yes, the option is checked.. Didn't know it existed, sorry maybe I was able to work in your PR. |
|
I think this option is fairly new. but you would not have been able to work in my PR as it requires write access to the androidannoations repo to get permission to write on PR branches when this option is checked. |
|
It looks like the problems with the test are related to the When its I'll see if i can find a way to keep this source state for the deffered adding of the erros that you implemented. But I'm afraid that this might limit our compiler support as the implementation of the |
|
Ok, I guess sacrificing the tests is not an option neither... I simply didn't find a way to keep the compiler working at least that the report was done in that last round... not sure if making it in the prev-last round would make difference, but that would be hard to detect from within the framework. I'm not sure of this, but isn't there a way to send information to the test from within the annotation processor?... of course that would require to change those tests... |
|
The problem is that we not only break the tests with this behaviour but also e.g. eclipse would no longer be able to show the error on the annotation as it currently is as it is missing the information about the source file that has the error. |
|
Making it like this for Android Studio only would work? The real problem that we are solving with this PR is exactly there. |
|
Well, as soon as IntelliJ / Android Studio starts to show errors on the source/annotation it would not be able to do this for AndroidAnnotations as the required information here is missing. :( |
|
Well, I guess if it is applied to Android Studio, it would be to IntellyJ in general.. not sure how to detect that exactly, but well, between eclipse and IntellyJ the biggest difference is the folder structure.. |
|
Mmm.. well, in that moment, this fixed is not needed anymore... right now IMHO, this is something serious, to be no able to detect the bugs, or have to make that process of watching the console, or going to the log file (I have it opened all the time in NotePad++ for instance) it is really annoying for programming... I really wouldn't like to omit this fix.. of course, maybe there's some other way to make it work.. |
|
Maybe not completely good idea, and it sounds more like a "patch" to this is 8000 sue... but well, it can be created a Flag to pass as parameter to the framework which activates this behavior.. |
|
It is even an issue in CLI builds as the javac does not know the files where the error is added. See this screenshot as an example. the blue line should be part of the |
well it would still be required as on bigger projects not generating source might again turn the whole project red and makes the source of the issue hard to find. |
|
Yes that's right.. but this was something that I didn't understand, but now that you mention it, maybe there's a simple solution to it.. in the code, what was done, it is that the error is reported, by a mechanism in which I don't see the Element.. This is the original code (which I didn't modify): for (Element annotatedElement : annotatedElements) {
ElementValidation elementValidation = annotationHandler.validate(annotatedElement);
AnnotationMirror annotationMirror = elementValidation.getAnnotationMirror();
for (ElementValidation.Error error : elementValidation.getErrors()) {
LOGGER.error(error.getMessage(), error.getElement(), annotationMirror);
}
for (String warning : elementValidation.getWarnings()) {
LOGGER.warn(warning, elementValidation.getElement(), elementValidation.getAnnotationMirror());
}
if (elementValidation.isValid()) {
validatedAnnotatedElements.add(annotatedElement);
} else {
LOGGER.warn("Element {} invalidated by {}", annotatedElement, annotationName);
}
}Maybe it would be enough if we instead of reporting all the errors separated and the warning separated, we could create a single String and reported in the last step with the major Level (so if there's Info and Warns, a Warn would be reported, if there's Error, then this is what would be reported). I will try this I will let you know what happens then. |
|
Mmm.. no, I see they are almost the same... the main difference is that the last one uses de annotated alement and a Warn, which is reported correctly... |
|
Ok, I got it completely now.. there's no easy work around.. since the JavaFileObject exists only in that round, probably you cannot reference it in a different round... But in general JavaFileObjects can be created and passed to other methods.. not sure if that could be a possible solution, I would have to see more of your research about it... Well, a different approach it is to report all as a Warning and fail in the last round (this will solve what you point of the place the Error is being generate).. But of course, it still doesn't solve what the Tests do.... |
|
Please note that I simply passed a reference to the Element and Annotation Mirror obtained in the first round, to the last one.... I'm just thinking if trying to create an an element in the last round pointing to the same place would make any difference... |
|
I do not think that creating a Element or something is a solution as we are in java compiler (java/ecj/whatever) internal api where the relevant stuff happens... :( |
well there is not much to know. I found that the Messager is |
|
unfortunately i haven't found a solution for this problem yet. :( |
|
I got here some good result with something, I will let you know if I can get more improvements ;)
|
|
What have you done to reach this? maybe I'll can try too |
|
I'm still testing this... what I did was to get the elements from the TypeElement: I use getElement to send the message private class Message {
Kind kind;
String message;
AnnotationMirror annotationMirror;
List <String> elements = new LinkedList<>();
Message(Kind kind, String message, Element element,
AnnotationMirror annotationMirror) {
super();
this.kind = kind;
this.message = message;
this.annotationMirror = annotationMirror;
Element enclosingElement = element;
do {
elements.add(0, enclosingElement.toString());
enclosingElement = enclosingElement.getEnclosingElement();
} while (!enclosingElement.getKind().equals(ElementKind.PACKAGE));
System.out.println("ELEMENTS: " + elements);
}
public Element getElement() {
Element element = processingEnv.getElementUtils().getTypeElement(elements.get(0));
elements.remove(0);
while (elements.size() > 0) {
for (Element elem : element.getEnclosedElements()) {
if (elem.toString().equals(elements.get(0))) {
System.out.println("FOUND: " + elem);
elements.remove(0);
element = elem;
break;
}
}
}
return element;
}
} |
|
The test is getting stuck.. I'm checking this:
To be honest I'm not sure why is needed the AnnotationMirror... well, I will try to generate it also from the element |
|
I think we could tackle this quite easily. Where we are depending on generated code, we should introduce a new check, if the generating element is valid. I think we have to do this for |
|
It is good idea, but I'm not sure to be honest if this will solve the initial problem better... I mean, if you make this validation this means that the activity will be not generated due to its dependency (a Bean) will not be generated. You could be using that Activity (or Fragment, or Service, or... ) from many other places, which will generate as well a chain of errors (this is what this PR was trying to reduce, the huge amount of errors generated while compiling)... I'm not saying it is incorrect, just that it would be nice to weight what is more important here... once again, the PR was to reduce the amount of errors and let the programmer to focus in a few of them... |
|
@smaugho the Activity itself will be generated. The code which injects the bean into the activity will not be generated in it. |
|
@smaugho again, if we introduce these checks, we do not leak error to the user, because the Activity will be generated. Only we do not generate the bean injection code inside. So there will be no |
|
Yep. I am wondering if there are other cases where on annotation is depending on the other? |
|
From AA, I don't remember another... @WonderCsabo are you implementing this already? It would be good to invalidate @bean when the "linked" @ebean validation failed... but I don't find a way to know this at priori just in the validation phase... at least that you pass the "validatedAnnotatedElements" as parameter to the validations in the Annotation Handler... Any better idea? |
|
I am experimenting with this, yeah. |
|
Ok, I guess you have it cracked :)... probably enough getting the validated elements from the AndroidAnnotationEnvironment.. |
|
@WonderCsabo you should have write access to the branch of this PR. |
|
@WonderCsabo Does your code work with incremental compilation? |
563c6ea to
1e2932c
Compare
|
@dodgex thanks, i did not know that, it is cool! |
|
@dodgex yes it works, i actually checked it, that is why i introduced the |
| return Kind.OTHER; | ||
| } | ||
|
|
||
| private class Message { |
There was a problem hiding this comment.
only if we pass the processingEnv as it is used in getElement()
| } | ||
| } | ||
|
|
||
| private class ElementDetails { |
| return null; | ||
| } | ||
|
|
||
| private Element getElement() { |
There was a problem hiding this comment.
This is a rather complicated method, should not we cache the result? Or this is called only once?
There was a problem hiding this comment.
I think we should move this to an utility method, this logic is too much for a getter.
There was a problem hiding this comment.
As far as i can see it is only called once when the appender is closed in the last round.
There was a problem hiding this comment.
what do you mean by move this to an utility method? If it is to much logic for a "getter" i'd just rename the method to something like findRelatedElement or so. :D
There was a problem hiding this comment.
Okay, we can leave this here.
|
Did we update all licences? :) |
|
I think all licenses are up to date. but I can check that when updating the PR. how to go with the |
In some cases, error messages were not cleared during last round. It is better to not use static fields to store these messages.
If some annotation depends on other generating annotations, we should validate if the other annotations are ok, otherwise we will generate not compilable code.
1e2932c to
0732dc3
Compare
|
You missed to update the licenses but i fixed that. Also I made the ElementDetails static. |
|
Finally merged. Let's hope it will not introduce problems. Thanks for your work on this, @smaugho ! |
|
You're are welcome! Would like to be able to contribute much more to AA... lacking of time :'( ... |
This PR includes changes applies by the PR 1977, plus some improvements.
see #1976