Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

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

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.

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
Morty Proxy This is a proxified and sanitized view of the page, visit original site.