Resolve for typedefs/aliases and records in `typeArguments` & resolve duplicate typedefs #776

dickermoshe · 2024-10-29T01:27:35Z

Closes: #775
Closes: #777
Closes: #778

The fix for these issues is closely related so I've done both of them in a single PR.

Mockito wasn't checking for typedefs/aliases in typeArguments
it also wasn't looking into records on type args.
Typedefs which had the same underlying type were ignored even if the typedef had a different name

These issues cause the dreaded is missing from the asset URI mapping error.

See the related issue for a repro of each of these. I've also added regression tests for all of these.

Looking forward to a swift review!

I’ve reviewed the contributor guide and applied the relevant portions to this PR.

Contribution guidelines:

See our contributor guide for general expectations for PRs.
Larger or significant changes should be discussed in an issue before creating a PR.
Contributions to our repos should follow the Dart style guide and use dart format.
Most changes should add an entry to the changelog and may need to rev the pubspec package version.
Changes to packages require corresponding tests.

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

srawlins · 2024-10-29T01:51:52Z

CC @mosuem

dickermoshe · 2024-10-29T14:46:20Z

It seems that the bulk of this code is rely on InterfaceType as the only interface object, while records are interfaces themselves.
It seems that the addTypesFrom function, which until now only took a InterfaceType will now have to take records too.

dickermoshe · 2024-10-29T16:01:24Z

Could this be cherrypicked into a 5.4.5 release?

dickermoshe · 2024-10-29T16:12:11Z

CI is broken on the entire repo as dart stable does not support analyzer 6.9. Tests pass locally on my machine.

dickermoshe · 2024-11-06T00:04:28Z

@mosuem
I know people hate they get this, but.... Ping

mosuem · 2024-11-06T08:48:33Z

I don't mind at all, but I am also not a maintainer of this repo and maybe not the right person to ping ;) I just stumbled across the same CI error in dart-lang/test#2293

mosuem · 2024-11-06T08:49:38Z

Maybe @srawlins wants to take a look (I hope he you don't mind the ping). I am fine refiling the PR to move to dart-lang/test and merging this first.

srawlins

Thanks so much! I left a few notes but generally the fix is gold 😁

test/builder/auto_mocks_test.dart

srawlins · 2024-11-06T16:06:34Z

test/builder/auto_mocks_test.dart

@@ -3553,6 +3553,64 @@ void main() {
        expect(mocksContent, contains('class MockBaz extends _i1.Mock'));
        expect(mocksContent, contains('implements _i2.Baz'));
      });
+
+      test('when its a type parameter of function which returns another type',


I'm not sure what you mean here by "a type parameter of function which returns another type." In this test case, the typedef appears to only be used as a type argument in a class definition class Foo extends BaseFoo<CreateBar>.

How about?
typedef mocks are generated properly when it\'s a function which returns any type

In this instance, Bar was being ignored becuase the return type of CreateBar want being resolved

That SG, thanks!

test/builder/auto_mocks_test.dart

lib/src/builder.dart

CHANGELOG.md

srawlins · 2024-11-06T16:17:44Z

@mosuem I love a friendly ping 😁 I am fine doing whatever procedure works best for you, migrating to dart-lang/test. I think a git commit can be wired up that attributes the fix to @dickermoshe still...

For external PRs like this, I think I've been able to just hand-copy the PR, once landed, into google3, and then re-export. But there might be a good copybara mechanism for that, at this point...

mosuem · 2024-11-06T17:33:01Z

I'll have to check the copybara settings, but normally even g3 SoT repos should be able to accept Github PRs. We can probably copy the config from package:intl, then it should just work.

mosuem · 2024-11-06T17:33:51Z

But please go ahead with merging this, any infra work can wait, features are much nicer ;)

dickermoshe · 2024-11-06T19:45:53Z

Thanks for the timely response. This is the coolest library ever, thank so much!
Fixed all the nits, lmk if there is anything else

srawlins · 2024-12-03T00:17:31Z

Sorry for the delay, we're shipping a big change to the analyzer element model, and analyzer 7.0.0 just dropped. I think we have to do some cleanup on our side before landing this. Hopefully this week. 🤞 I'll stay in touch.

srawlins · 2024-12-04T00:28:42Z

OK we should be good to go. Ship it!

…uplicate typedefs #776 Closes: #775 Closes: #777 Closes: #778 The fix for these issues is closely related so I've done both of them in a single PR. - Mockito wasn't checking for typedefs/aliases in `typeArguments` - it also wasn't looking into records on type args. - Typedefs which had the same underlying type were ignored even if the typedef had a different name These issues cause the dreaded `is missing from the asset URI mapping` error. See the related issue for a repro of each of these. I've also added regression tests for all of these. PiperOrigin-RevId: 702712435

