qt/qtdeclarative.git/src/qmlls/qqmlcompletionsupport.cpp, branch wip/nativemenus Qt Declarative (Quick 2) qmlls: implement commitToBase workflow for lazy files 2024-05-23T11:28:15+00:00 Sami Shalayel sami.shalayel@qt.io 2024-05-13T14:40:01+00:00 13761ee3c7d6ecb756db490d8c1fa8980117dca4 It seems that the current lazy QmlFile implementation does not take into account the commitToBase workflow used in qmlls, which leads to crashes and weird behaviors. Change the logic in QDeferredFactory<QQmlJSScope> to allow a custom TypeReader: QQmlJSTypeReader always reads from disk which is bad because qmlls operates most of the time on in-memory files and introduces an unnecessary IO operation, and therefore a new TypeReader is introduced in this commit for files loaded in the Dom. Populate lazy QmlFiles into the environment that they were loaded into with the new TypeReader before commitToBase() calls, and into the base environment after the commitToBase() call. This fixes the crashes during completion where a heap-use-after-free happened when a lazy file tried to populate itself after a commitToBase call using a destructed temporary DomEnvironment. Add a resetFactory() method to QDeferredSharedPtr to force recompute a QQmlJSScope when a file was changed without having to recreate all the qqmljsscopes for the other files, and use it also during commitToBase() calls to update the environment needed for the lazy loading. If the previous factory was used already, then the qqmljsscope is eagerly populated to avoid stale data. Also make sure to update the loadPaths for the build directory tests when getting a SemanticAnalysis struct from the base environment. Recreate the "completion crash" scenario from QTBUG-124799 in tst_qmldomitem. Add tests to make sure that files are correctly populated before and after commitToBase() calls. Also add a test in tst_qmlls_qqmlcodemodel to make sure that files are correctly lazy loaded in qqmlcodemodel, and simplify qqmlcompletionsupport's completions method by early exiting. Make populateFromQmlFile populate the logger in the same way as the QQmlJSTypeReader does. Fixes: QTBUG-124799 Fixes: QTBUG-125388 Change-Id: Id8bd7a1cbe60ba961eadb37b121f8f0670b863f4 Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io> Reviewed-by: Ulf Hermann <ulf.hermann@qt.io> 
It seems that the current lazy QmlFile implementation does not take into
account the commitToBase workflow used in qmlls, which leads to crashes
and weird behaviors.

Change the logic in QDeferredFactory<QQmlJSScope> to allow a custom
TypeReader: QQmlJSTypeReader always reads from disk which is bad
because qmlls operates most of the time on in-memory files and
introduces an unnecessary IO operation, and therefore a new TypeReader
is introduced in this commit for files loaded in the Dom.

Populate lazy QmlFiles into the environment that they were loaded into
with the new TypeReader before commitToBase() calls, and into the base
environment after the commitToBase() call. This fixes the crashes during
completion where a heap-use-after-free happened when a lazy file tried
to populate itself after a commitToBase call using a destructed
temporary DomEnvironment.

Add a resetFactory() method to QDeferredSharedPtr to force
recompute a QQmlJSScope when a file was changed without having to
recreate all the qqmljsscopes for the other files, and use it also
during commitToBase() calls to update the environment needed for the
lazy loading. If the previous factory was used already, then
the qqmljsscope is eagerly populated to avoid stale data.

Also make sure to update the loadPaths for the build directory tests
when getting a SemanticAnalysis struct from the base environment.

Recreate the "completion crash" scenario from QTBUG-124799 in
tst_qmldomitem. Add tests to make sure that files are correctly
populated before and after commitToBase() calls.

Also add a test in tst_qmlls_qqmlcodemodel to make sure that files are
correctly lazy loaded in qqmlcodemodel, and simplify
qqmlcompletionsupport's completions method by early exiting.

Make populateFromQmlFile populate the logger in the same way as the
QQmlJSTypeReader does.

