Changes for the language server #254

alxkzmn · 2024-05-30T06:25:56Z

The following changes were necessary to integrate with the language server:

Usages tracking for SymTableEntry (and some WIP additional functionality in the Analyzer to record the usages during the analysis).
"Reverse" lookup by offset in SymTable
File reference in DebugSymRef changes from Rc to Arc because the language server is asynchronous.

alxkzmn · 2024-05-30T07:37:57Z

I see that some tests are failing due to the new usages field in the SymTableEntry, but I'd rather fix them only if the changes make it through the review 😃

src/compiler/semantic/analyser.rs

src/compiler/semantic/mod.rs

src/parser/ast/debug_sym_factory.rs

src/parser/ast/mod.rs

leolara · 2024-06-08T01:33:00Z

so, now the changes to the analyser are missing?

alxkzmn · 2024-06-08T06:36:42Z

@leolara maybe I misunderstood but I thought that the conclusion was to extract the usages in a separate pass and put them in a separate data structure. In this case it could all be done in the language server.

leolara · 2024-06-08T09:38:42Z

@alxkzmn That is not the conclusion: #254 (comment)

The conclusion is to remove the lines that check if the state symbol is there and create it if it is not. The state symbol will be there.

alxkzmn · 2024-06-10T03:41:04Z

@leolara to make sure I'm on the right track:

it's OK to process the usages while analyzing, all in single pass;
the usages should be recorded to a separate data structure.
Is this correct?

leolara · 2024-06-10T05:57:32Z

Yes, single pass, you don't need a separate data structure. What you had before is ok, just needs to assume that the states are already in the symbol table, because they should be. As I explained, we are already doing kind of two passes, one to collect the state symbols and then to analyse each state after. So when you are analysing a transition you already should have the state in the symbol table

alxkzmn · 2024-06-10T10:32:52Z

@leolara the PR is ready for review.

leolara · 2024-06-12T03:09:30Z

src/compiler/semantic/analyser.rs

