Rust: Type inference for ? expressions
#19367
Merged
+184
−4
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
f7021ab to
9e0304b
Compare
9e0304b to
a3c26b4
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for inferring types of Rust ? (try) expressions and includes test coverage for Result-based ? patterns.
- Introduce
inferTryExprTypein the inference engine and include it in cached inference. - Add a new
try_expressionstest module exercising?on variousResultscenarios. - Update expected outputs for the new tests and extend the Postgres path‐resolution consistency expectations.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| rust/ql/lib/codeql/rust/internal/TypeInference.qll | Added inferTryExprType, an import, and integrated it into cached inference |
| rust/ql/test/library-tests/type-inference/main.rs | Added try_expressions module with ?-operator tests for Result |
| rust/ql/test/library-tests/type-inference/type-inference.expected | Updated expected outputs to include ?-expression inference entries |
| rust/ql/test/library-tests/frameworks/postgres/CONSISTENCY/PathResolutionConsistency.expected | Added duplicate method-call entries for consistency verification |
Comments suppressed due to low confidence (3)
rust/ql/test/library-tests/type-inference/main.rs:922
- The new
inferTryExprTypealso handlesOption-based?expressions, but the tests only coverResult. Consider adding a test function that uses?on anOptionto validate inference forOption<T>.
mod try_expressions {
rust/ql/lib/codeql/rust/internal/TypeInference.qll:9
- The import
codeql.rust.frameworks.stdlib.Stdlibis not used byinferTryExprTypeor elsewhere in this file. Consider removing it to avoid a redundant dependency.
+private import codeql.rust.frameworks.stdlib.Stdlib
rust/ql/test/library-tests/type-inference/main.rs:945
- In
try_chained, the first?is applied tox: Result<Result<S1, S1>, S1>, but the function signature returnsResult<S1, S2>. Without convertingS1toS2(e.g. viamap_err), this will fail to compile. Align the error type or add an explicit conversion.
fn try_chained() -> Result<S1, S2> {
geoffw0
approved these changes
May 1, 2025
| | main.rs:30:5:30:39 | conn.query_opt(...) | file://:0:0:0:0 | fn query_opt | | ||
| | main.rs:30:5:30:39 | conn.query_opt(...) | file://:0:0:0:0 | fn query_opt | | ||
| | main.rs:35:17:35:67 | conn.query(...) | file://:0:0:0:0 | fn query | | ||
| | main.rs:35:17:35:67 | conn.query(...) | file://:0:0:0:0 | fn query | |
There was a problem hiding this comment.
How much of a concern are the new inconsistencies?
There was a problem hiding this comment.
These consistencies are not really new, they are just surfaced because we can now infer a type for conn at
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
DCA looks great; we gain more call edges without affecting performance.