Fixes: QTBUG-124799
Fixes: QTBUG-125388
Change-Id: Id8bd7a1cbe60ba961eadb37b121f8f0670b863f4
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>
dom: use only one DomCreationOptions per DomEnvironment 2024-03-14T10:41:58+00:00 Sami Shalayel sami.shalayel@qt.io 2024-02-06T10:29:28+00:00 9d3126b2eec6e9503cc4dc876ee9885de619e375 Currently, a DomEnvironment could load QML files with different DomCreationOptions. This makes things more complicated when loading dependencies, for example the implicit directory import might load a qmldir file that imports another module that contains a certain QML file, and then one has to remember which file triggered all of this dependency loading and what DomCreationOptions it had. In addition, there is currently no use case for something that complicated. Instead of that, move the DomCreationOptions to the DomEnvironment itself. When a DomEnvironment is used to load a file and its dependencies, it will use the DomCreationOptions set on this DomEnvironment. This ensures that all dependencies are loaded with the correct DomCreationOptions, and avoids weird bugs in qmlls features due to wrong DomCreationOptions used for dependencies. Remove the DomCreationOptions from the FileToLoad struct, adapt the DomEnvironment constructor and static factory method to expect a DomCreationOptions parameter, and change all DomUniverse::load() methods to have an extra DomCreationOptions parameter. This commit will break the formatting tests so QSKIP them, at least until the lazy-loading is done. The reason for the breakage is that the Dom eagerly loads all QML files from imports, and that takes too much time or crashes because it loads them now with the correct DomCreationOptions instead of DomCreationOption::None. For example, the formatting tests will implicitly load nestedFunctions.qml via the implicit directory import with the DomCreationOption::WithScriptExpressions and that will crash because of the nested functions, see also QTBUG-122707. Therefore QSKIP the formatting tests. Also adapt the timeout for a test that imports QtQuick.Controls.Basic, as the Dom also eagerly loads all the QML files from an imported module, which takes much more time with the DomCreationOption::WithScriptExpressions. Task-number: QTBUG-122645 Change-Id: Icb7eb6d17df15fc6ea133e014c00153df5cd1492 Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io> Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org> 
Currently, a DomEnvironment could load QML files with different
DomCreationOptions. This makes things more complicated when loading
dependencies, for example the implicit directory import might load a
qmldir file that imports another module that contains a certain QML
file, and then one has to remember which file triggered all of this
dependency loading and what DomCreationOptions it had.
In addition, there is currently no use case for something that
complicated.

Instead of that, move the DomCreationOptions to the DomEnvironment
itself. When a DomEnvironment is used to load a file and its
dependencies, it will use the DomCreationOptions set on this
DomEnvironment.
This ensures that all dependencies are loaded with the correct
DomCreationOptions, and avoids weird bugs in qmlls features due to
wrong DomCreationOptions used for dependencies.

Remove the DomCreationOptions from the FileToLoad struct, adapt the
DomEnvironment constructor and static factory method to expect a
DomCreationOptions parameter, and change all DomUniverse::load()
methods to have an extra DomCreationOptions parameter.

This commit will break the formatting tests so QSKIP them, at least
until the lazy-loading is done. The reason for the breakage is that the
Dom eagerly loads all QML files from imports, and that takes too much
time or crashes because it loads them now with the correct
DomCreationOptions instead of DomCreationOption::None.

For example, the formatting tests will implicitly load
nestedFunctions.qml via the implicit directory import with the
DomCreationOption::WithScriptExpressions and that will crash because
of the nested functions, see also QTBUG-122707.
Therefore QSKIP the formatting tests.

Also adapt the timeout for a test that imports QtQuick.Controls.Basic,
as the Dom also eagerly loads all the QML files from an imported module,
which takes much more time with the
DomCreationOption::WithScriptExpressions.

Task-number: QTBUG-122645
Change-Id: Icb7eb6d17df15fc6ea133e014c00153df5cd1492
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Introduce plugin support for qmlls and qmlls quick plugin 2024-02-29T11:08:07+00:00 Sami Shalayel sami.shalayel@qt.io 2024-02-12T14:49:18+00:00 6cdf8b829051b548463b682e7ba7c2256758ea81 Introduce the plugin support for qmlls, and add an (empty) snippet qmlls plugin that will provide quick snippets in the future. Add the QQmlLSPlugin plugin interface, and a QmlLSQuickPlugin that implements it. Use QFactoryLoader to load the plugins. Move the quick snippet code from qqmllscompletion to the newly introduced quick qmlls plugin for snippet completion. Add a test in tst_qmlls_modules to make sure that the plugin is loaded correctly in qmlls. Fixes: QTBUG-119969 Change-Id: If9cfedfbf166aff0eca8fe7008b0564035cd0153 Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io> Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org> 
Introduce the plugin support for qmlls, and add an (empty) snippet qmlls
plugin that will provide quick snippets in the future.

Add the QQmlLSPlugin plugin interface, and a QmlLSQuickPlugin that
implements it. Use QFactoryLoader to load the plugins.

