-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Rust: Type inference and path resolution for builtins #19474
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
base: main
Are you sure you want to change the base?
Conversation
a7477cf
to
d69e84a
Compare
d69e84a
to
c488065
Compare
c488065
to
542e883
Compare
542e883
to
a02bf18
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.
Really amazing to get types for builtins! 🤩
When I run the tests I get a lot of errors of the form:
Location is outside of test directory: file:///BUILTINS/types.rs:8:1:8:15
Do we have to live with those or is there some workaround? Maybe transforming the path a bit?
"Hello"; | ||
123.0f64; | ||
true; | ||
false; |
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 would be nice to turn these 5 lines into let ... = ...
expressions and add inline expectations as above.
) { | ||
exists(string file | | ||
this.getLocation().hasLocationInfo(file, startline, startcolumn, endline, endcolumn) and | ||
filepath = file.regexpReplaceAll("^/.*/tools/builtins/", "/BUILTINS/") |
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.
Maybe add a comment as for why we're doing this? Is it only to make things look cleaner in the .expected
file?
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 is because the actual file path points to the folder containing the extractor, so it is very much environment dependent.
} | ||
|
||
/** The builtin `isize` type. */ | ||
class ISize extends BuiltinType { |
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.
Given that the Rust type is written as one word in all lower-case (and not something like iSize
or i_size
) I'd expect the the QL name to be Isize
? Same for usize
above.
pragma[nomagic] | ||
StructType getBuiltinType(string name) { | ||
result = TStruct(any(Builtins::BuiltinType t | name = t.getName())) | ||
} | ||
|
||
pragma[nomagic] | ||
private StructType inferLiteralType(LiteralExpr le) { | ||
le instanceof CharLiteralExpr and | ||
result = TStruct(any(Builtins::Char t)) | ||
or | ||
le instanceof StringLiteralExpr and | ||
result = TStruct(any(Builtins::Str t)) | ||
or | ||
le = | ||
any(IntegerLiteralExpr n | | ||
not exists(n.getSuffix()) and | ||
result = getBuiltinType("i32") | ||
or | ||
result = getBuiltinType(n.getSuffix()) | ||
) | ||
or | ||
le = | ||
any(FloatLiteralExpr n | | ||
not exists(n.getSuffix()) and | ||
result = getBuiltinType("f32") | ||
or | ||
result = getBuiltinType(n.getSuffix()) | ||
) | ||
or | ||
le instanceof BooleanLiteralExpr and | ||
result = TStruct(any(Builtins::Bool t)) | ||
} |
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.
Here's an idea for a simple refactor. It factors out the repeated TStruct
and the repeated n.getSuffix()
logic.
Note that this requires getSuffix
to be an abstract predicate on NumberLiteralExpr
which makes sense as all the extensions have that predicate.
pragma[nomagic] | |
StructType getBuiltinType(string name) { | |
result = TStruct(any(Builtins::BuiltinType t | name = t.getName())) | |
} | |
pragma[nomagic] | |
private StructType inferLiteralType(LiteralExpr le) { | |
le instanceof CharLiteralExpr and | |
result = TStruct(any(Builtins::Char t)) | |
or | |
le instanceof StringLiteralExpr and | |
result = TStruct(any(Builtins::Str t)) | |
or | |
le = | |
any(IntegerLiteralExpr n | | |
not exists(n.getSuffix()) and | |
result = getBuiltinType("i32") | |
or | |
result = getBuiltinType(n.getSuffix()) | |
) | |
or | |
le = | |
any(FloatLiteralExpr n | | |
not exists(n.getSuffix()) and | |
result = getBuiltinType("f32") | |
or | |
result = getBuiltinType(n.getSuffix()) | |
) | |
or | |
le instanceof BooleanLiteralExpr and | |
result = TStruct(any(Builtins::Bool t)) | |
} | |
pragma[nomagic] | |
private StructType inferLiteralType(LiteralExpr le) { | |
exists(Builtins::BuiltinType t | result = TStruct(t) | | |
le instanceof CharLiteralExpr and | |
t instanceof Builtins::Char | |
or | |
le instanceof StringLiteralExpr and | |
t instanceof Builtins::Str | |
or | |
le = | |
any(NumberLiteralExpr ne | | |
t.getName() = ne.getSuffix() | |
or | |
not exists(ne.getSuffix()) and | |
( | |
ne instanceof IntegerLiteralExpr and | |
t instanceof Builtins::I32 | |
or | |
ne instanceof FloatLiteralExpr and | |
t instanceof Builtins::F64 | |
) | |
) | |
or | |
le instanceof BooleanLiteralExpr and | |
t instanceof Builtins::Bool | |
) | |
} |
Yeah, we have to live with those, or alternatively show no file path at all. |
Follows up on #19421, and adds type inference and path resolution for builtins.
DCA confirms that we gain additional call edges, without significant performance impact.