Basic Goto Declaration Support #22

ehedbor · 2024-05-13T13:56:00Z

Adds a first draft of goto declaration. Also adds support for multiple files and incremental parsing.

There are some known issues with the implementation:

Some language features like encapsulated packages and array indices in component expressions are not handled. These were not implemented due to lack of time.
Components' superclasses are ignored in nested component accesses (e.g in foo.bar, bar will not be found if it was declared in a superclass of whatever foo is). This is due to a bug that caused infinite recursion when the feature was enabled.
In case of a conflict, goto declaration will simply return the first declaration it finds. This may affect things like multiple inheritance (this begs the question: if multiple classes declare the same component, which one should be used?).
In the case of import aliases, it was debated if goto declaration should take the user to the alias or to the original symbol. Right now the algorithm does the latter.
We use LSP.LocationLinks to return the position of symbols we find. The advantage of this is that it allows one to separately specify both the declaration (VS Code shows this in tooltips) and the symbol being defined (VS Code highlights this when going to the declaration). However, we did not have time to define
Although it is enabled, goto definition is not supported. Known differences include import aliases (see 4) and inner/outer declarations.

Co-authored-by: PaddiM8 <hi@bakk.dev>

CLAassistant · 2024-05-13T13:56:07Z

All committers have signed the CLA.

AnHeuermann · 2024-05-14T08:55:33Z

In case of a conflict, goto declaration will simply return the first declaration it finds. This may affect things like multiple inheritance (this begs the question: if multiple classes declare the same component, which one should be used?).

The C++ language server extension from Microsoft will open a small view with a list of all definitions. We could at some point do something similar:

AnHeuermann

I'm not sure on what example to test this pull request. I did open the Modelica Standard Library directory /home/user/.openmodelica/libraries/Modelica 4.0.0+maint.om. I can see that the LS is detecting and loading all the mo files but I couldn't get the jump to definition part working.

Can you provide:

Unit tests for each new major class you added to the server part. Then we can check that they are working as expected and we don't break them in the future. For example one test for the incremental parsing. One test for the file detection part. One test for the jump to definition in a single file. One test for jump to definition with multiple (maybe two) files. Some parts you don't want to test can be mocked.
End to end tests for the client. Open a workspace containing some Modelica files or library and run jump to definition on one or two locations.

With these tests I might be able to see what I did wrong. Or maybe provide an example where you can demonstrate that your feature is indeed working.

And then this PR is very large. Ideally it is one pull request for one issue you are trying to solve. So for example the incremental parsing part and the file detection part are very nice. If you can split them in two separate PRs (and add tests) we can already merge them and you can concentrate on this large go to declaration feature.
If it's a lot of work to split this PR into three parts we can keep it this way.

Finally the end to end test is failing. No idea why. Looks like I broke that one on the main branch.

AnHeuermann · 2024-05-14T09:10:11Z

server/src/project/project.ts

+  readonly #parser: Parser;
+  readonly #libraries: ModelicaLibrary[];


I wasn't aware that ECMAScript private fields are a thing. Only shows that I'm not a JS/TS developer 😆

AnHeuermann · 2024-05-14T09:21:56Z

server/src/analysis/resolveReference.ts

+  const maybeVariable = reference.kind === "variable" || reference.kind === undefined;
+
+  if (maybeClass) {
+    // logger.debug("findReferenceInDocument: Checking if this node is a class...");


If the prints are not needed any more remove them.

AnHeuermann · 2024-05-14T09:34:32Z

server/src/analyzer.ts

  }

-  public analyze(document: TextDocument): LSP.Diagnostic[] {
-    logger.debug('analyze:');
+  public async loadLibrary(uri: LSP.URI, isWorkspace: boolean): Promise<void> {


Add documentation. It's one of the top-level functions run at initialization of the parser.
And will it load libraries / files if I add them after some time while the language server is already running?

AnHeuermann · 2024-05-14T09:40:31Z

server/src/project/document.ts

+    );
+  }
+
+  public async update(text: string, range?: LSP.Range): Promise<void> {


If I understand this correctly this is the incremental parsing part.
Is it possible to move this into a different PR to get one PR for one issue?

AnHeuermann · 2024-05-14T09:41:18Z

server/src/project/library.ts

+ * See the full OSMC Public License conditions for more details.
+ *
+ */
+


And this is probably the "handle-multiple-files" part. Can this become a separate PR as well?

AnHeuermann · 2024-05-14T09:48:07Z

server/src/util/index.ts

+    // the node pathToFileURL uses ':' anyways. Manually fix this here.
+    // This is a bit hacky but we should ideally only be working with the URIs from LSP anyways.
+    return uri.slice(0, 5) + uri.slice(5).replace(":", "%3A");
+}


I have

"files.insertFinalNewline": true, "files.trimTrailingWhitespace": true,

in my VS Code settings.json, so I always add new lines at the end of documents. If you are also using VS Code this can be usefull.

AnHeuermann · 2024-05-14T09:54:35Z

server/src/util/tree-sitter.ts

