grafana / grafana/jsonnet-language-server

Variable shadowing not handled correctly

Open
#207 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
223
Forks
33
PR merge metrics
No merged PRs in 30d

Description

When function parameters and comprehension variables have names colliding with variables defined in some outer scope, go to definitions and find references produce wrong results.

The following patch adds two test cases illustrating the issues:

```diff
diff --git a/pkg/server/definition_test.go b/pkg/server/definition_test.go
index 0a27308..00d740e 100644
--- a/pkg/server/definition_test.go
+++ b/pkg/server/definition_test.go
@@ -28,6 +28,28 @@ type definitionTestCase struct {
}

var definitionTestCases = []definitionTestCase{
+ {
+ name: "test function parameter shadowing local variable",
+ filename: "testdata/shadow-function-parameter.jsonnet",
+ position: protocol.Position{Line: 2, Character: 2},
+ results: []definitionResult{{
+ targetRange: protocol.Range{
+ Start: protocol.Position{Line: 1, Character: 9},
+ End: protocol.Position{Line: 1, Character: 10},
+ },
+ }},
+ },
+ {
+ name: "test list comprehension variable shadowing local variable",
+ filename: "testdata/shadow-list-comprehension.jsonnet",
+ position: protocol.Position{Line: 2, Character: 2},
+ results: []definitionResult{{
+ targetRange: protocol.Range{
+ Start: protocol.Position{Line: 3, Character: 6},
+ End: protocol.Position{Line: 3, Character: 7},
+ },
+ }},
+ },
{
name: "test goto definition for var myvar",
filename: "./testdata/test_goto_definition.jsonnet",
diff --git a/pkg/server/testdata/shadow-function-parameter.jsonnet b/pkg/server/testdata/shadow-function-parameter.jsonnet
new file mode 100644
index 0000000..16cdc02
--- /dev/null
+++ b/pkg/server/testdata/shadow-function-parameter.jsonnet
@@ -0,0 +1,3 @@
+local x = 1;
+function(x)
+ x
diff --git a/pkg/server/testdata/shadow-list-comprehension.jsonnet b/pkg/server/testdata/shadow-list-comprehension.jsonnet
new file mode 100644
index 0000000..1131a42
--- /dev/null
+++ b/pkg/server/testdata/shadow-list-comprehension.jsonnet
@@ -0,0 +1,5 @@
+local x = 1;
+[
+ x
+ for x in std.range(1, 3)
+]
```

Test failure output:

```
=== FAIL: pkg/server TestDefinition/test_list_comprehension_variable_shadowing_local_variable (0.00s)
definition_test.go:1066:
Error Trace: /Users/lian/local/src/jsonnet-language-server/main/pkg/server/definition_test.go:1066
Error: Not equal:
expected: []protocol.LocationLink{protocol.LocationLink{OriginSelectionRange:0:0-0:0, TargetURI:"file:///Users/lian/local/src/jsonnet-language-server/main/pkg/server/testdata/shadow-list-comprehension.jsonnet", TargetRange:3:6-3:7, TargetSelectionRange:3:6-3:7}}
actual : []protocol.LocationLink{protocol.LocationLink{OriginSelectionRange:0:0-0:0, TargetURI:"file:///Users/lian/local/src/jsonnet-language-server/main/pkg/server/testdata/shadow-list-comprehension.jsonnet", TargetRange:0:6-0:11, TargetSelectionRange:0:6-0:7}}

Diff:
--- Expected
+++ Actual
@@ -15,3 +15,3 @@
Start: (protocol.Position) {
- Line: (uint32) 3,
+ Line: (uint32) 0,
Character: (uint32) 6
@@ -19,4 +19,4 @@
End: (protocol.Position) {
- Line: (uint32) 3,
- Character: (uint32) 7
+ Line: (uint32) 0,
+ Character: (uint32) 11
}
@@ -25,3 +25,3 @@
Start: (protocol.Position) {
- Line: (uint32) 3,
+ Line: (uint32) 0,
Character: (uint32) 6
@@ -29,3 +29,3 @@
End: (protocol.Position) {
- Line: (uint32) 3,
+ Line: (uint32) 0,
Character: (uint32) 7
Test: TestDefinition/test_list_comprehension_variable_shadowing_local_variable

=== FAIL: pkg/server TestDefinition (0.03s)
```

In both test cases, the symbol `x` resolves to the top-level local variable instead of the function parameter and the list-comprehension variable.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start with pkg/server/definition_test.go and the two fixtures in pkg/server/testdata: shadow-function-parameter.jsonnet and shadow-list-comprehension.jsonnet. Run TestDefinition to reproduce the failures, then trace the definition lookup used by these cases. Done means both shadowing tests pass and x resolves to the parameter or comprehension variable rather than the outer local.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
devtools
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.