Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discrepancy in parser metadata handling between LS and GUI #6718

Closed
mwu-tow opened this issue May 17, 2023 · 7 comments
Closed

Discrepancy in parser metadata handling between LS and GUI #6718

mwu-tow opened this issue May 17, 2023 · 7 comments

Comments

@mwu-tow
Copy link
Contributor

mwu-tow commented May 17, 2023

As I was looking into Undo/Redo issues, I observed that some operations are triggering code text edits even when there is no reason to.
One of the reasons seems to be a discrepancy between GUI and Engine in how they handle metadata.

Consider the following code from the initial project:

from Standard.Base import all
from Standard.Table import all
from Standard.Database import all
from Standard.AWS import all
import Standard.Visualization

main =
    operator1 = "Press TAB key to create a new node"



#### METADATA ####
[[{"index":{"value":5},"size":{"value":8}},"6ce0ea61-6e74-40b2-94f7-215b05fdd193"],[{"index":{"value":13},"size":{"value":1}},"39cbb9f1-2dca-4280-bc20-f2cfe5105ce0"],[{"index":{"value":14},"size":{"value":4}},"3276535b-b8b1-4743-96c2-f0c1860cc75f"],[{"index":{"value":5},"size":{"value":13}},"f313e5e6-b112-4e67-94f3-3824de0d5a70"],[{"index":{"value":0},"size":{"value":29}},"1cd3e872-18e4-4119-9ba3-8188e9e59fd2"],[{"index":{"value":35},"size":{"value":8}},"e301e5bd-48df-40b9-8e1c-696032ec984a"],[{"index":{"value":43},"size":{"value":1}},"3e954491-4634-4cc7-a914-2dc18638568a"],[{"index":{"value":44},"size":{"value":5}},"8a7318fd-8fa8-4a34-8262-2eec54c85b15"],[{"index":{"value":35},"size":{"value":14}},"bdacfdeb-0dea-4628-81a5-e1c2918f3e09"],[{"index":{"value":30},"size":{"value":30}},"0ecec61b-370c-41d8-a676-6ac9c0b3021a"],[{"index":{"value":66},"size":{"value":8}},"1d6c71ed-56b5-47ae-bdea-291e7f371b3e"],[{"index":{"value":74},"size":{"value":1}},"fe26e978-4753-4e1a-8c46-31fe56ceb854"],[{"index":{"value":75},"size":{"value":8}},"3e0aee4d-a459-4777-8d5e-6ca9cb997c3b"],[{"index":{"value":66},"size":{"value":17}},"6823563f-e35a-4e22-982e-ad23011d5a07"],[{"index":{"value":61},"size":{"value":33}},"1420cec5-4469-440e-8343-f051660c4025"],[{"index":{"value":100},"size":{"value":8}},"e61928aa-3a1d-4fc3-ba1e-469eedd5d7e4"],[{"index":{"value":108},"size":{"value":1}},"a2368807-7dc1-46c8-8260-b2555d8ab31c"],[{"index":{"value":109},"size":{"value":3}},"04567bf6-ef77-4b77-b488-601f88e9d854"],[{"index":{"value":100},"size":{"value":12}},"729304fd-a1ac-440a-b25e-1e9b1e1c30ee"],[{"index":{"value":95},"size":{"value":28}},"ec2670f3-c071-4702-9ee4-8fa2355ff53d"],[{"index":{"value":131},"size":{"value":8}},"43660892-5940-4233-8ce1-2d153da194b6"],[{"index":{"value":139},"size":{"value":1}},"cfe75ac9-95fb-4817-9c8a-cf714e7bf690"],[{"index":{"value":140},"size":{"value":13}},"402c84dd-7f3d-46e7-a092-639ab9c2e59c"],[{"index":{"value":131},"size":{"value":22}},"000667b6-49b8-41b2-8b70-af9ce048b13d"],[{"index":{"value":124},"size":{"value":29}},"2a41fd0c-8410-4679-8ebc-6ed049cd6976"],[{"index":{"value":155},"size":{"value":4}},"65f99db3-624f-45e6-afb1-10c60d701076"],[{"index":{"value":160},"size":{"value":1}},"aaea3452-8bae-4e59-9b50-d56353bacb0e"],[{"index":{"value":166},"size":{"value":9}},"283ba823-9ef2-4f46-9994-c23711f52f73"],[{"index":{"value":176},"size":{"value":1}},"5454f719-68c2-45c3-b13e-4cc3fad6ffb5"],[{"index":{"value":178},"size":{"value":36}},"93623c69-b912-41c2-a49d-cdaf4bf50de2"],[{"index":{"value":166},"size":{"value":48}},"f591f965-79aa-4009-87b5-5a3aa873c5f1"],[{"index":{"value":161},"size":{"value":53}},"82a89b53-3911-472e-9059-b24b67e5e014"],[{"index":{"value":155},"size":{"value":59}},"bdcbe47f-2531-44dc-a769-8f733f4b28cd"],[{"index":{"value":0},"size":{"value":215}},"82008f59-2c5c-45fd-a2f0-e85268502888"]]
{"ide":{"node":{"93623c69-b912-41c2-a49d-cdaf4bf50de2":{"position":{"vector":[-100.0,140.0]},"intended_method":null,"uploading_file":null,"selected":false,"visualization":null}},"import":{},"project":null}}

