Rust: Make trait a base type mention of the self type parameter

[paldepind]

This PR changes how the Self parameter is handled in trait methods. Suppose some trait Trait<A>. Today a method on this trait is handled like this (where the | is some fictional syntax):

fn before<Self>(self: Self | Trait<A>) -> A;

The self parameter has the type of an implicit Self parameter and the trait type. On a call the passed self is then matched as a "subtype" of Trait<A> to infer A.

With this PR the situration is now like this:

fn after<A, Self : Trait<A>>(self: Self) -> A;

The self parameter only has the Self type and the Self type parameter now has the trait as a trait bound. This works since we can now handle trait bounds on type parameters, and A is inferred through that.

The biggest benefit of this is that we no longer infer superfluous types. For instance, we currently infer multiple return types for calls to trait methods that return Self: both the inferred Self type and the trait type itself. But the latter follows from the former. This reduction in type can be seen in the change to the .expected file where all the trait types and their type arguments are now gone.

[Copilot]

Copilot wasn't able to review any files in this pull request.

Files not reviewed (4)

• rust/ql/lib/codeql/rust/internal/Type.qll: Language not supported
• rust/ql/lib/codeql/rust/internal/TypeInference.qll: Language not supported
• rust/ql/lib/codeql/rust/internal/TypeMention.qll: Language not supported
• rust/ql/test/library-tests/type-inference/type-inference.expected: Language not supported

Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more

[hvitved]

Very nice refactor, one small suggestion and question.

[hvitved]

I think, instead of using AstNode, it would be better to use something like

private import codeql.rust.elements.internal.generated.Synth

private class TTypeParamOrTrait = Synth::TTypeParam or Synth::TTrait;

private class TypeParamOrTrait extends AstNode, TTypeParamOrTrait { }

in order to reduce pressure on the equivalenceRelation HOP.

[paldepind]

That makes sense. I remember that we also do equivalenceRelation on Raw::AstNode over in the CFG implementation. Perhaps it would make sense to factor that one out and reuse it for both CFG and type inference?

[hvitved]

In order to be properly shared, we would have to make it cached, I'm not sure we'd want that.

[paldepind]

Is it only the type of idOf that should be changed? Not idOfRaw and id?

[paldepind]

I've changed idOf now. I picked at different name than TypeParamOrTrait as we'll also have to add TypeAliases later when they become type parameters.

[hvitved]

idOfRaw and id should be updated as well, otherwise the equivalenceRelation invocation will not be restricted.

[paldepind]

I'm not sure how they should be updated to match the suggestion? I've changed idOfRaw now such that it only applies to the relevant extensional predicates, but it doesn't look like the above.

[hvitved]

So, the Self type parameter is now moved from the trait into each of the methods, which makes sense. But I wonder if this approach will also work once support for associated type defaults lands? E.g., in something like

trait MyTrait {
  type T = Option<Self>

  fn foo(self) -> T;
}

Perhaps it will Just Work with your other PR that introduced support for type aliases?

[paldepind]

I haven't thought of that. I don't think it will just work, but I think it can be made to work.

Associated types will be type parameters of the trait and the trait's methods, like this:

fn foo<Self : MyTrait<T>, T>(self) -> T

So we have to ensure that whatever makes Self a MyTrait has a type argument for T and we should read that off of the default if nothing else is provided. So for an impl block that uses the default it should behave as if the default was written for T:

impl MyTrait<Option<Foo>> for Foo { ... }