…uplicate typedefs dart-lang/mockito#776 Closes: dart-lang/mockito#775 Closes: dart-lang/mockito#777 Closes: dart-lang/mockito#778 The fix for these issues is closely related so I've done both of them in a single PR. - Mockito wasn't checking for typedefs/aliases in `typeArguments` - it also wasn't looking into records on type args. - Typedefs which had the same underlying type were ignored even if the typedef had a different name These issues cause the dreaded `is missing from the asset URI mapping` error. See the related issue for a repro of each of these. I've also added regression tests for all of these. PiperOrigin-RevId: 702712435

dickermoshe mentioned this pull request Oct 29, 2024

managers extends public classes simolus3/drift#3314

Closed

dickermoshe changed the title ~~Resolve for typedefs/aliases in typeArguments~~ Resolve for typedefs/aliases and records in typeArguments Oct 29, 2024

dickermoshe marked this pull request as draft October 29, 2024 15:03

dickermoshe marked this pull request as ready for review October 29, 2024 15:32

parse records and typedef in type arguments

e8e95bc

dickermoshe force-pushed the master branch from b8642c7 to e8e95bc Compare October 29, 2024 15:55

dickermoshe marked this pull request as draft October 29, 2024 17:04

parse the contents of a typedef

1470659

dickermoshe marked this pull request as ready for review October 29, 2024 17:05

allow duplicate typedefs

6730288

dickermoshe changed the title ~~Resolve for typedefs/aliases and records in typeArguments~~ Resolve for typedefs/aliases and records in typeArguments & allow resolve typedefs Oct 29, 2024

dickermoshe changed the title ~~Resolve for typedefs/aliases and records in typeArguments & allow resolve typedefs~~ Resolve for typedefs/aliases and records in typeArguments & resolve duplicate typedefs Oct 29, 2024

srawlins requested changes Nov 6, 2024

View reviewed changes

dickermoshe added 2 commits November 6, 2024 14:33

fix nits

f083cc5

fix identical type alias test

8b2f248

Merge branch 'master' into dickermoshe/master

4b39373

srawlins enabled auto-merge (rebase) December 4, 2024 00:28

srawlins approved these changes Dec 4, 2024

View reviewed changes

srawlins merged commit dbc9302 into dart-lang:master Dec 4, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve for typedefs/aliases and records in `typeArguments` & resolve duplicate typedefs #776

Resolve for typedefs/aliases and records in `typeArguments` & resolve duplicate typedefs #776

dickermoshe commented Oct 29, 2024 •

edited

Loading

srawlins commented Oct 29, 2024

dickermoshe commented Oct 29, 2024

dickermoshe commented Oct 29, 2024

dickermoshe commented Oct 29, 2024

dickermoshe commented Nov 6, 2024

mosuem commented Nov 6, 2024

mosuem commented Nov 6, 2024

srawlins left a comment

srawlins Nov 6, 2024

dickermoshe Nov 6, 2024

srawlins Dec 3, 2024

srawlins commented Nov 6, 2024

mosuem commented Nov 6, 2024

mosuem commented Nov 6, 2024

dickermoshe commented Nov 6, 2024

srawlins commented Dec 3, 2024

srawlins commented Dec 4, 2024

Resolve for typedefs/aliases and records in typeArguments & resolve duplicate typedefs #776

Resolve for typedefs/aliases and records in typeArguments & resolve duplicate typedefs #776

Conversation

dickermoshe commented Oct 29, 2024 • edited Loading

srawlins commented Oct 29, 2024

dickermoshe commented Oct 29, 2024

dickermoshe commented Oct 29, 2024

dickermoshe commented Oct 29, 2024

dickermoshe commented Nov 6, 2024

mosuem commented Nov 6, 2024

mosuem commented Nov 6, 2024

srawlins left a comment

Choose a reason for hiding this comment

srawlins Nov 6, 2024

Choose a reason for hiding this comment

dickermoshe Nov 6, 2024

Choose a reason for hiding this comment

srawlins Dec 3, 2024

Choose a reason for hiding this comment

srawlins commented Nov 6, 2024

mosuem commented Nov 6, 2024

mosuem commented Nov 6, 2024

dickermoshe commented Nov 6, 2024

srawlins commented Dec 3, 2024

srawlins commented Dec 4, 2024

Resolve for typedefs/aliases and records in `typeArguments` & resolve duplicate typedefs #776

Resolve for typedefs/aliases and records in `typeArguments` & resolve duplicate typedefs #776

dickermoshe commented Oct 29, 2024 •

edited

Loading