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

Revive to the object's variableElement if available #713

Merged
merged 5 commits into from
Jun 9, 2024

Conversation

mattrberry
Copy link
Contributor

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

  • Thanks for your contribution! Please replace this text with a description of what this PR is changing or adding and why, list any relevant issues, and review the contribution guidelines below.

  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

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

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
@kevmoo
Copy link
Member

kevmoo commented Jun 4, 2024

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

@mattrberry
Copy link
Contributor Author

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
Copy link
Member

kevmoo commented Jun 9, 2024

Please add a changelog entry!

@kevmoo kevmoo merged commit ac1837f into dart-lang:master Jun 9, 2024
12 checks passed
@jakemac53
Copy link
Contributor

This seems to be breaking for mockito internally @mattrberry

@jakemac53
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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 added a commit that referenced this pull request Sep 11, 2024
@jakemac53
Copy link
Contributor

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

jakemac53 added a commit that referenced this pull request Sep 11, 2024
@mattrberry
Copy link
Contributor Author

mattrberry commented Sep 12, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants