Review Request 118743: Make every QML/JS file see the other QML/JS files in its directory

Sat Jun 14 20:46:24 UTC 2014

> On June 14, 2014, 6:51 p.m., Sven Brauch wrote:
> > duchain/declarationbuilder.cpp, line 87
> > <https://git.reviewboard.kde.org/r/118743/diff/1/?file=281148#file281148line87>
> >
> >     Hm, this seems backward to me. I would prefer if you did it like "if a.qml is parsed, it looks for other files in the directory and imports them."
> >     
> >     For example, if you open all the files initially, will highlighting be correct (i.e. include highlighting for stuff from imported contexts) if they are parsed in the wrong order? I don't tihnk so, because you don't tell it to update the highlighting anywhere.
> 
> Denis Steckelmacher wrote:
>     ParseSession::contextOfFile returns the TopDUContext of a given file. If the file has not yet been parsed by KDevelop, then a new parse job is scheduled for it and for the current file. This way, the order in which files are parsed has no importance:
>     
>     * The first file to be parsed will schedule the parsing of all the other files, and will be parsed "as is", that is to say without any information about the QML components available in the current directory.
>     * Every file in the directory will be parsed. Each file will be able to immediately see the files that have already been parsed, and will make a link (in both directions) between it and the already-parsed files. This makes sense because the already-parsed files were not able to see this file and miss an import statement for it. Import statements are created only the first time a file is parsed, so there is no chance a duplicate is created. Every file (but the last one) will also be scheduled for a reparse, because it tried to import a file that has not yet been parsed.
>     * The last file that is explored is able to see all the others and is happy, and registers itself into all the other files of the directory. Now that every file has been parsed and has either been able to see some files, or was updated by other files when they were parsed, every file has an import statement for every other file.
>     * Every file (but the last one) now gets re-parsed, because some files were missing the first time they were parsed and they are now complete. QmlParseJob calls highlightDuchain, which gets the highlighting right (I think).
>     
>     I agree that it is somewhat complicated (in fact, I've thought about this problem for several days and it is difficult to have a code that is simple yet efficient). I'm not quite happy that most of the files of a directory need to be parsed two times (and each time requires two passes), but I think that it is the only solution: every file must be parsed a first time to build its TopDUContext, then these TopDUContexts can be added to all the other files that need to be reparsed now that new declarations become visible.
> 
> Sven Brauch wrote:
>     Hmm, so, why do you need to modify top-contexts other than the one you're currently building? Can't you just leave that out in that case? It should still work if each file just imports the already-built ones and then reschedules itself in case there's still missing files.
> 
> Denis Steckelmacher wrote:
>     Iterating over directories is quite expensive (at least a readdir() call, then potentially many stat() calls). I would like to avoid exploring directories every time a file is parsed (which can be quite frequent when the user is typing). The only way to avoid that is to explore the current directory only the first time a file is parsed (when recompiling() returns false).
>     
>     The problem is that because its directory is explored only the first time a file is parsed, reparsing the file will not allow it to see the new files. This is because of that that new files add themselves to the top-context of already-parsed files: because already-parsed files will never re-check whether a new file has appeared.
> 
> Sven Brauch wrote:
>     To be honest it sounds quite fragile. But if you're optimistic that it works, you can try it if you like.
> 
> Denis Steckelmacher wrote:
>     I just remembered the main reason that made me do what I do (especially the insertion of the current context into all the other files): if the user creates a file, the already-opened QML files will not be notified that a new file has been created. Generally, when the user creates a file, he opens it in KDevelop shortly afterward. When this happen, KDevelop will parse the file, that will add its top-level context to all the already-opened files. This way, the already-opened files don't need to be notified when a file is created and opened.
>     
>     One thing to note, though: in most cases, when the user creates a file and writes code in it, the other files don't yet use any symbol of the new file. The already-opened files will therefore always be modified (and thus reparsed) in order to take advantage of the new file.
>     
>     This makes me think that your solution may well be valid and preferable: remove the !recompiling() test (a directory listing operation should not be so slow that it would hurt performance when done two times per second) and only import the top-context of known files, without adding the current context to theirs. This should simplify the code and maybe make what it does a bit clearer :-) .

Be aware that when you import a project, all files in that project are parsed anyways, open or not. That also holds for new files.

About re-parsing stuff when a new file is created I'm actually not sure, we'll need to investigate how that is usually done. It'd be useful for python too. Did you test whether C++ does it? (#include "foo.h", then create foo.h, does the error go away?)

- Sven

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

On June 14, 2014, 1 p.m., Denis Steckelmacher wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118743/
> -----------------------------------------------------------
> 
> (Updated June 14, 2014, 1 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdev-qmljs
> 
> 
> Description
> -------
> 
> If a directory contains a.js, b.js and c.js, a.js imports b.js and c.js, b.js imports a.js and c.js, and c.js imports a.js and b.js. This way, every file can see the declarations of all the other files. This brings two important features:
> 
> * The same code is used for QML files. Because this patch also declares the top-level component of a MyComponent.qml QML file as MyComponent, QML files can now instantiate the top-level component of other files. This is the "custom components" feature.
> * Javascript files generally don't contain import statements, and it is difficult to know which declarations should be visible in a file. With this patch, every file can see the others in the same directory. This is already quite useful because the majority of the Javascript developers put all their files in the same directory (mysite.js uses the jquery.js file stored along it).
> * QML files can see JS files and the other way around. This may be useful if an user writes a JS files designed to be used by a QML file (I think it is possible to share code between the QML and the Javascript worlds) because the QML components will be visible in the JS files (that can instantiate them), and the JS functions/objects can be used in QML slots.
> 
> This patch is a bit simpler than what I've imagined, but here are some remarks that I have about the code:
> 
> * I use QFileInfo because I've not found a static method that takes a file path and returns the path of its parent directory (a simple section('/', 0, -2) would have been enough if every operating system used / separators). QDirIterator is easy to use, but is it okay to use these classes in KDE code?
> * Every file includes every other file in the same directory. When there are many files in a directory, this can become quite huge, but KDevelop seems to handle imported parent contexts in a quite efficient manner.
> * I filter out the files in /kdevqmljssupport/, because they are big and numerous. I don't want all 24 of them to include all the others and to be parsed every time KDevelop starts.
> * The "class" of the top-level QML component of a file is not anonymous anymore but has a name, hence the small changes in the unit tests.
> 
> 
> Diffs
> -----
> 
>   codecompletion/tests/qmlcompletiontest.cpp 7e32a16 
>   duchain/declarationbuilder.h 2744ffe 
>   duchain/declarationbuilder.cpp 5a9b830 
>   duchain/parsesession.h 8afc145 
>   duchain/parsesession.cpp 3133c80 
>   duchain/tests/testdeclarations.cpp 51b90a5 
>   tests/files/plugins.qml 9b1c0d4 
>   tests/files/test.qml 4b08b47 
>   tests/files/types.js 2428b89 
>   tests/files/uses.js bfbd6fc 
> 
> Diff: https://git.reviewboard.kde.org/r/118743/diff/
> 
> 
> Testing
> -------
> 
> Two tests are added: one declares a JS variable in a file and uses it in another file (and checks that the use count is 1), the other instantiates the top-level QML component of "test.qml" in "plugins.qml" and checks that the uses are okay.
> 
> Some other unit tests needed to be changed because top-level QML components now have a class name. With these changes, the whole testsuite continues to pass and has not been slowed down by this patch.
> 
> 
> Thanks,
> 
> Denis Steckelmacher
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20140614/65cc253d/attachment-0001.html>