Move the quick snippet code from qqmllscompletion to the newly
introduced quick qmlls plugin for snippet completion.

Add a test in tst_qmlls_modules to make sure that the plugin is loaded
correctly in qmlls.

Fixes: QTBUG-119969
Change-Id: If9cfedfbf166aff0eca8fe7008b0564035cd0153
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
qmllsutils: split completion related features into own class 2024-02-29T11:08:07+00:00 Sami Shalayel sami.shalayel@qt.io 2024-02-12T14:41:57+00:00 b1f94cc4f8535c8fe04c49b55ed4a893c626bd13 If we want to have a separation between the qmlls code and the quick snippets, then the completion feature needs some state to hold the quick snippets into memory. This commit moves all the completion related features into its own QQmlLSCompletion class to prepare for that. No new code was added, only moved around (+ some fixes to make it compile again). Make some qqmllsutils static helper functions into static member to be able to use them from the new QQmlLSCompletion class. The methods of QQmlLSCompletion are not static, such that they can all access the completion state. The state will be added in a further commit. Adapt the qqmllscompletion module to use the new QQmlLSCompletion class. Task-number: QTBUG-119969 Change-Id: I379bfef4bed245243ad52a56d52eda062437b2f0 Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io> 
If we want to have a separation between the qmlls code and the quick
snippets, then the completion feature needs some state to hold the quick
snippets into memory.

This commit moves all the completion related features into its
own QQmlLSCompletion class to prepare for that. No new code was added,
only moved around (+ some fixes to make it compile again).

Make some qqmllsutils static helper functions into static member to be
able to use them from the new QQmlLSCompletion class.

The methods of QQmlLSCompletion are not static, such that they can all
access the completion state. The state will be added in a further
commit.

Adapt the qqmllscompletion module to use the new QQmlLSCompletion class.

Task-number: QTBUG-119969
Change-Id: I379bfef4bed245243ad52a56d52eda062437b2f0
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>
DOM refactoring. Remove LoadOptions 2024-01-30T16:14:09+00:00 Dmitrii Akshintsev dmitrii.akshintsev@qt.io 2024-01-24T13:39:30+00:00 37ef144704c09e23fd2edfd49daf86b368147d15 Only defaultLoad is being used, hence it doesn't make sense to have them at all at the current stage Task-number: QTBUG-119550 Change-Id: I761e7c55d6aaafa29d234ace4b4b65bcc46a9855 Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io> 
Only defaultLoad is being used, hence it doesn't make sense to have them
at all at the current stage

Task-number: QTBUG-119550
Change-Id: I761e7c55d6aaafa29d234ace4b4b65bcc46a9855
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>
DOM refactoring. Move loadPendingDependencies to DomEnvironment 2024-01-18T17:46:18+00:00 Dmitrii Akshintsev dmitrii.akshintsev@qt.io 2024-01-08T16:52:18+00:00 3805424abc7f61c3c3643d143d0cb8fdefbc5269 One of the goals of this refactoring is to move DomTop specific functionality from DomItem API to the corresponding DomEnvironment and DomUniverse. This commit moves loading of the dependencies (which are added to the pending load queue implicitly as part of the callbacks of the loadFile when DomEnv is created without the flag "NoDependencies") to the DomEnvironment interface Moreover, once work with dependencies is happening through the DomEnvironment explicitly, it allows us to get rid of "self" parameter. Task-number: QTBUG-119550 Change-Id: Ib3deb97eabe091ffded8c8c782ee08f8024468f2 Reviewed-by: Sami Shalayel <sami.shalayel@qt.io> 
One of the goals of this refactoring is to move DomTop specific
functionality from DomItem API to the corresponding DomEnvironment
and DomUniverse.

This commit moves loading of the dependencies (which are added to
the pending load queue implicitly as part of the callbacks of the
loadFile when DomEnv is created without the flag "NoDependencies") to
the DomEnvironment interface

Moreover, once work with dependencies is happening through the
DomEnvironment explicitly, it allows us to get rid of "self"
parameter.

