Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

There is no way to generate a mock of Foo<Bar> if Bar is a type parameter bound #559

Closed
yanok opened this issue Aug 3, 2022 · 10 comments
Closed
Labels
P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@yanok
Copy link
Contributor

yanok commented Aug 3, 2022

Assume we have

@GenerateMocks([], customMocks: [MockSpec<Foo<Bar>>(as: #FooBar)])
import 'test.mocks.dart';

class Bar {}
abstract class Foo<T extends Bar> {
  T m();
}

This is supposed to work without unsupportedMembers since T gets instantiated to the concrete type Bar, but the codegen can't actually take apart Foo<Bar> from just Foo (analyzer limitation?), so it assumes it's Foo and generates a mock for the generic type, even though an instantiated version was requested, requiring to add unsupportedMembers or fallbackGenerators.

@srawlins
Copy link
Member

srawlins commented Aug 3, 2022

Yeah I think we can't tell in the element model if T was instantiated to its bound, or if it's bound was provided directly.

@srawlins
Copy link
Member

srawlins commented Aug 3, 2022

One way to support this would be to add a flag which sends an explicit signal to mockito code-gen to "make this non-generic." Something like MockSpec(instantiateToBound: true).

@srawlins srawlins added type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) P2 A bug or feature request we're likely to work on labels Aug 3, 2022
@yanok
Copy link
Contributor Author

yanok commented Aug 3, 2022

One way to support this would be to add a flag which sends an explicit signal to mockito code-gen to "make this non-generic." Something like MockSpec(instantiateToBound: true).

Yeah, that would be a possible workaround but it really feels wrong that user has to tell us this explicitly. Why would I (as an user) even care I instantiate it with the exact bound?

I would argue that since Foo actually means Foo<Bar> in Dart (I don't approve that BTW ;)), it should be the other way around: the default is to generate a mock for Foo<Bar> and have an explicit flag to generate a generic one... But that would mean breaking the existing usage...

I found another workaround BTW: we can do ElementAnnotation.toSource() and check whether Foo was passed with type arguments or not... But that is super freaking hacky...

@srawlins
Copy link
Member

srawlins commented Aug 3, 2022

Yeah, that would be a possible workaround but it really feels wrong that user has to tell us this explicitly.

Agreed.

I found another workaround BTW: we can do ElementAnnotation.toSource() and check whether Foo was passed with type arguments or not... But that is super freaking hacky...

Yeah, agreed. ☹️

@scheglov
Copy link
Contributor

scheglov commented Aug 4, 2022

I don't understand why you need to distinguish. Both Foo and Foo<Bar> produce the same type.
I probably don't know enough of Mockito to understand the description in the initial comment.

@srawlins
Copy link
Member

srawlins commented Aug 4, 2022

When a user wants to say, "I want to generate a mock class for Foo", our API lets them do that with an annotation:

MockSpec<Foo>

However, with an annotation like that, there is no way to specify whether they want a generic mock (class MockFoo<T> extends Mock implements Foo<T>) or a mock extending Foo with its type parameters instantiated-to-bounds (class MockFoo extends Mock implements Foo<dynamic>).

(Long story) We currently assume they want a generic one, because who wants dynamic yuck. But in the case of a non-dynamic bound, like Bar, it seems more common to want a non-generic mock which implements Foo<Bar>. That generic-vs-nongeneric distinction becomes important for complicated reasons around methods which return the type variable (a method like T getSomething()). It is impossible for mockito to override this method in the generic case, but it can override the instantiated case.

@scheglov
Copy link
Contributor

scheglov commented Aug 4, 2022

Can you add a parameter to MockSpec constructor that explicitly says whether a generic mock should be generated? Just like as.

@srawlins
Copy link
Member

srawlins commented Aug 4, 2022

That is my idea above 😄

@scheglov
Copy link
Contributor

scheglov commented Aug 4, 2022

Yes, almost. I just thought (randomly) to ask explicitly for generic. Now, I can rationalize it by saying that Foo means Foo<Bar>, so by default you ask for instantiated version.

It seems that what we need it a reference to a class, and pass it to MockSpec.generic(#Foo) (if #Foo actually meant the reference).

@yanok
Copy link
Contributor Author

yanok commented Aug 4, 2022

Yes, I agree that it makes more sense to get the instantiated class by default and only make it generic if asked explicitly (with almost the same argument, see my comment), but that's a breaking API change. Well, maybe we can break the new @GenerateNiceMocks API, which it not used much yet...

srawlins pushed a commit that referenced this issue Sep 13, 2022
Assuming `Foo` is declared as `class Foo<T extends Bar> { /* ... */ }`,
there is indeed no way to take apart `Foo` from `Foo<Bar>` once it gets
into the analyzer internals and becomes an `InterfaceType`. The AST still
preserve this information though but we are not looking at the
annotation AST, instead we utilize the `computeContantValue` that gives us
`DartObject` that is easier to work with but now `Foo` and `Foo<Bar>` are
smashed together. It appears to me that there is currently even no public
API to get the AST for the annotation, so I had to cheat with
```dart
(annotation as ElementAnnotationImpl).annotationAst
```
to get the actual AST. When we can use it to just check the presence of
type arguments and pass the results down.

This is still rather unclean, not using the best practice and doesn't work in
all cases where constant evaluation does. For example, with constant evaluation
one can declared `MockSpec` as a constant outside of the annotation and then
use it, but it breaks with direct AST access. I haven't found any other
examples so far. Not being able to use `MockSpec`s defined as constants
doesn't really look like a big deal though: there is no reasonable way to
use a `MockSpec` instance more than once anyway, I think.

On the positive side, this change fixes the bug without changing the API.

Fixes #559 and
#563.

Also opens a way for fixing #562.

PiperOrigin-RevId: 467867637
srawlins added a commit that referenced this issue Sep 13, 2022
Assuming `Foo` is declared as `class Foo<T extends Bar> { /* ... */ }`,
there is indeed no way to take apart `Foo` from `Foo<Bar>` once it gets
into the analyzer internals and becomes an `InterfaceType`. The AST still
preserve this information though but we are not looking at the
annotation AST, instead we utilize the `computeContantValue` that gives us
`DartObject` that is easier to work with but now `Foo` and `Foo<Bar>` are
smashed together. It appears to me that there is currently even no public
API to get the AST for the annotation, so I had to cheat with
```dart
(annotation as ElementAnnotationImpl).annotationAst
```
to get the actual AST. When we can use it to just check the presence of
type arguments and pass the results down.

This is still rather unclean, not using the best practice and doesn't work in
all cases where constant evaluation does. For example, with constant evaluation
one can declared `MockSpec` as a constant outside of the annotation and then
use it, but it breaks with direct AST access. I haven't found any other
examples so far. Not being able to use `MockSpec`s defined as constants
doesn't really look like a big deal though: there is no reasonable way to
use a `MockSpec` instance more than once anyway, I think.

On the positive side, this change fixes the bug without changing the API.

Fixes #559 and
#563.

Also opens a way for fixing #562.

PiperOrigin-RevId: 467867637
srawlins added a commit that referenced this issue Sep 13, 2022
Assuming `Foo` is declared as `class Foo<T extends Bar> { /* ... */ }`,
there is indeed no way to take apart `Foo` from `Foo<Bar>` once it gets
into the analyzer internals and becomes an `InterfaceType`. The AST still
preserve this information though but we are not looking at the
annotation AST, instead we utilize the `computeContantValue` that gives us
`DartObject` that is easier to work with but now `Foo` and `Foo<Bar>` are
smashed together. It appears to me that there is currently even no public
API to get the AST for the annotation, so I had to cheat with
```dart
(annotation as ElementAnnotationImpl).annotationAst
```
to get the actual AST. When we can use it to just check the presence of
type arguments and pass the results down.

This is still rather unclean, not using the best practice and doesn't work in
all cases where constant evaluation does. For example, with constant evaluation
one can declared `MockSpec` as a constant outside of the annotation and then
use it, but it breaks with direct AST access. I haven't found any other
examples so far. Not being able to use `MockSpec`s defined as constants
doesn't really look like a big deal though: there is no reasonable way to
use a `MockSpec` instance more than once anyway, I think.

On the positive side, this change fixes the bug without changing the API.

Fixes #559 and
#563.

Also opens a way for fixing #562.

PiperOrigin-RevId: 467867637
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

3 participants