dart-lang / source_gen

Automatic source code generation for Dart
https://pub.dev/packages/source_gen
BSD 3-Clause "New" or "Revised" License
484 stars 105 forks source link

Revive to the object's `variableElement` if available #713

Closed mattrberry closed 3 months ago

mattrberry commented 4 months ago

Variables have been tracked on DartObject since https://dart.googlesource.com/sdk/+/54b7f4b72a1701f8f9a0334c94ce6f59732bd261. This change uses the variable in the reviver if it exists. I believe the existing tests in https://github.com/dart-lang/source_gen/blob/master/source_gen/test/constants_test.dart already cover this, but lmk if you'd like any additional tests


Contribution guidelines:
- See our [contributor guide](https://github.com/dart-lang/.github/blob/main/CONTRIBUTING.md) 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](https://dart.dev/guides/language/effective-dart) and use `dart format`. - Most changes should add an entry to the changelog and may need to [rev the pubspec package version](https://github.com/dart-lang/sdk/wiki/External-Package-Maintenance#making-a-change). - Changes to packages require [corresponding tests](https://github.com/dart-lang/.github/blob/main/CONTRIBUTING.md#Testing). Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.
kevmoo commented 4 months ago

I mean, if this changes the behavior, we should have a test that shows the behavior change/improvement...right?

mattrberry commented 4 months ago

This is really just a fast-path that avoids iterating all members in the library rather than a change in behavior.. That said, revive doesn't support extension types yet, so this technically does change behavior in that it adds support for extension types assigned to variables that would be otherwise un-revivable. It's not really a regression test for this feature, but it's something.

kevmoo commented 3 months ago

Please add a changelog entry!

jakemac53 commented 3 weeks ago

This seems to be breaking for mockito internally @mattrberry

jakemac53 commented 3 weeks ago

We are anyways doing a breaking release, so we can possibly make this change, but need to call it out in the changelog and fix mockito

jakemac53 commented 3 weeks ago

Fwiw the issue seems to be that imports don't get added for these variables in generated mockito code. That might just be a mockito bug or something I am not sure.

mattrberry commented 3 weeks ago

I also just looked at one case and it seems like the source_gen Revivable is correct, which would align with your thought that mockito just isn't emitting the import. I haven't worked much with mockito, but I can take a look if you aren't already

jakemac53 commented 3 weeks ago

but I can take a look if you aren't already

If you can that would be great, I only looked enough to understand mockito was the problem

mattrberry commented 3 weeks ago

Mockito assumes that the import uri is where the object's type is defined, which isn't always true. This seems like a bug in mockito.

e.g.

// foo.dart
class Foo {
  static final bar = Bar();
}
// bar.dart
class Bar {}

With this change, we now create a Revivable for Foo.bar, and mockito reads the import for that as bar.dart rather than foo.dart.