Task-number: QTBUG-119550
Change-Id: Ib3deb97eabe091ffded8c8c782ee08f8024468f2
Reviewed-by: Sami Shalayel <sami.shalayel@qt.io>
DOM refactoring. Remove DomItem::loadItem. enable_shared_from_this for DomEnv* 2024-01-18T10:12:30+00:00 Dmitrii Akshintsev dmitrii.akshintsev@qt.io 2023-12-28T20:31:13+00:00 bb4391a93e2c50efabf837ac32712057063ce9db As it was mentioned in the previous commits in the chain one of the goals of the refactoring is to introduce clearer separation of concerns. Only DomTop types are allowed to load files, hence it's arguable should be reflected in the interfaces explicitly. With the help of previous commits the relevant pieces of this function were moved to the corresponding DomEnvironment and DomUniverse types. This commit finally removes loadFile from the DomItem API. *It's also worth noting that in order to minimaze refactoring efforts and to keep the public API simple (not requiring DomItem &self argument on the top level), this commit also makes DomEnvironment a child of the std::enable_shared_from_this. This somewhat "breaks" the initial design approach inscribed in the DOM API allowing DomEnvironemt to get a shared_ptr to itself bypassing corresponding DomItem.ownerAs<> API. However, in the tradeoff of preserving consistency of the previous design and the explicitness of the interface, I would value the latter, hence I find this change acceptable. Change-Id: Ic2ce3fc80876be6bdbec93e4d273e4c6b7a2b218 Task-number: QTBUG-119550 Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io> 
As it was mentioned in the previous commits in the chain one of the
goals of the refactoring is to introduce clearer separation of
concerns.
Only DomTop types are allowed to load files, hence it's arguable
should be reflected in the interfaces explicitly.
With the help of previous commits the relevant pieces of this
function were moved to the corresponding DomEnvironment and DomUniverse
types.
This commit finally removes loadFile from the DomItem API.

*It's also worth noting that in order to minimaze refactoring
efforts and to keep the public API simple (not requiring
DomItem &self argument on the top level), this commit also makes
DomEnvironment a child of the std::enable_shared_from_this.
This somewhat "breaks" the initial design approach inscribed
in the DOM API allowing DomEnvironemt to get a shared_ptr to itself
bypassing corresponding DomItem.ownerAs<> API.
However, in the tradeoff of preserving consistency of the previous
design and the explicitness of the interface, I would value the latter,
hence I find this change acceptable.

Change-Id: Ic2ce3fc80876be6bdbec93e4d273e4c6b7a2b218
Task-number: QTBUG-119550
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>
qmlls: insert semicolons behind dots as completion workaround 2024-01-16T07:07:18+00:00 Sami Shalayel sami.shalayel@qt.io 2023-12-21T10:04:19+00:00 d06ed7fdd8beec490e453ce493e53041f58a2107 IDE's usually ask completion requests whenever users starts typing a '.'. For this patch, we assume that the user is currently writing code, that the '.' he just entered is followed by a newline and that the line below the cursor is unrelated, which should be a pretty realistic scenario of writing code. For example, imagine a user typing in following code: ``` x: root.<current cursor> SomeQualifiedModule.Item {} ``` The LSP client will request completion suggestions when the user types in `root.` and qmlls will be confused about the completion, as it will see a QmlObject binding in the Dom representation. Another example are JS statements, where the parser stops working when he sees ``` root.<current cursor> for (;;) {} ``` because it tries to construct the function call `root.for` and has troubles with the semicolons inside the function call argument. To go around this problem, insert semicolons when the symbol that requested the completion is a dot and when this dot is followed by a newline. In qqmlcompletionsupport, create a new Dom representation with the patched code, and keep it out the QQmlCodeModel to avoid bothering other modules, like the linting module, with the inserted "invisible" semicolon (invisible in the eyes of the user). Now, when the parser is run again on the patched code, it will see ``` root.; ``` and will happily create the correct field member expression using its recovery mode. One could also have inserted any identifier like '_dummyIdentifier' followed by a semicolon to get: ``` root._dummyIdentifier; ``` which would have worked too I believe. Add two tests, one in tst_qmlls_modules to test if the patched Dom can be used to provide the completions, and on in tst_qmlls_utils to test if the "normal" dom construction works on (hand)patched versions of the files. tst_qmlls_utils cannot test the patching itself as that happens in the qqmlcompletionsupport module. The afterDots.qml files are invalid and as such should be ignored by tst_qmlformat. Pick-to: 6.7 Fixes: QTBUG-119839 Change-Id: Ib914b78512894c423792588842d635d3d1467922 Reviewed-by: Ulf Hermann <ulf.hermann@qt.io> 
IDE's usually ask completion requests whenever users starts typing a
'.'. For this patch, we assume that the user is currently writing code,
that the '.' he just entered is followed by a newline and that the line
below the cursor is unrelated, which should be a pretty realistic
scenario of writing code.

