Review Request 118483: Ensure that declarations returned by Utils::getDeclaration are visible in the context passed as parameter

[Denis Steckelmacher]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118483/
-----------------------------------------------------------

(Updated June 3, 2014, 9:04 a.m.)


Status
------

This change has been discarded.


Review request for KDevelop.


Repository: kdev-qmljs


Description
-------

This is a workaround for a behavior that I cannot completely explain. Here is a snippet of Javascript code that exhibits the behavior:

----
var a = {key: "value"};

key = 2;
----

Without my patch, "key = 2" is considered to be an use of a.key. However, the declaration of "key" is in a context that spans the space inside the brackets. Some debugging has even shown that "key" is not listed in DUContext::getDeclarations(), called on the context in which "key = 2" appears (the top-level context):

----
looking for "key":
    <struct> a
 -> found "string key"
----

By looking at the code and running GDB, I've seen that when the context in which a declaration is being looked for is a TopDUContext, then the symbol table is used instead of the normal code path. The problem is that "key" appears in the symbol table (I think this is expected), but I don't want it to be "visible" from the top-level context.

This patch is therefore a mean to avoid using TopDUContext::getDeclarations. First, getLocalDeclarations is tried (this finds declaration in the current context and its parents, but not its imported parent contexts, if I've correctly understood how the thing works), then, and only if the context has imported parent contexts, getDeclarations is used.

This is quite a dirty solution. Is there a mean to prevent TopDUContext from short-circuiting the normal declaration filtering process and from returning declarations that should normally not be visible?


Diffs
-----

  duchain/helper.cpp 27f5dd4 

Diff: https://git.reviewboard.kde.org/r/118483/diff/


Testing
-------

The whole testsuite passes, but the patch seems a little fragile: the top-level context may also have imported parent contexts (if there are import statements, or in the future when Javascript require() statements will be supported), and so the second if may well be entered even if context is a TopDUContext. The patch therefore does not fix the bug in all cases.


Thanks,

Denis Steckelmacher

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20140603/25c42c88/attachment.html>