Note the last entry in the ID map is [{"index":{"value":0},"size":{"value":215}},"82008f59-2c5c-45fd-a2f0-e85268502888"]. It assigns an ID to the whole module's scope, likely corresponding to the root AST Module node.
However, when GUI parser it, the root Module node does not have an ID assigned to it. After looking into translation code it seems that it was intentional, as the new_no_id constructor is used. Thus, when GUI converts the AST back to text, the ID disappears, introducing a faux difference.

The problem is that the Language Server seems to be introducing this ID back on its own, causing some kind of edit war between the GUI and the Language Server and causing trouble for Undo/Redo functionality.

I suppose that this is caused by LS using a different parser API, not relying on Rust's AST translation code.

We should either:

  • make the GUI have missing IDs, assuming the root issues were addressed;
  • make sure that LS does not get any IDs that GUI does not see.
@kazcw
Copy link
Contributor

kazcw commented May 17, 2023

Module doesn't need to be constructed with new_no_id. It should use the same start_ast()/finish_ast() API as other nodes.

@kazcw
Copy link
Contributor

kazcw commented May 19, 2023

After speaking with @mwu-tow about this, I think the GUI is doing the right thing here. It is important not to assign IDs to two nodes that have the same span; a (very simple) module could have the same span as its child line. Since the IDE and the LS must be consistent, we should change this on the LS side.

@kazcw kazcw removed their assignment May 19, 2023
@Akirathan Akirathan removed the triage label May 19, 2023
@jdunkerley jdunkerley changed the title Discrepancy in parser metada handling between LS and GUI Discrepancy in parser metadata handling between LS and GUI May 23, 2023
@jdunkerley jdunkerley moved this from ❓New to 📤 Backlog in Issues Board May 23, 2023
mwu-tow added a commit that referenced this issue May 29, 2023
@jdunkerley jdunkerley added this to the Beta Release milestone May 30, 2023
mergify bot pushed a commit that referenced this issue Jun 12, 2023
This PR adds facilities for controllers to be aware of what shortcut command is currently being processed. This allows grouping consequences of single user action into a single transaction without hard-coding it separately for all the separate paths case-by-case, which turned out to be challenging and error-prone.

Additionally, a number of minor fixes were carried over from #6877:
* workaround for #6718;
* avoiding creating spurious transactions when dealing with node positions;
* dropping any non-user user-triggered transactions that occur during the IDE project initialization.
@jdunkerley jdunkerley assigned 4e6 and unassigned hubertp Jul 18, 2023
@4e6
Copy link
Contributor

4e6 commented Jul 25, 2023

The problem is that the Language Server seems to be introducing this ID back on its own, causing some kind of edit war between the GUI and the Language Server and causing trouble for Undo/Redo functionality.

The Language Server only reads and never updates the metadata. If it tries to do that, it will result in a checksum error after the following applyEdit command from IDE.

@JaroslavTulach
Copy link
Member

JaroslavTulach commented Jul 26, 2023

The problem is that the Language Server seems to be introducing this ID back on its own, causing some kind of edit war between the GUI and the Language Server and causing trouble for Undo/Redo functionality.

The Language Server only reads and never updates the metadata. If it tries to do that, it will result in a checksum error after the following applyEdit command from IDE.

Unrelated & colloquial: According to @kustosz, the motivation for using UUIDs was exactly to allow every party (including language server) to introduce new UUID. If that feature isn't used by language server, I could propose to stop using UUIDs again ;-)

@4e6
Copy link
Contributor

4e6 commented Jul 26, 2023

I could propose to stop using UUIDs again

I agree that using the path in the tree instead of UUID is a more elegant solution, but it will require work on both ide and the engine side in order to support it

@JaroslavTulach
Copy link
Member

@jdunkerley
Copy link
Member

Removing as obsolete.

@jdunkerley jdunkerley closed this as not planned Won't fix, can't repro, duplicate, stale Jul 2, 2024
@github-project-automation github-project-automation bot moved this from 📤 Backlog to 🟢 Accepted in Issues Board Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

7 participants