Skip to content

Rust: Non-symmetric type propagation for lub coercions - #20592

Merged
hvitved merged 3 commits into
github:mainfrom
hvitved:rust/type-inference-branch-propagation
Oct 24, 2025
Merged

Rust: Non-symmetric type propagation for lub coercions#20592
hvitved merged 3 commits into
github:mainfrom
hvitved:rust/type-inference-branch-propagation

Conversation

@hvitved

@hvitvedhvitved commented Oct 7, 2025

Copy link
Copy Markdown
Contributor

Rust applies least-upper bound coercions for certain constructs, such as the branches of an if expression, which means that is not sound to propagate type information directly between branches. In fact, doing so can lead to type inference explosions, as witnessed by the example added on this PR.

Instead of attempting to model lub coercions, we simply prevent type information to propagate from one branch to another by going directly via the type of the if expression (and similar for other constructs with lub coercions).

DCA looks great; we recover the performance lost on #20282 for ruff, rendiation, and coreutils.

@github-actionsgithub-actionsBot added documentation Rust Pull requests that update Rust code labels Oct 7, 2025
@hvitved
hvitvedforce-pushed the rust/type-inference-branch-propagation branch 2 times, most recently from 1f8f5d3 to bfea742CompareOctober 8, 2025 08:20
@hvitvedhvitved changed the title Rust: Non-symmetrict type propagation for if and matchRust: Non-symmetric type propagation for if and matchOct 9, 2025
@hvitved
hvitvedforce-pushed the rust/type-inference-branch-propagation branch 3 times, most recently from 2f501e6 to d0b7388CompareOctober 10, 2025 11:14
@hvitvedhvitved changed the title Rust: Non-symmetric type propagation for if and matchRust: Non-symmetric type propagation for lub coercionsOct 10, 2025
@hvitved
hvitvedforce-pushed the rust/type-inference-branch-propagation branch 2 times, most recently from 61d6041 to 4914a3fCompareOctober 10, 2025 11:55
@hvitved
hvitvedforce-pushed the rust/type-inference-branch-propagation branch 2 times, most recently from 4a16f67 to a1fd231CompareOctober 22, 2025 14:38
@hvitved
hvitvedforce-pushed the rust/type-inference-branch-propagation branch from a1fd231 to d22f70eCompareOctober 22, 2025 14:42
Comment threadrust/ql/test/extractor-tests/generated/ClosureExpr/ClosureExpr.ql Dismissed
Comment threadrust/ql/test/extractor-tests/generated/Function/Function.ql Dismissed
Comment threadrust/ql/test/extractor-tests/generated/Function/Function.ql Dismissed
@hvitved
hvitvedforce-pushed the rust/type-inference-branch-propagation branch from d22f70e to 65b706aCompareOctober 22, 2025 17:57
@hvitvedhvitved added the no-change-note-required This PR does not need a change note label Oct 23, 2025
@hvitved
hvitved marked this pull request as ready for review October 23, 2025 08:54
@hvitved
hvitved requested a review from a team as a code ownerOctober 23, 2025 08:54
CopilotAI review requested due to automatic review settings October 23, 2025 08:54

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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 addresses a type inference performance issue in Rust by preventing symmetric type propagation for constructs that use least-upper bound (LUB) coercions, such as if expressions and match arms with multiple branches.

Key Changes:

  • Introduced non-symmetric type propagation for LUB coercion contexts to prevent type inference explosions
  • Restricted symmetric type equality to single-branch/single-element cases
  • Added a test case demonstrating the type inference explosion scenario

Reviewed Changes

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

FileDescription
rust/ql/lib/codeql/rust/internal/TypeInference.qllImplements non-symmetric type propagation via new predicates lubCoercion and typeEqualityNonSymmetric, and restricts symmetric propagation for multi-branch constructs
rust/ql/test/library-tests/type-inference/main.rsAdds test case with if expression demonstrating type inference explosion scenario
rust/ql/test/library-tests/type-inference/type-inference.expectedUpdates expected test output reflecting reduced type inference results due to non-symmetric propagation

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +704 to +705
lubCoercion(mid, n2, suffix) and
not lubCoercion(mid, n1, _) and

CopilotAIOct 23, 2025

Copy link

Choose a reason for hiding this comment

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

The predicate checks lubCoercion(mid, n2, suffix) but doesn't verify that n1 is also a child of mid that would be subject to LUB coercion. This could lead to unintended type propagation paths. Consider adding a check to ensure n1 is also a relevant child of mid when n2 is subject to LUB coercion.

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I had the same concern, it looks like n1 is bound by type but not by location in the program.

What would the Rust code this logic targets look like?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

For example, if we have

let x = if b {A}else{C}

then

  1. Type information flows from A and B to the entire if.
  2. Type information does not flow from if to neither A nor B.
  3. Type information flows in both directions between if and x.
  4. If type information can flow from something to x, then it also flows to A, but only when that something is not B (and vice versa).

Does that make sense?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, I was reading typeEquality as "these types are equal" (potentially by coincidence, e.g. two unrelated u32s in the program) whereas it is in fact "these types must be equal" (by type inference). So n1 is in fact quite tightly bound by typeEquality. And I should've read the qldoc for typeEquality.

@geoffw0geoffw0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

DCA is indeed excellent.

Couple of questions and comments.

Comment threadrust/ql/test/library-tests/type-inference/main.rs Outdated
Comment threadrust/ql/lib/codeql/rust/internal/TypeInference.qll
Comment threadrust/ql/lib/codeql/rust/internal/TypeInference.qll Outdated
Comment threadrust/ql/lib/codeql/rust/internal/TypeInference.qll
Comment on lines +704 to +705
lubCoercion(mid, n2, suffix) and
not lubCoercion(mid, n1, _) and

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I had the same concern, it looks like n1 is bound by type but not by location in the program.

What would the Rust code this logic targets look like?

@hvitved
hvitved merged commit 672977a into github:mainOct 24, 2025
19 checks passed
@hvitved
hvitved deleted the rust/type-inference-branch-propagation branch October 24, 2025 09:35
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-change-note-requiredThis PR does not need a change noteRustPull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@hvitved@geoffw0@github-advanced-security