scip-ctags: Fix line number mismatch due to scopes

[mmanela]

Fixes PRIV-3054

scip-ctags is mis-attributing the some identifiers to the start of the scope versus the location of the identifier. I found this issue in both the C# and Python implementations of scip-ctags and saw two different reasons.

Quick Summary

1. Updated scip-ctags to emit line number based on ident_range instead of scope_range (all tests pass) (Need @varungandhi-src to validate if this is fundamentally wrong)
2. Update Python scip-tags.scm to mark the scope as the whole class declaration and not just the body (technically not required with # 1 above but seems semantically correct to say the class declaration line is part of the scope)
3. Add a test for typescript scip-tags
4. Update the C# test example to cover more cases with attributes

Details

Python

In python we have the @scope declared as the body of the class declaration, changing this to the whole declaration mitiages the issue

--- a/docker-images/syntax-highlighter/crates/syntax-analysis/queries/python/scip-tags.scm
+++ b/docker-images/syntax-highlighter/crates/syntax-analysis/queries/python/scip-tags.scm
@@ -8,7 +8,7 @@
 ; (import_statement name: (_) @descriptor.term)
 ; (import_from_statement name: (_) @descriptor.term)
 
-(class_definition name: (_) @descriptor.type @kind.class body: (_) @scope)
+(class_definition name: (_) @descriptor.type @kind.class body: (_) ) @scope

C#

However, for CSharp the issue is a bit different. In CSharp we are misattributing the line number whenever a class has an attirubte on it . For example

    [Flags]
    public class TheClass
    {
        public Clickable ClickAction { get; set; }
        public string Text { get; set; }
    }

This is because the class declaration in the tree sitter grammar for C# includes the attributes as part of the scope.

C# scip-tags.scm

(class_declaration name: (_) @descriptor.type @kind.class) @scope

C# grammar

 class_declaration: $ => seq(
      repeat($.attribute_list),
      repeat($.modifier),
      'class',
      field('name', $.identifier),
      field('type_parameters', optional($.type_parameter_list)),
      field('bases', optional($.base_list)),
      repeat($.type_parameter_constraints_clause),
      field('body', $.declaration_list),
      $._opt_semi,
    ),

There for I think in our scip-ctags logic, we should not use scope.scope_range.start_line and instead use scope.ident_range.start_line since in for scip_ctags we care about the identifier location and not the start of the scope.

--- a/docker-images/syntax-highlighter/crates/syntax-analysis/src/ctags.rs
+++ b/docker-images/syntax-highlighter/crates/syntax-analysis/src/ctags.rs
@@ -82,7 +82,7 @@ impl<'a> Reply<'a> {
             name,
             path,
             language,
-            line: scope.scope_range.start_line as usize + 1,
+            line: scope.ident_range.start_line as usize + 1,

Test plan

• Unit tests updated
• Manual testing locally

[jtibshirani]

This makes sense to me. We are emitting the lines for identifiers, so it's appropriate to use the identifier range. And I think the Python query was just an oversight on my part, the scope should indeed be the class definition.

I'm not an expert here and it'd be great to get @varungandhi-src 's thoughts too !

[varungandhi-src]

The @scope marking change is consistent with how we're handling it for other types, so that's OK. Unfortunately, the terminology scope itself here is somewhat misleading/not appropriate (it should probably be something like declcontainer or similar, I have not looked too closely at all usages). We also have @scope for locals queries, and it means something quite different there.

Using ident_range rather than scope_range looks correct. However, I noticed that there aren't any doc comments on the field names at the struct definition, it would be worthwhile to fix that.

The C# test looks OK but you could trim it down further. The larger the test case, the more someone needs to read or skim over when they're looking at it.

For these kinds of PRs, generally, it is helpful to follow a 2-commit pattern. First commit is you add a test case and update the snapshot output showing the wrong result. Second commit you tweak the code and update the snapshot again. This way the diff in the snapshot resulting from the diff in the code becomes immediately understandable.

As for formatting, we previously had a rustfmt.toml file but hadn't set up formatting using Bazel, I've opened a PR for that here. #62371

Not a fan of bundling several unrelated changes into the same PR. If you're having to write a summary with 3-4 unrelated points, that's a strong sign that the PR needs to be broken up. 👎🏽