Rust: Jump-to-def for operations and indexing by hvitved · Pull Request #20900 · github/codeql

hvitved · 2025-11-24T12:21:43Z

For debugging, it is handy to also have jump-to-def for operations and indexing.

Copilot

Pull request overview

This PR adds jump-to-definition support for Rust operations (like unary - and binary +) and indexing expressions (like S[0]). When these operators are used, they can now link to their underlying trait method implementations (neg, add, index, etc.).

Added hasLocationInfo predicate to the Use abstract class as a base implementation
Introduced OperationUse class to handle jump-to-definition for overloaded operations
Introduced IndexExprUse class to handle jump-to-definition for indexing operations

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
rust/ql/lib/codeql/rust/internal/Definitions.qll	Added base `hasLocationInfo` predicate to `Use` class and implemented `OperationUse` and `IndexExprUse` classes to enable jump-to-definition for operations and indexing
rust/ql/test/library-tests/definitions/main.rs	Added test cases for operations (`-S`, `S + S`) and indexing (`S[0]`) with corresponding trait implementations
rust/ql/test/library-tests/definitions/Definitions.expected	Updated expected test output to include jump-to-definition entries for the new operation and indexing test cases

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-25T09:26:05Z

rust/ql/lib/codeql/rust/internal/Definitions.qll

+    this =
+      any(PrefixExpr pe |
+        pe.getLocation().hasLocationInfo(filepath, startline, startcolumn, _, _) and
+        pe.getExpr().getLocation().hasLocationInfo(_, endline, endcolumn + 2, _, _)


The location calculation appears to have the start and end columns inverted. For a prefix expression like -S on line 67, the expected output shows 67:5:67:4 (start column 5, end column 4), where the end column is before the start column. This will likely cause issues with jump-to-definition functionality.

Looking at the logic:

Line 193: pe.getLocation().hasLocationInfo(filepath, startline, startcolumn, _, _) gets the prefix expression's start location

Line 194: pe.getExpr().getLocation().hasLocationInfo(_, endline, endcolumn + 2, _, _) gets the operand's end location

The issue is that for -S, the prefix expression location is the entire -S, and the operand S comes after the -. So using the prefix expression's start column and the operand's end column (which comes before it) results in inverted columns.

Consider calculating the operator location more directly, or ensure endcolumn >= startcolumn.

Suggested change

pe.getExpr().getLocation().hasLocationInfo(_, endline, endcolumn + 2, _, _)

endline = startline and

endcolumn = startcolumn + 1

Copilot · 2025-11-25T09:26:06Z

rust/ql/lib/codeql/rust/internal/Definitions.qll

+    super.getIndex().getLocation().hasLocationInfo(filepath, _, _, startline, startcolumn - 2) and
+    this.getLocation().hasLocationInfo(_, _, _, endline, endcolumn)


The location calculation results in inverted columns where end column is before start column. For an index expression like S[0] on line 69, the expected test output shows 69:9:69:8 (start column 9, end column 8).

Looking at the logic:

Line 214: super.getIndex().getLocation().hasLocationInfo(filepath, _, _, startline, startcolumn - 2) - This tries to get a location where the index ends at startcolumn - 2, which would place startcolumn approximately at the opening bracket [

Line 215: this.getLocation().hasLocationInfo(_, _, _, endline, endcolumn) - This gets the end of the entire IndexExpr

The issue is that getIndex() returns the index expression (e.g., 0), and this code assumes its end location minus 2 gives the bracket position. However, the arithmetic appears incorrect, resulting in endcolumn < startcolumn.

Consider revising the location calculation to ensure endcolumn >= startcolumn, or compute the bracket positions more accurately.

Suggested change

super.getIndex().getLocation().hasLocationInfo(filepath, _, _, startline, startcolumn - 2) and

this.getLocation().hasLocationInfo(_, _, _, endline, endcolumn)

this.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)

geoffw0

I think Copilot is right, some of the location approximations could be improved, though I disagree with its suggested changes.

geoffw0 · 2025-11-25T11:30:05Z

rust/ql/test/library-tests/definitions/Definitions.expected

 | main.rs:66:14:66:14 | S | main.rs:3:1:4:9 | struct S | path |
 | main.rs:66:18:66:20 | new | main.rs:46:9:48:9 | fn new | path |
 | main.rs:66:22:66:22 | S | main.rs:3:1:4:9 | struct S | path |
+| main.rs:67:5:67:4 | - ... | main.rs:11:5:13:5 | fn neg | method |


Shouldn't this be main.rs:67:5:67:5? i.e. endcolumn + 2 in the logic should be endcolumn + 1.

That's what I did initially, but that means putting the cursor between - and S will jump to both neg and S, and I want to avoid overlapping targets. With the above, only putting the cursor before - will jump to neg.

That's odd - S is clearly (from the line below) character 6, while - is character 5.

I'm not sure what you mean about "putting the cursor between" two characters - surely the granularity here is which single character you're pointing at?

putting the cursor between - and S

I still don't know what you mean here. Surely your mouse is either over the - or over the S?

geoffw0 · 2025-11-25T11:31:24Z

rust/ql/test/library-tests/definitions/Definitions.expected

+| main.rs:68:7:68:7 | ... + ... | main.rs:19:5:21:5 | fn add | method |
 | main.rs:68:9:68:9 | S | main.rs:3:1:4:9 | struct S | path |
 | main.rs:69:5:69:5 | S | main.rs:3:1:4:9 | struct S | path |
+| main.rs:69:9:69:8 | S[0] | main.rs:27:5:29:5 | fn index | method |


... and shouldn't this be main.rs:69:6:69:8? I think the formula for brackets ought to be one character wider than the index, i.e. super.getIndex().getLocation().hasLocationInfo(filepath, startline, startcolumn + 1, endline, endcolumn - 1).

If we do it like that, then putting the cursor between x and ] in a[x] will jump to both index and x.