For example, imagine a user typing in following code:
```
x: root.<current cursor>
SomeQualifiedModule.Item {}
```
The LSP client will request completion suggestions when the user types
in `root.` and qmlls will be confused about the completion, as it will
see a QmlObject binding in the Dom representation.

Another example are JS statements, where the parser stops working when
he sees
```
root.<current cursor>
for (;;) {}
```
because it tries to construct the function call `root.for` and has
troubles with the semicolons inside the function call argument.

To go around this problem, insert semicolons when the symbol that
requested the completion is a dot and when this dot is followed by a
newline.
In qqmlcompletionsupport, create a new Dom representation with the
patched code, and keep it out the QQmlCodeModel to avoid bothering other
modules, like the linting module, with the inserted "invisible"
semicolon (invisible in the eyes of the user).

Now, when the parser is run again on the patched code, it will see
```
root.;
```
and will happily create the correct field member expression using its
recovery mode. One could also have inserted any identifier like
'_dummyIdentifier' followed by a semicolon to get:
```
root._dummyIdentifier;
```
which would have worked too I believe.

Add two tests, one in tst_qmlls_modules to test if the patched Dom can
be used to provide the completions, and on in tst_qmlls_utils to test if
the "normal" dom construction works on (hand)patched versions of the
files. tst_qmlls_utils cannot test the patching itself as that happens
in the qqmlcompletionsupport module.

The afterDots.qml files are invalid and as such should be ignored by
tst_qmlformat.

Pick-to: 6.7
Fixes: QTBUG-119839
Change-Id: Ib914b78512894c423792588842d635d3d1467922
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>
qmlls: move completion support to QQmlLSUtils 2023-09-26T18:46:38+00:00 Sami Shalayel sami.shalayel@qt.io 2023-09-07T13:31:11+00:00 0232b45cef0bef90ec4e24fb3a30ea2a3dafd09f Preparation works for using QQmlJSScope to power the qmlls completions. Move the completion support into QQmlLSUtils and out of qqmlcompletionsupport. This allows to test the completion support in tst_qmlls_utils without having to start a qmlls instance. Task-number: QTBUG-113727 Change-Id: I51977e29bf8c380b8e137fe9b187f28cc1472822 Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io> 
Preparation works for using QQmlJSScope to power the qmlls completions.

Move the completion support into QQmlLSUtils and out of
qqmlcompletionsupport.
This allows to test the completion support in tst_qmlls_utils without
having to start a qmlls instance.

Task-number: QTBUG-113727
Change-Id: I51977e29bf8c380b8e137fe9b187f28cc1472822
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>
qmldom: Fix coverity warnings 2023-09-15T13:32:40+00:00 Ulf Hermann ulf.hermann@qt.io 2023-09-13T15:36:59+00:00 6e5a342b1030e5df47e13917a8f488ffbce6add0 - initialize members - iterate const containers, and use const refs for elements - pass values by reference - provide move constructors Coverity-Id: 417104 Coverity-Id: 417102 Coverity-Id: 417101 Coverity-Id: 417100 Coverity-Id: 417099 Coverity-Id: 417098 Coverity-Id: 417097 Coverity-Id: 417096 Coverity-Id: 417095 Coverity-Id: 417094 Coverity-Id: 417093 Coverity-Id: 417092 Coverity-Id: 417090 Coverity-Id: 417088 Coverity-Id: 417087 Coverity-Id: 417086 Coverity-Id: 417085 Change-Id: I624ed4e71c49c3eebced9483dd73554dbdcb981b Reviewed-by: Sami Shalayel <sami.shalayel@qt.io> 
- initialize members
- iterate const containers, and use const refs for elements
- pass values by reference
- provide move constructors

Coverity-Id: 417104
Coverity-Id: 417102
Coverity-Id: 417101
Coverity-Id: 417100
Coverity-Id: 417099
Coverity-Id: 417098
Coverity-Id: 417097
Coverity-Id: 417096
Coverity-Id: 417095
Coverity-Id: 417094
Coverity-Id: 417093
Coverity-Id: 417092
Coverity-Id: 417090
Coverity-Id: 417088
Coverity-Id: 417087
Coverity-Id: 417086
Coverity-Id: 417085
Change-Id: I624ed4e71c49c3eebced9483dd73554dbdcb981b
Reviewed-by: Sami Shalayel <sami.shalayel@qt.io>