+ * @param node The node to check. Must be a declaration.
+ * @returns The identifiers.
+ */
+export function getDeclaredIdentifiers(node: SyntaxNode): string[] {


Maybe it's simpler to use queries at this point. See https://tree-sitter.github.io/tree-sitter/using-parsers#query-syntax

Co-authored-by: PaddiM8 <hi@bakk.dev>

ehedbor · 2024-05-21T15:16:52Z

Edit: added links to draft pull requests

We have more time to work on fixing the PR now. We plan on splitting things up into multiple pull requests as you requested. This means we'll have to basically manually rewrite the git history but it was a mess anyways so no harm there. Currently planning on the following:

Add Prettier/other config stuff (see Add prettier #23)
Multi-file support (see Add support for multiple files #24)
Incremental updates (see Add incremental parsing support #25)
Goto declaration (WIP)

adeas31 · 2024-05-22T11:24:31Z

.vscode/settings.json

-  ]
+  ],
+  "files.insertFinalNewline": true,
+  "files.trimTrailingWhitespace": true,


I think @AnHeuermann meant that you can add this to your VS code settings so the code you write is always formatted without trailing whitespaces. I don't think he meant to add these settings to the project.

Indeed 😆

Surely it would be a good idea to add these to the project settings anyways, right? Doing that would help to ensure a more consistent style across contributors.

Not everyone uses VS Code, so I usually don't add the .vscode directory. For a VS Code extension this might be different.
Both is fine with me.

PaddiM8 · 2024-06-17T16:03:18Z

Replaced by #26

ehedbor and others added 21 commits April 11, 2024 15:59

Analyze entire project on opening it

e23da49

Co-authored-by: PaddiM8 <hi@bakk.dev>

handle workspace events

e0cc38b

Fix text document modification detection

07bd76d

Co-authored-by: PaddiM8 <hi@bakk.dev>

Add caching for tree sitter analysis

380e65c

Add prettier config

683af77

Co-authored-by: PaddiM8 <hi@bakk.dev>

Steal representation of Modelica projects from OMFrontend

e6f5b3b

Finish resolve algorithm; integrate with analyzer and server

38da1aa

Co-authored-by: PaddiM8 <hi@bakk.dev>

Get outline working again; changes to uri handling

3646148

Co-authored-by: PaddiM8 <hi@bakk.dev>

Fix infinite recursion and declaration symbols not being found

7eb3727

Implement goto declaration/definition for locals

9dae5cb

Co-authored-by: PaddiM8 <hi@bakk.dev>

Minor tweaks to resolve algorithm

8795ed1

Co-authored-by: PaddiM8 <hi@bakk.dev>

Add new resolve algorithm

30cbf1d

Co-authored-by: PaddiM8 <hi@bakk.dev>

Fix resolution of types and locals

df86c40

Fix resolution of inherited members

c15d93f

Allow relative references in extends clauses

aaa6dbc

Bugfixes; use paths internally

6101ada

Co-authored-by: PaddiM8 <hi@bakk.dev>

Fix endless recursion bug in resolveReference

e015766

Co-authored-by: PaddiM8 <hi@bakk.dev>

Fix resolution algorithm

2d0cc19

Tweak resolution algorithm

b597f9a

Co-authored-by: PaddiM8 <hi@bakk.dev>

Reanalyse when a file is modified, before it's saved

de6798c

Incremental parsing

634fb3c

AnHeuermann self-requested a review May 13, 2024 14:23

Whitespace changes

cf6d053

AnHeuermann requested changes May 14, 2024

View reviewed changes

ehedbor and others added 2 commits May 21, 2024 14:41

Add additional documentation.

6a727b3

Co-authored-by: PaddiM8 <hi@bakk.dev>

update settings

38dc70e

ehedbor marked this pull request as draft May 21, 2024 14:49

adeas31 reviewed May 22, 2024

View reviewed changes

ehedbor closed this Jun 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic Goto Declaration Support #22

Basic Goto Declaration Support #22

ehedbor commented May 13, 2024

CLAassistant commented May 13, 2024 •

edited

Loading

AnHeuermann commented May 14, 2024

AnHeuermann left a comment

AnHeuermann May 14, 2024

AnHeuermann May 14, 2024 •

edited

Loading

AnHeuermann May 14, 2024

AnHeuermann May 14, 2024

AnHeuermann May 14, 2024

AnHeuermann May 14, 2024

AnHeuermann May 14, 2024

ehedbor commented May 21, 2024 •

edited

Loading

adeas31 May 22, 2024

AnHeuermann May 22, 2024

ehedbor May 22, 2024

AnHeuermann May 23, 2024

PaddiM8 commented Jun 17, 2024

		readonly #parser: Parser;
		readonly #libraries: ModelicaLibrary[];

Basic Goto Declaration Support #22

Basic Goto Declaration Support #22

Conversation

ehedbor commented May 13, 2024

CLAassistant commented May 13, 2024 • edited Loading

AnHeuermann commented May 14, 2024

AnHeuermann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnHeuermann May 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehedbor commented May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaddiM8 commented Jun 17, 2024

CLAassistant commented May 13, 2024 •

edited

Loading

AnHeuermann May 14, 2024 •

edited

Loading

ehedbor commented May 21, 2024 •

edited

Loading