Rust: Type inference and path resolution for builtins#19474
Rust: Type inference and path resolution for builtins#19474hvitved merged 4 commits intogithub:mainfrom
Conversation
a7477cf to
d69e84a
Compare
d69e84a to
c488065
Compare
c488065 to
542e883
Compare
542e883 to
a02bf18
Compare
paldepind
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.