Skip to content

Rust: Only include relevant AST nodes in TypeMention #19557

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

Merged
merged 4 commits into from
May 27, 2025

Conversation

paldepind
Copy link
Contributor

At some point when quick eval'ing something I noticed that we have a lot of superfluous elements in TypeMention.

This PR adds a basic consistency check for TypeMentions: that they have a type in the root. One could go further and check that the entire type tree corresponds to the types, but checking the root was enough at the moment to find interesting stuff.

The biggest source of unnecessary TypeMentions was that we include Path in TypeMention, which also included all paths that are not types.

A second inconsistency where paths where path resolution failed to find anything. These have now been excluded, as they don't do any good as TypeMentions anyway.

There's still a lot of inconsistencies reported by the PR. One source is paths that resolve to a union, which don't have a type as we do not support unions.

On rust this PR reduces the number of TypeMentions from around 13 million to around 2 million.

@github-actions github-actions bot added the Rust Pull requests that update Rust code label May 22, 2025
@@ -0,0 +1,2 @@
illFormedTypeMention
| main.rs:403:18:403:24 | FuncPtr |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This inconsistency is a path to a function pointer type, which doesn't have a type as we don't support function pointers types.

@paldepind paldepind marked this pull request as ready for review May 22, 2025 13:27
@paldepind paldepind requested a review from a team as a code owner May 22, 2025 13:27
@paldepind paldepind added the no-change-note-required This PR does not need a change note label May 22, 2025
@paldepind
Copy link
Contributor Author

DCA seems fine though I don't really understand the result.

  • Missing call targets is down, but I wouldn't expect this PR to affect that.
  • I would also have expected inconsistencies to go up somewhere, but that's not the case. Are type inference inconsistencies no in DCA?

@hvitved
Copy link
Contributor

hvitved commented May 23, 2025

  • I would also have expected inconsistencies to go up somewhere, but that's not the case. Are type inference inconsistencies no in DCA?

They would have to be added here, and then we would also have to add a new report to DCA.

@paldepind
Copy link
Contributor Author

Thanks. I've added type inference inconsistencies to Stats.qll. I guess the DCA changes should be made once this is merged.

Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

LGTM, but some tests are failing.

@paldepind paldepind force-pushed the rust/type-mention-consistency branch 6 times, most recently from a627277 to 5e4576b Compare May 26, 2025 13:51
@paldepind paldepind force-pushed the rust/type-mention-consistency branch 3 times, most recently from 3099d3d to 0023122 Compare May 27, 2025 08:45
@paldepind
Copy link
Contributor Author

Now the CI is (finally) happy. I removed type inference inconsistencies from the reduced summary stats and also suppressed inconsistencies in library code.

hvitved
hvitved previously approved these changes May 27, 2025
@@ -156,7 +164,14 @@ predicate inconsistencyStats(string key, int value) {
or
key = "Inconsistencies - SSA" and value = getTotalSsaInconsistencies()
or
key = "Inconsistencies - data flow" and value = getTotalDataFlowInconsistencies()
key = "Inconsistencies - Data flow" and value = getTotalDataFlowInconsistencies()
Copy link
Contributor

Choose a reason for hiding this comment

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

This will require an internal change in the DCA report.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh 😭 I'll revert it.

@paldepind paldepind force-pushed the rust/type-mention-consistency branch from 0023122 to 6313703 Compare May 27, 2025 10:41
@paldepind paldepind force-pushed the rust/type-mention-consistency branch from 6313703 to 5228062 Compare May 27, 2025 10:43
@paldepind paldepind merged commit 254eabf into github:main May 27, 2025
39 checks passed
@paldepind paldepind deleted the rust/type-mention-consistency branch May 27, 2025 11:45
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