Community
Participate
Working Groups
Created attachment 175312 [details] a patch for the universal editor that will fix the non-termination problem We've written some language services that add markers to resources based on various kinds of analyses. All these services are triggered by IMP after a successful parse. The problem is that when the services add marker annotations to the resources, this triggers another parse by IMP, which triggers another call to these services, which changes the markers, which triggers another parse by IMP, ..., ad infinitum. I've traced this behavior in the run-time, and it seems that the AnnotationUpdater inner class in UniversalEditor is responsible for calling the parser on an annotation change. By not calling the parser if its simply a marker update, the problem is resolved. I will include a patch. Please review for unwanted side-effects? We've tested this code and it solves our problem.
Hmmm... the patch is incomplete - how is "isMarkerChange" computed?
Oops... never mind the previous comment...
I can see that the patch would fix the problem you encountered, but at the expense of disabling the very feature this mechanism was intended to provide. Also, I think there's a significant problem with your scenario: that "parsing" (i.e., what's done on behalf of the *editor*) creates resource markers. As I understand it, this is counter to what the Eclipse platform is set up to do. A given resource's markers should always be in sync with the resource's state, not with the unsaved contents of an editor buffer for that resource. Builders are generally the only entities that create markers, and they're oblivious to editors and annotations. Editors create annotations, and typically, they also create annotations corresponding to the markers on the corresponding resource. But back to the infinite-looping problem, which I think we can and should address (at least partially). If I remember correctly, the intent of AnnotationUpdater is to re-process the given source editor's contents precisely when *other* resources' markers changed. Remember that in languages that have cross-compilation-unit dependencies (which are very common), changes in one file's marker state could have an effect on another file's syntactic or semantic correctness. I.e., changing file A could fix a syntactic/semantic problem in file B. Now, if a source editor for file B is in sync with the corresponding resource, the annotations on source editor B are exactly the same as the markers on file B. Moreover, the builder (if it handles dependencies correctly) should update the markers for B properly. If, on the other hand, source editor B has unsaved changes, there could be annotations (errors/warnings) on B that don't correspond to any marker, since they reflect the difference between B's source-buffer contents and B's saved file state. Thus re-building won't affect these annotations. So, by reprocessing the editor's buffer in response to other resources' changes, the parser-produced annotations that correspond to the source editor buffer's state will be updated to reflect the new state of that buffer's dependents. FWIW, the JDT behaves this way, though I confess I haven't looked at precisely how it does this. What produces the infinite loop is that the bit of code in AnnotationUpdater.problemsChanged() that triggers reparsing the source buffer doesn't pay attention to what resources had marker changes. In particular, if instead it only triggered a re-parse when there were changes to at least one *other* resource, the process wouldn't loop infinitely. Comments? ========= BTW, it would be good to use dependency information to refine the conditions for reparsing. I.e., we shouldn't reparse an editor buffer because *some* other resource changed, but only if some *dependent's* resource changed. We have API related to the builder that allows the language-specific IDE to expose dependency information, but we don't have any such thing for the editor at this point.
I don't get it completely. I would think that updates to the *model* of a program could indeed trigger other features of the IDE, but I would not think that updates to the presentation of a file (i.e. the resource markers) should trigger a re-parse, and generate a fresh model of the program such that all other analyses are run again. Error markers are aggregated up the container hierarchy (i.e. class, file, package, project), for display purposes, but between resources I'm not sure if any information should be transferred between compilation units via the marker mechanism. The build mechanism and a system for managing models of compilation units would seem more fit for that (PDB). T But yes, indeed, if there would be a check for another update in another resource then this would fix the problem and I'd be happy. The current workaround is a bit expensive (you have to cache the byte array of the input or the extracted model to see if anything has changed). I'd expect other IMP clients to benefit a bit from a fix too because they can remove this kind of workaround.
Yes, IMP certainly supports the idea that clients can listen for updates to the *model* of some source text. The key question is whether such clients that listen to a *transient* model should be creating markers on persistent resources.
aha. thanks for that remark! Question: if we would have hooked these static analyses to the builder service, would the marking not have triggered another parse? In the meantime, I'll change our code to trigger on a build instead of a parse, which seems to be the best thing to do.
Ok. I'll get the aforementioned fix (not triggering a re-parse for marker changes to the file that the editor's visiting) in ASAP. At the same time, I think your scenario might suggest that we're missing some API. Namely, some clients just want to reprocess source files when they change, but for ease of use, they want IMP to handle the parsing and just present them with an updated model. [In fact, they might even like an model delta, so they can see precisely what changed in the model.] This still fits in with the builder mechanism (which is all about triggering activity in response to resource changes), but perhaps we can wrap it. We do provide the BuilderBase class (which could use some improvements), which helps with some of this, but perhaps a little more abstraction could be even more helpful? Ideas?
yes. there is currently no IMP-level extension point for the builder service, simply because there is a wizard that uses the eclipse core service directly. I'd propose to add a wrapper extension point with an IMP-ish name for starters. I'll get in touch when I've seen more of the BuilderBase class.
