Bugzilla – Full Text Bug Listing
| Summary: | ContentAssist: Improve parent symbol resolution | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [WebTools] Java Server Faces | Reporter: | Matthias Fuessel <mat.fuessel> | ||||||
| Component: | Core | Assignee: | Ian Trimble <ian.trimble> | ||||||
| Status: | NEW --- | QA Contact: | |||||||
| Severity: | enhancement | ||||||||
| Priority: | P3 | CC: | cameron.bateman, carlin.rogers, raghunathan.srinivasan, xiaonan_jiang | ||||||
| Version: | 2.0 | Keywords: | helpwanted | ||||||
| Target Milestone: | Future | ||||||||
| Hardware: | PC | ||||||||
| OS: | Windows XP | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
|
Description
Matthias Fuessel
Created attachment 76035 [details]
patch to unify el parsing for ContentAssist and validation
This patch tries to base symbol resolution (for ContentAssist and other purposes) and validation on the same el parsing logic. Therefore, it extracts a new superclass "ASTTraverser" out of ASTSemanticValidator, on which both ASTSemanticValidator and the new ASTResolver - for resolving symbols in el expressions - are based. ContentAssistParser now uses ASTResolver.
The patch also makes parsing of "[]" operator recover from unknown/erroneous second parameter if first parameter is a Map. And finally, it make MemberOperator ("[]" and ".") do validation and result calculation at the same time, since it mainly does the same thing for both purposes.
(Getting the "usual" value type of a map by simply passing it an arbitrary string as key is a bit of a hack. Probably this can be done better...)
user-visible improvements are:
-ContentAssist benefits from typed lists/maps
-result of el parsing is consistent throughout jsf tooling
Improvement for development is that the complex logic of parsing el expressions has to be implemented/maintained in only *one* place.
The downside is that ASTTraverser still contains some validation-specific stuff, since in some places this would be separable only for the price of much more complexity.
Hi Matthias, This looks like a really useful enhancement! Has the patched code passed the regression test suite (particularly org.eclipse.jst.jsf.validation.el)? --Cam Sorry, I didn't manage to do this before my vacation. So this has not been regression-tested. There are also two known issues: CA on subproperties of *unknown* objects/properties gives all instance symbols instead of no proposals, and CA on *empty* or unmatched "[" bracket fails because JSP EL parser fails with ParserException. I will be able to spend some time on this about 5/6 weeks from now, but not earlier. Created attachment 83328 [details] updated version, handles someList[-1] correctly Some smaller changes, most notably List/array access with index < 0 is handled correctly (error, continue with index 0). This version passes validation.el tests. Unfortunately it brakes contentassist tests completely, because it is no longer possible to calculate some prefix for an el expression without a file containing this expression. This breaks only the tests, no real usecase. The (minor) problems mentioned in comment 3 still exist, the second one (CA on mismatched/empty "[" operator fails) occurring also with current, unmodified jsf tooling. A binary version containing all my changes can be downloaded for testing from http://www2.informatik.hu-berlin.de/~fuessel/eclipse/eclipse.html#jsf_ext Deferred due to lack of resources. For review. For review For review Deferred to next release due to lack of resources. The underlying code on which this patch is based has changed and hence requires more work to absorb this fix. We will revisit in the next release. We appreciate the code contribution and look forward to any help from the community on this bug. |