Rust: Add inline expectations test for type inference#19198
Rust: Add inline expectations test for type inference#19198paldepind merged 4 commits intogithub:mainfrom
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
paldepind
force-pushed
the
rust-ti-inline-expectations
branch
from
1eca118 to
5c08cd6
Compare
| if this.getArgList().getNumberOfArgs() = 0 | ||
| then parenthesis = "()" | ||
| else parenthesis = "(...)" | ||
| ) and |
There was a problem hiding this comment.
It would be better to implement this on ArgList, but that might cause more changes so I'm leaving that for a follow up PR.
paldepind
force-pushed
the
rust-ti-inline-expectations
branch
from
5c08cd6 to
715765a
Compare
geoffw0
left a comment
geoffw0
left a comment
There was a problem hiding this comment.
I think ideally this would have been two PRs, one for the inline expecatation tests change, and one of the changes to comment and type toStringss that cause a lot of noisy changes in the tests. Splitting it into different commits would have helped. It probably isn't worth doing retroactively though.
That aside, the use of inline expectations is an improvement that will pay off if we're expecting a lot of future changes to this test. 👍
| // Explicit type argument | ||
| let x = GenericThing::<S> { a: S }; | ||
| println!("{:?}", x.a); | ||
| let x = GenericThing::<S> { a: S }; // $ type=x:A.S |
There was a problem hiding this comment.
What does this annotation mean exactly?
There was a problem hiding this comment.
This means that x has a type which at parameter position A contains S. In this case x has the type GenericThing<S> and GenericThing has a type parameter called A, hence x has S at A. For this line we could also add $ type=x:GenericThing since the "root" of x's type is GenericThing.
hvitved
left a comment
hvitved
left a comment
There was a problem hiding this comment.
Can I ask you to (force push) a change that moves all ToString changes into a separate commit before continuing my review, please?
| then parenthesis = "()" | ||
| else parenthesis = "(...)" | ||
| ) and | ||
| result = base + separator + this.getIdentifier().toStringImpl() + parenthesis |
There was a problem hiding this comment.
It would be better to use the toStringPart pattern here instead, to avoid computing intermediate strings
| @@ -43,10 +43,15 @@ module Impl { | |||
| } | |||
|
|
|||
| override string toStringImpl() { | |||
There was a problem hiding this comment.
It would have been nice to have all toString changes (including changes in .expected files) in a separate commit.
paldepind
force-pushed
the
rust-ti-inline-expectations
branch
from
715765a to
d5d61dd
Compare
Done, now split in two commits :) |
| trait T2<T>: T1<S<T>> { | ||
| fn bar(self) { | ||
| self.foo() | ||
| self.foo() // $ method=foo |
There was a problem hiding this comment.
I think we should change this to using canonical paths, once that PR lands, because the comment as-is doesn't really tell us anything about the target.
There was a problem hiding this comment.
Yes, that is a good idea if they're not too cumbersome to write.
But this also works reasonably well. We only use just the name in the cases where there's a single method of a given name in scope, and then the method resolution would have to do something very wrong to find another method with the same name.
| x2.set(S); // $ method=MyOption::set | ||
| println!("{:?}", x2); | ||
|
|
||
| let mut x3 = MyOption::new(); // missing type `S` from `MyOption<S>` (but can resolve `MyTrait<S>`) |
There was a problem hiding this comment.
Can this comment be updated?
There was a problem hiding this comment.
Good question. I left this in place because the x3 is stringified as "mut x3". I think it might make more sense to just print it as "x3" and I'd prefer to not write too many "unstable" stringifications in the annotations.
| f.getPrecedingComment().getCommentText() = value | ||
| or | ||
| not exists(f.getPrecedingComment()) and | ||
| value = f.getName().getText() |
There was a problem hiding this comment.
Perhaps add a TODO comment to use canonical name instead (if it exists)?
4 participants
Adds inline expectations tests for type inference.
The annotations for resolved method calls and field expressions are pretty straightforward and the annotations now subsume what we had in the expected file before.
I've also added annotations for inferred types. We obviously don't want to annotate every single expression, so these annotations are optional and can be used in important places only. These annotations depend on stringifying AST nodes which can be a bit annoying, but I don't think it's too bad is we only use them sparingly and can write the tests with that in mind.
Let's merge at least #19146 before this.