So having it just cover ] is a compromise to avoid overlaps.

I still don't see why the range is backwards from character 9 to character 8 - shouldn't it just be character 8?

geoffw0 · 2025-11-25T11:36:07Z

rust/ql/test/library-tests/definitions/main.rs

    s.method();
    M1::S2::<S>::new(S);
+    -S;
+    S + S;


I think there would be an issue if you'd wrote S+S here. We might be better off accepting that we can't assume spaces are present, and just include them in the location range of the +? i.e. replace startcolumn - 2 with startcolumn - 1 and endcolumn + 2 with endcolumn + 1.

I have added more tests, refined the logic, and added more comments.

geoffw0

More suggestions. I get the impression we have different mental models of locations though. My understanding is that they're a 1-based range of characters including both ends, so for example main.rs:10:1:10:2 is exactly the first and second characters of line ten.

geoffw0 · 2025-11-26T14:10:30Z

rust/ql/lib/codeql/rust/internal/Definitions.qll

+      (
+        startline < endline
+        or
+        endcolumn = startcolumn


I think this would be sufficient:

Suggested change

endcolumn = startcolumn

startcolumn <= endcolumn

Though it seems like this check is only needed because you offset by 2 places instead of 1, above.

geoffw0 · 2025-11-26T14:11:03Z

rust/ql/lib/codeql/rust/internal/Definitions.qll

+      be.getLhs().getLocation().hasLocationInfo(filepath, _, _, startline, startcolumn - 2) and
+      be.getRhs().getLocation().hasLocationInfo(filepath, endline, endcolumn + 2, _, _) and


Is there any reason we don't take all the space between the LHS and RHS?

Suggested change

be.getLhs().getLocation().hasLocationInfo(filepath, _, _, startline, startcolumn - 2) and

be.getRhs().getLocation().hasLocationInfo(filepath, endline, endcolumn + 2, _, _) and

be.getLhs().getLocation().hasLocationInfo(filepath, _, _, startline, startcolumn - 1) and

be.getRhs().getLocation().hasLocationInfo(filepath, endline, endcolumn + 1, _, _) and

geoffw0 · 2025-11-26T14:13:53Z

rust/ql/lib/codeql/rust/internal/Definitions.qll

+    string filepath, int startline, int startcolumn, int endline, int endcolumn
+  ) {
+    // `x[y]`: placing the cursor after `]` jumps to `index`
+    super.getIndex().getLocation().hasLocationInfo(filepath, _, _, startline, startcolumn - 2) and


Suggested change

super.getIndex().getLocation().hasLocationInfo(filepath, _, _, startline, startcolumn - 2) and

super.getIndex().getLocation().hasLocationInfo(filepath, _, _, startline, startcolumn - 1) and

?

geoffw0 · 2025-11-26T14:17:15Z

rust/ql/lib/codeql/rust/internal/Definitions.qll

+    any(PrefixExpr pe |
+      pe.getLocation().hasLocationInfo(filepath, startline, startcolumn, _, _) and
+      endline = startline and
+      endcolumn = startcolumn - 1


I think this would fix the backwards-location issue on test line 67:

Suggested change

endcolumn = startcolumn - 1

endcolumn = startcolumn

Though I'm starting to get the impression you maybe want the location starting at column 5, ending at column 4 for some reason. If that's true you're going to have to explain why - it just seems invalid to me.

github-actions bot added the Rust Pull requests that update Rust code label Nov 24, 2025

hvitved force-pushed the rust/operation-index-jump-to-def branch from fd73345 to c81a1c2 Compare November 24, 2025 12:27

hvitved added no-change-note-required This PR does not need a change note labels Nov 24, 2025

hvitved marked this pull request as ready for review November 25, 2025 09:21

hvitved requested a review from a team as a code owner November 25, 2025 09:21

hvitved requested review from Copilot and geoffw0 November 25, 2025 09:21

Copilot started reviewing on behalf of hvitved November 25, 2025 09:22 View session

Copilot finished reviewing on behalf of hvitved November 25, 2025 09:25

Copilot AI reviewed Nov 25, 2025

View reviewed changes

geoffw0 reviewed Nov 25, 2025

View reviewed changes

geoffw0 reviewed Nov 26, 2025

View reviewed changes

hvitved added 3 commits December 1, 2025 13:56

Rust: Add jump-to-def tests for operations and indexing

9c747e7

Rust: Jump-to-def for operations and indexing

7982ae9

Rust: Add more jump-to-def test cases and refine logic

86013a3

hvitved force-pushed the rust/operation-index-jump-to-def branch from f04c359 to 86013a3 Compare December 1, 2025 12:58

	pe.getExpr().getLocation().hasLocationInfo(_, endline, endcolumn + 2, _, _)
	endline = startline and
	endcolumn = startcolumn + 1

		super.getIndex().getLocation().hasLocationInfo(filepath, _, _, startline, startcolumn - 2) and
		this.getLocation().hasLocationInfo(_, _, _, endline, endcolumn)

	super.getIndex().getLocation().hasLocationInfo(filepath, _, _, startline, startcolumn - 2) and
	this.getLocation().hasLocationInfo(_, _, _, endline, endcolumn)
	this.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)

		be.getLhs().getLocation().hasLocationInfo(filepath, _, _, startline, startcolumn - 2) and
		be.getRhs().getLocation().hasLocationInfo(filepath, endline, endcolumn + 2, _, _) and

Conversation

hvitved commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

geoffw0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geoffw0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hvitved commented Nov 24, 2025 •

edited

Loading