Skip to content

Conversation

@hvitved
Copy link
Contributor

@hvitved hvitved commented Oct 28, 2025

Although unit types are not interesting in themselves, it makes a difference now that we are keeping track of expressions without a known type.

Thanks to @paldepind for originally suggesting to have ShorthandReturnTypeMention (like we have ShorthandSelfParameterMention).

DCA confirms that we now have fewer expression without types.

@github-actions github-actions bot added the Rust Pull requests that update Rust code label Oct 28, 2025
@hvitved hvitved changed the title Rust: More type inference tests Rust: Infer more () types Oct 28, 2025
@hvitved hvitved force-pushed the rust/type-inference-unit branch 3 times, most recently from a37c4c0 to 23467ca Compare October 28, 2025 15:29
@hvitved hvitved added the no-change-note-required This PR does not need a change note label Oct 29, 2025
@hvitved hvitved marked this pull request as ready for review October 29, 2025 08:19
@hvitved hvitved requested a review from a team as a code owner October 29, 2025 08:19
Copilot AI review requested due to automatic review settings October 29, 2025 08:19
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances Rust type inference to handle implicit unit return types (()) for functions without explicit return types. The main implementation adds a ShorthandReturnTypeMention class that represents the implicit () return type using the function's name as a placeholder element.

Key changes:

  • Implemented ShorthandReturnTypeMention to handle implicit unit return types for functions
  • Enhanced block expression type inference to detect unit-type blocks (no tail expression and no return statements)
  • Updated documentation examples to use valid Rust syntax with function bodies

Reviewed Changes

Copilot reviewed 10 out of 20 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
rust/ql/lib/codeql/rust/internal/TypeMention.qll Added ShorthandReturnTypeMention class and getReturnTypeMention() function to handle implicit unit return types
rust/ql/lib/codeql/rust/internal/TypeInference.qll Updated type inference to use new return type mention API, enhanced block expression handling with isUnitBlockExpr(), and inferBlockExprType()
rust/ql/test/library-tests/type-inference/main.rs Added test cases for async blocks returning unit type and local functions
rust/schema/annotations.py Fixed documentation examples to use valid Rust syntax
rust/ql/test/extractor-tests/generated/*.rs Updated generated test files with corrected documentation examples
rust/ql/lib/codeql/rust/elements/*.qll Updated documentation in generated QL files
rust/ql/test/library-tests/type-inference/CONSISTENCY/PathResolutionConsistency.expected Updated test expectations with corrected line numbers
rust/ql/.generated.list Updated checksums for regenerated files
rust/ql/test/extractor-tests/generated/.generated_tests.list Updated checksums for regenerated test files

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@hvitved hvitved requested a review from paldepind October 29, 2025 08:46
@hvitved hvitved force-pushed the rust/type-inference-unit branch from 17cd361 to 4b632cf Compare October 29, 2025 12:21
@hvitved hvitved force-pushed the rust/type-inference-unit branch from 4b632cf to bc53fee Compare October 29, 2025 14:43
Copy link
Contributor

@paldepind paldepind left a comment

Choose a reason for hiding this comment

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

Nice! I think it's great to get these covered.

Three thoughts (in addition to my comments):

  • Superficially it looks like it would make sense to use getReturnTypeMention here in getTypeMention for FunctionPosition. Is there a reason not to do that?

  • I would've guessed that knowing more types certainly would reduce "Nodes With Type At Length Limit", but it increases per the DCA report. I think it would be worth it to take a look at one of the affected projects and see what's up.

  • Just for the future, it would have been really nice if the changes to annotations.py and the generated files has been a separate commit.

@hvitved hvitved force-pushed the rust/type-inference-unit branch from c84a964 to cca458c Compare October 30, 2025 12:31
@hvitved
Copy link
Contributor Author

hvitved commented Oct 30, 2025

Done, thanks.

  • I would've guessed that knowing more types certainly would reduce "Nodes With Type At Length Limit", but it increases per the DCA report. I think it would be worth it to take a look at one of the affected projects and see what's up.

I think I was able to figure it out: panic!(...) was incorrectly inferred to have type (), and this meant that we could end up with a receiver having both type () and some other type, which meant that we would allow for a method call both with and without an implicit borrow, which can then explode. The fix is to first make sure that panic!(...) gets the type !, and then subsequently to filter away ! types when matching receiver types against types of self parameters.

  • Just for the future, it would have been really nice if the changes to annotations.py and the generated files has been a separate commit.

Yes, sorry, I should have done that in a separate commit.

@hvitved hvitved requested a review from paldepind October 30, 2025 14:37
Copy link
Contributor

@paldepind paldepind left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and for addressing my comments.

@hvitved hvitved merged commit 95e60ad into github:main Oct 31, 2025
19 checks passed
@hvitved hvitved deleted the rust/type-inference-unit branch October 31, 2025 12:43
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