Skip to content

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Jul 1, 2025

Update even more legacy MaD models to the new model format (continues from #19942 and should be independent of that).

@geoffw0 geoffw0 added no-change-note-required This PR does not need a change note Rust Pull requests that update Rust code labels Jul 1, 2025
@geoffw0
Copy link
Contributor Author

geoffw0 commented Jul 2, 2025

There's currently a large number of test changes caused by this PR (highlighted by CI). I think it would be prudent to get the first two PRs merged before attempting to address this. --- no longer true.

@aibaars
Copy link
Contributor

aibaars commented Jul 10, 2025

At a glance the test changes are due to re-ordering of lines, so that looks fine to me.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Jul 10, 2025

The lost sources in the frameworks/rusqlite test I've added to the issue logging regressions. The rest of the CI failures were just location and model changes. I think this is good to merge now.

@geoffw0 geoffw0 marked this pull request as ready for review July 10, 2025 16:36
@geoffw0 geoffw0 requested a review from a team as a code owner July 10, 2025 16:36
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
Copy link
Contributor

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)?

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor

@aibaars aibaars left a 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.

@@ -24,27 +24,27 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
)",
(),
)?;


let query = format!("INSERT INTO person (name, age) VALUES ('{}', '{}')", name, age);

connection.execute(&query, ())?; // $ sql-sink

let person = connection.query_row(&query, (), |row| { // $ sql-sink
Ok(Person {
Copy link
Contributor

@aibaars aibaars Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just insert the following and record the failure cause as a type inferencer test.

        let row: &rusqlite::Row<'_> = row;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do this tomorrow morning and check everything looks OK then.

age: row.get(2)?, // $ database-read
id: row.get(0)?, // $ MISSING: database-read
name: row.get(1)?, // $ MISSING: database-read
age: row.get(2)?, // $ MISSING: database-read
})
})?;

let mut stmt = connection.prepare("SELECT id, name, age FROM person")?; // $ sql-sink
let people = stmt.query_map([], |row| {
Ok(Person {
Copy link
Contributor

@aibaars aibaars Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just insert the following and record the failure cause as a type inferencer test.

        let row: &rusqlite::Row<'_> = row;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-change-note-required This PR does not need a change note Rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants