-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Rust: Rust: add jump to definition for format arguments #17805
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
import codeql.rust.elements.GenericParam | ||
import codeql.rust.elements.GenericParamList | ||
import codeql.rust.elements.IdentPat | ||
import codeql.rust.elements.IfExpr |
Check warning
Code scanning / CodeQL
Redundant import Warning
codeql.rust.elements.FormatArgsExpr
Redundant import, the module is already imported inside codeql.rust.elements.FormatArgument.
|
||
private import internal.FormatArgumentImpl | ||
import codeql.rust.elements.Format | ||
import codeql.rust.elements.Locatable |
Check warning
Code scanning / CodeQL
Redundant import Warning
codeql.rust.elements.Format
8653e35
to
a755175
Compare
4cfbd20
to
24abe34
Compare
a755175
to
54589af
Compare
24abe34
to
a7628e7
Compare
17d4746
to
35b4aa0
Compare
35b4aa0
to
f092594
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, a few comments. I also think we should have some tests.
/** | ||
* Gets an element, of kind `kind`, that element `use` uses, if any. | ||
*/ | ||
cached |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should probably also cache TDef
, and make sure to cache them together, e.g.
private cached module Cached {
cached newtype TDef ...
cached Definition definitionOf(Use use, string kind) ...
}
predicate definitionOf = Cached::definitionOf/2;
private import codeql.Locations | ||
|
||
/** An element with an associated definition. */ | ||
abstract class Use extends Locatable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably makes sense to restrict this to uses that are not the result of a macro expansion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand. This PR adds jump to definition for format arguments, which are all uses that are the result of macro expansion.
Perhaps we should restrict the definitions though. That should get rid of all the left_val
and right_val
references in the expansion of assert!
macros.
*/ | ||
|
||
private import codeql.rust.elements.Variable | ||
private import codeql.rust.elements.Locatable |
Check warning
Code scanning / CodeQL
Redundant import Warning
codeql.rust.elements.Format
|
||
private import codeql.rust.elements.Variable | ||
private import codeql.rust.elements.Locatable | ||
private import codeql.rust.elements.FormatArgsExpr |
Check warning
Code scanning / CodeQL
Redundant import Warning
codeql.rust.elements.Format
private import codeql.rust.elements.Variable | ||
private import codeql.rust.elements.Locatable | ||
private import codeql.rust.elements.FormatArgsExpr | ||
private import codeql.rust.elements.FormatArgsArg |
Check warning
Code scanning / CodeQL
Redundant import Warning
@hvitved FYI