Skip to content

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Aug 25, 2025

No description provided.

@github-actions github-actions bot added the Rust Pull requests that update Rust code label Aug 25, 2025
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@hvitved hvitved force-pushed the rust/type-inference-method-call-resolution-rework branch 2 times, most recently from e4cfb86 to 4a8c37c Compare August 26, 2025 18:30
@hvitved hvitved force-pushed the rust/type-inference-method-call-resolution-rework branch from 4a8c37c to e75d79e Compare August 27, 2025 15:34

/**
* Holds if `constraint` might occur as the third argument of
* `potentialInstantiationOf`. Defaults to simply projecting the third
* argument of `potentialInstantiationOf`.
*/
default predicate relevantTypeMention(TypeMention tm) { potentialInstantiationOf(_, _, tm) }
default predicate relevantTypeMention(Constraint tm) { potentialInstantiationOf(_, _, tm) }

Check warning

Code scanning / CodeQL

Missing QLDoc for parameter Warning

The QLDoc has no documentation for tm, but the QLDoc mentions constraint
@hvitved hvitved force-pushed the rust/type-inference-method-call-resolution-rework branch 8 times, most recently from 61866bf to 2d1ed65 Compare September 1, 2025 09:45
@hvitved hvitved force-pushed the rust/type-inference-method-call-resolution-rework branch from 2d1ed65 to 3d19a06 Compare September 1, 2025 10:30
@hvitved hvitved force-pushed the rust/type-inference-method-call-resolution-rework branch from 3d19a06 to 153c10b Compare September 1, 2025 17:56
@hvitved hvitved force-pushed the rust/type-inference-method-call-resolution-rework branch from 153c10b to e161d4c Compare September 1, 2025 18:35
@hvitved hvitved force-pushed the rust/type-inference-method-call-resolution-rework branch from dd45f7b to a20c440 Compare September 2, 2025 07:24
@hvitved hvitved force-pushed the rust/type-inference-method-call-resolution-rework branch 5 times, most recently from f45d2d5 to f9f8782 Compare September 3, 2025 13:07
pragma[nomagic]
private Type inferMethodCallExprType(AstNode n, TypePath path) {
exists(
MethodCallMatchingInput::Access a, MethodCallMatchingInput::AccessPosition apos, string state,

Check warning

Code scanning / CodeQL

Omittable 'exists' variable Warning

This exists variable can be omitted by using a don't-care expression
in this argument
.
@hvitved hvitved force-pushed the rust/type-inference-method-call-resolution-rework branch from f9f8782 to 4bfd7aa Compare September 3, 2025 15:00
|
functionResolutionDependsOnArgument(impl, _, result, pos, _, _)
or
exists(TypeParameter tp | traitTypeParameterOccurrence(trait, resolved, _, pos, _, tp) |

Check warning

Code scanning / CodeQL

Omittable 'exists' variable Warning

This exists variable can be omitted by using a don't-care expression
in this argument
.
@hvitved hvitved force-pushed the rust/type-inference-method-call-resolution-rework branch from 4bfd7aa to 8cd0e06 Compare September 3, 2025 15:17
* `self5` | `impl T2 for X` | `X`
*/
private class FunctionPositionType extends TFunctionPositionType {
private predicate asFunctionPositionType(Function f, FunctionPosition pos, ImplOrTraitItemNode i) {

Check warning

Code scanning / CodeQL

Predicates starting with "get" or "as" should return a value Warning

This predicate starts with 'as' but does not return a value.

pragma[nomagic]
abstract Type getParameterType(DeclarationPosition dpos, TypePath path);
private predicate asInheritedFunctionPositionType(

Check warning

Code scanning / CodeQL

Predicates starting with "get" or "as" should return a value Warning

This predicate starts with 'as' but does not return a value.
*/
// pragma[nomagic]
pragma[inline]
private predicate methodCandidateTrait(

Check warning

Code scanning / CodeQL

Missing QLDoc for parameter Warning

The QLDoc has no documentation for i, or self, but the QLDoc mentions impl
@Copilot Copilot AI review requested due to automatic review settings September 3, 2025 17:52
@hvitved hvitved force-pushed the rust/type-inference-method-call-resolution-rework branch from eb173a1 to 39b1a4a Compare September 3, 2025 17:52
Copy link
Contributor

@Copilot 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 reworks type inference for method calls in the Rust codeQL extractor, addressing a major limitation where method calls with multiple potential targets couldn't be properly resolved. The changes implement a more sophisticated approach that distinguishes between stateful and stateless contexts to improve method resolution accuracy.

Key changes:

  • Introduced stateful type inference to handle method calls with multiple candidates
  • Updated type inference infrastructure to support state-based resolution
  • Enhanced method call target resolution for complex scenarios

Reviewed Changes

Copilot reviewed 34 out of 36 changed files in this pull request and generated 2 comments.

File Description
shared/typeinference/codeql/typeinference/internal/TypeInference.qll Major refactoring to support stateful type inference, introducing MatchingWithState module and state-based method resolution
rust/ql/lib/codeql/rust/internal/TypeMention.qll Added new type mention classes including SelfParameterImplicitMention for handling implicit self parameters
rust/ql/lib/codeql/rust/internal/Type.qll Added support for union types, never type, and pointer types, enhancing the type system coverage
Multiple .expected files Updated test expectations reflecting improved method call resolution accuracy

Comment on lines 1744 to +1745
filepath.matches("%/main.rs") and
startline = 52
startline = 167
Copy link
Preview

Copilot AI Sep 3, 2025

Choose a reason for hiding this comment

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

This hardcoded line number (167) in the debug module appears to be a leftover from development/testing. Consider removing this specific line constraint or making it configurable to avoid issues when the test file structure changes.

See below for a potential fix:

      filepath.matches("%/main.rs")

Copilot uses AI. Check for mistakes.

Copilot

This comment was marked as outdated.

@hvitved hvitved force-pushed the rust/type-inference-method-call-resolution-rework branch from 39b1a4a to b4bd65b Compare September 3, 2025 18:43
}

/** Holds if this relevant access should satisfy `constraint`. */
Type getConstraint() { relevantAccessConstraint(a, target, apos, path, result) }
Type getConstraint(Declaration target) {

Check warning

Code scanning / CodeQL

Missing QLDoc for parameter Warning

The QLDoc has no documentation for target, but the QLDoc mentions constraint
@Copilot Copilot AI review requested due to automatic review settings September 3, 2025 19:00
@hvitved hvitved force-pushed the rust/type-inference-method-call-resolution-rework branch from b4bd65b to cb8a603 Compare September 3, 2025 19:00
Copy link
Contributor

@Copilot 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 reworks type inference for method calls in Rust by introducing a state-based type matching system. The main goal is to improve method resolution accuracy by tracking type parameter instantiations across different candidate methods during resolution.

Key changes:

  • Introduces a new MatchingWithState module that tracks state during type matching
  • Refactors the IsInstantiationOf module to support generic constraints alongside applications
  • Adds support for additional type constructs like unions, never types, and pointer types
  • Improves method call resolution by considering multiple potential targets with different states

Reviewed Changes

Copilot reviewed 34 out of 36 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
shared/typeinference/codeql/typeinference/internal/TypeInference.qll Major refactoring of type inference logic to support state-based matching and improved instantiation checking
rust/ql/test/utils-tests/modelgenerator/CaptureSummaryModels.expected Updated test expectations for generated model summaries
rust/ql/test/library-tests/type-inference/main.rs Test file updates to reflect improved method resolution capabilities
rust/ql/test/library-tests/type-inference/dereference.rs Added comprehensive tests for dereferencing scenarios and method resolution
rust/ql/lib/codeql/rust/internal/TypeMention.qll Enhanced type mention resolution with support for self parameters and additional type constructs
rust/ql/lib/codeql/rust/internal/Type.qll Added support for union types, never types, and pointer types
Various test expected files Updated to reflect improved type inference and method resolution accuracy

Comment on lines 698 to 699
app.getTypeAt(path) != t
)
Copy link
Preview

Copilot AI Sep 3, 2025

Choose a reason for hiding this comment

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

The removed code that checked for inconsistent type parameter instantiations appears to be important validation logic. Removing this without clear justification could lead to incorrect type inference results. Consider adding a comment explaining why this validation is no longer needed or restore it if the removal was unintentional.

Suggested change
app.getTypeAt(path) != t
)
)
or
// There is an inconsistent instantiation of a type parameter
exists(int i |
exists(TypeParameter tp | tp = getNthTypeParameter(abs, i)) and
not typeParametersHaveEqualInstantiationFromIndex(app, abs, tm, i)
)

Copilot uses AI. Check for mistakes.

@@ -1740,7 +1742,7 @@ private module Debug {
exists(string filepath, int startline, int startcolumn, int endline, int endcolumn |
result.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn) and
filepath.matches("%/main.rs") and
startline = 52
startline = 167
Copy link
Preview

Copilot AI Sep 3, 2025

Choose a reason for hiding this comment

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

[nitpick] This hardcoded line number in the debug module should be removed or made configurable. Having hardcoded debug coordinates in production code can cause confusion and makes the code less maintainable.

See below for a potential fix:

  /**
   * Returns a relevant locatable for debugging, configurable by line number.
   */
  private Locatable getRelevantLocatable(int startline) {
    exists(string filepath, int sline, int startcolumn, int endline, int endcolumn |
      result.getLocation().hasLocationInfo(filepath, sline, startcolumn, endline, endcolumn) and
      filepath.matches("%/main.rs") and
      sline = startline
    )
  }

  /** Convenience: default line number for debugging. */
  private int defaultDebugStartLine() { result = 167 }

  /** Overload for backward compatibility: uses default line number. */
  private Locatable getRelevantLocatable() { result = getRelevantLocatable(defaultDebugStartLine()) }

  predicate debugUnqualifiedPathLookup(
    RelevantPath p, string name, Namespace ns, ItemNode encl, string path
  ) {
    p = getRelevantLocatable(defaultDebugStartLine()) and
    unqualifiedPathLookup(encl, name, ns, p) and
    path = p.toStringDebug()
  }

  ItemNode debugResolvePath(RelevantPath path) {
    path = getRelevantLocatable(defaultDebugStartLine()) and
    result = resolvePath(path)
  }

  predicate debugUseImportEdge(Use use, string name, ItemNode item, SuccessorKind kind) {
    use = getRelevantLocatable(defaultDebugStartLine()) and
    useImportEdge(use, name, item, kind)
  }

  ItemNode debugGetASuccessor(ItemNode i, string name, SuccessorKind kind) {
    i = getRelevantLocatable(defaultDebugStartLine()) and
    result = i.getASuccessor(name, kind)
  }

  predicate debugFileImportEdge(Module mod, string name, ItemNode item, SuccessorKind kind) {
    mod = getRelevantLocatable(defaultDebugStartLine()) and
    fileImportEdge(mod, name, item, kind)
  }

  predicate debugFileImport(Module m, SourceFile f) {
    m = getRelevantLocatable(defaultDebugStartLine()) and
    fileImport(m, f)
  }

  predicate debugPreludeEdge(SourceFile f, string name, ItemNode i) {
    preludeEdge(f, name, i) and
    f = getRelevantLocatable(defaultDebugStartLine())
  }

  string debugGetCanonicalPath(ItemNode i, Crate c) {
    result = i.getCanonicalPath(c) and
    i = getRelevantLocatable(defaultDebugStartLine())

Copilot uses AI. Check for mistakes.

ImplItemNode i, FunctionPositionType self, string derefChainBorrow, MethodCall mc, Trait trait
) {
exists(Type rootType, string name, int arity |
mc.(MethodCall).isMethodCall0(rootType, name, arity, derefChainBorrow) and

Check warning

Code scanning / CodeQL

Redundant cast Warning

Redundant cast to
MethodCall
.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant