-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Rust: Update legacy MaD models 3 #19946
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
|
At a glance the test changes are due to re-ordering of lines, so that looks fine to me. |
The lost sources in the |
id: row.get(0)?, // $ database-read | ||
name: row.get(1)?, // $ database-read | ||
age: row.get(2)?, // $ database-read | ||
id: row.get(0)?, // $ MISSING: database-read |
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.
Are you sure the problem is a missing canonical path, or is the problem that getStaticTarget
fails (due to type inferencer problem)?
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.
Just checked the canonical path exists, the problem is the type inferencer, most likely the closure argument.
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.
Yep you're right, the getStaticTarget
fails (and thus getStaticTarget().(Addressable).getCanonicalPath()
fails too) so it's likely a type inference problem.
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.
Yeah that's why I use a concat
in my test predicate:
predicate test(MethodCallExpr e, Function target, string path) {
target = e.getStaticTarget() and
path = concat(target.getCanonicalPath())
}
- no result means
getStaticTarget()
failed - empty string means: a target was found, but
getCanonicalPath()
failed
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.
most likely the closure argument.
Correct; type inference for closures is a known gap.
Yeah that's why I use a concat in my test predicate:
I can also recommend the various debug predicates in TypeInference.qll
; change getRelevantLocatable
to the relevant file+line, and then quick-eval debugInferType
et al.
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.
I will try the TypeInference.qll
debug predicates, thanks for reminding me those exist.
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.
Looks mostly fine, the rusqlite test failures can be fixed though.
Fixed issues and merged in main. Ready for approval. |
Have you checked the hyper project? DCA reports we are losing a fair number of sources. Just to make sure the hyper models are good |
There is still a test failure:
|
Update even more legacy MaD models to the new model format (continues from #19942 and should be independent of that).