@@ -62,7 +62,9 @@ impl Analyser {
                block,
            } => {
                let sym = SymTableEntry {
+                    id: id.name(),


@alxkzmn perhaps we could have a constructor new that always sets usages to vec![]

leolara · 2024-06-12T03:10:47Z

src/compiler/semantic/analyser.rs

-                .into_iter()
-                .for_each(|expr| self.analyse_expression(expr)),
+            Statement::SignalAssignment(_, ids, exprs) => {
+                self.extract_id_usages(&ids);


@alxkzmn wouldn't it be better to extract the usages after the semantic analysis? I mean swaping the calls to extract_id_usages and analyse_expression ?

I agree, at least it seems more intuitively to make more sense to analyse first and then extract the usages

The IDs weren't initially used (_), so shouldn't be any difference (only affects the order in which they are recorded). But I'll rename and move the method after the analysis, just in case.

src/compiler/semantic/analyser.rs

src/parser/ast/mod.rs

src/compiler/semantic/mod.rs

leolara · 2024-06-18T04:37:14Z

src/parser/ast/mod.rs

+
+    /// Returns the proximity score of the given offset to the debug symbol.
+    /// The proximity score is the sum of the distance from the start and end of the symbol.
+    /// If the offset is not within the symbol, -1 is returned.


The comment is no longer valid

leolara · 2024-06-18T04:46:54Z

I put all my ideas here together to avoid more cycles:

It seems update_usages is repeating what find_symbol does? Cannot be it rewritten using find_symbol?
Here we are reading and "adding" a symbol to update its usages. Wouldn't it be better to add a method add_usage for this, more readable and encapsulated (https://github.com/privacy-scaling-explorations/chiquito/pull/254/files#diff-feef6fedaf732a222ce51a0e815b9afe9d0386c1099a96d97cd3b6dca7ba8f60R291-R299)
In find_symbol_by_offset I think the if can be simplified by putting an and of the first two conditions. (https://github.com/privacy-scaling-explorations/chiquito/pull/254/files#diff-feef6fedaf732a222ce51a0e815b9afe9d0386c1099a96d97cd3b6dca7ba8f60R374-R376)
In check_usage_at the code can be simplified and more stateless by returning only the "closest" of the collected symbols. This can be done without a map. Keeping the closest found until now in a variable.
In find_symbol_by_offset as well the code can be simplified by using a variable to keep the closest found until now, and don't use a map.
extract_usages_recursively is better rename to extract_usages_expression and be called after the analysis like in the statements. This is more coherent with the current naming.
extract_usages_expression could call collect_id_usages([id]) but perhaps this is too nit pick.

leolara · 2024-06-18T11:31:54Z

src/compiler/semantic/mod.rs

+    /// ### Returns
+    /// The `SymTableEntry` that is closest to the offset.
+    pub fn find_symbol_by_offset(&self, filename: String, offset: usize) -> Option<SymTableEntry> {
+        let mut closest_symbol: Option<SymTableEntry> = None;


@alxkzmn I think it would be much more efficient and clear to store the closest_symbol_proximity in a variable instead of calling proximity score every time.

I initially wanted to do that but then refactored into calling the method for readability because it seemed like overoptimizing.

I think it also make it more readbale, also perhaps you could have an and in the two conditions, although I think that feature with if let is experimental.

But I also thought for a moment that proximity_score had a couple of loops inside so I thought it was more important.

leolara · 2024-06-19T03:23:29Z

It was aprove already, you can merge

Introduce usage tracking in the symbol table

20f12fd

alxkzmn requested a review from leolara May 30, 2024 06:26

leolara reviewed May 30, 2024

View reviewed changes

src/compiler/semantic/analyser.rs Outdated Show resolved Hide resolved

leolara reviewed May 30, 2024

View reviewed changes

src/compiler/semantic/mod.rs Outdated Show resolved Hide resolved

leolara reviewed May 30, 2024

View reviewed changes

src/parser/ast/debug_sym_factory.rs Show resolved Hide resolved

leolara reviewed May 30, 2024

View reviewed changes

src/parser/ast/mod.rs Show resolved Hide resolved

alxkzmn requested a review from leolara June 7, 2024 10:37

alxkzmn force-pushed the language-server branch from a3cecdd to 20f12fd Compare June 10, 2024 07:19

alxkzmn added 10 commits June 10, 2024 17:05

Improve signal and state reference analysis

27a3851

Fix test

917d0c3

Merge branch 'chiquito-2024' into language-server

e548dc1

Fix clippy hints

d28370e

Merge branch 'chiquito-2024' into language-server

ad8f20d

Fix merge clippy error

b504968

Remove debug_sym_ref getter from Identifiable

3badca0

Add new function docs

6ed2726

Add unit test for find_symbol_by_offset

772a6ff

Clarify update_usages docs

561333c

leolara reviewed Jun 12, 2024

View reviewed changes

src/compiler/semantic/analyser.rs Outdated Show resolved Hide resolved

leolara reviewed Jun 12, 2024

View reviewed changes

src/parser/ast/mod.rs Outdated Show resolved Hide resolved

leolara reviewed Jun 12, 2024

View reviewed changes

src/compiler/semantic/mod.rs Outdated Show resolved Hide resolved

Implement code review remarks

ddf9fa3

alxkzmn requested review from leolara and rutefig June 17, 2024 07:32

alxkzmn added 2 commits June 17, 2024 15:46

Merge branch 'chiquito-2024' into language-server

e7c5373

Fix merge

3fe45a9

leolara reviewed Jun 18, 2024

View reviewed changes

alxkzmn added 4 commits June 18, 2024 12:54

Always use the debug sym ref of ID for usages

fd759f0

Refactor according to code review remarks

fed9661

Move expression usage extraction after the expression rule application

a244a3b

Remove unused field, improve comments.

a07288b

alxkzmn requested a review from leolara June 18, 2024 09:47

leolara reviewed Jun 18, 2024

View reviewed changes

leolara approved these changes Jun 18, 2024

View reviewed changes

Optimize find_symbol_by_offset

c61bb6a

Optimize find_symbol_by_offset

8eeff76

alxkzmn merged commit b023b4e into chiquito-2024 Jun 19, 2024
4 checks passed

alxkzmn deleted the language-server branch June 19, 2024 04:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes for the language server #254

Changes for the language server #254

alxkzmn commented May 30, 2024

alxkzmn commented May 30, 2024 •

edited

Loading

leolara commented Jun 8, 2024

alxkzmn commented Jun 8, 2024

leolara commented Jun 8, 2024

alxkzmn commented Jun 10, 2024

leolara commented Jun 10, 2024

alxkzmn commented Jun 10, 2024

leolara Jun 12, 2024

leolara Jun 12, 2024

rutefig Jun 12, 2024

alxkzmn Jun 17, 2024

leolara Jun 18, 2024

leolara commented Jun 18, 2024

leolara Jun 18, 2024

alxkzmn Jun 18, 2024

leolara Jun 18, 2024

leolara commented Jun 19, 2024

Changes for the language server #254

Changes for the language server #254

Conversation

alxkzmn commented May 30, 2024

alxkzmn commented May 30, 2024 • edited Loading

leolara commented Jun 8, 2024

alxkzmn commented Jun 8, 2024

leolara commented Jun 8, 2024

alxkzmn commented Jun 10, 2024

leolara commented Jun 10, 2024

alxkzmn commented Jun 10, 2024

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

leolara commented Jun 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leolara commented Jun 19, 2024

alxkzmn commented May 30, 2024 •

edited

Loading