[pigeon] migrate to null-safety

[Goddchen]

Migrating pigeon package to null-safety.

I marked this as a draft because I have trouble getting one test to pass: packages/pigeon/mock_handler_tester/test/widget_test.dart:calling methods with null and I just can't work it out :/ The call throws an AssertionError, which is the expected behavior. Yet the test fails stating that this error was thrown. Can anyone help?

[google-cla]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

[Goddchen]

@googlebot I signed it!

[mitchross]

I really need this pacakge too... How do I include this fork in my own project?

[mitchross]

@Goddchen here is your error

[Goddchen]

thanks @mitchross . I didn't event notice this test failing. It's fixed. But the other one that I mentioned in the PR descriptions still puzzles me.

[Goddchen]

Also the local_tests CHANNEL:master check fails with the message No dart files found at: /private/var/folders/sr/b58hwhtj0jbcf4r09zmg0wlc0000gn/T/cirrus-ci-build/packages/pigeon/lib. Well... Obviously there are Dart files in that folder?! 🤷‍♂️

[mitchross]

@Goddchen did lots of digging, the issue isn't the unit test, the issue is the assertion throw "fails" the unit test. The unit test will catch it, but it doesn't matter if you comment out the entire test, the test still fails!

[Goddchen]

@mitchross Yeah, I see that piece of code. What puzzles me really is that if I replace the throwing .send(null) code with just an assert(false); failing assertion. The whole test just runs fine. What's the difference between my assert(...) and the one from the code you just showed? 🤔

[mitchross]

@mitchross Yeah, I see that piece of code. What puzzles me really is that if I replace the throwing .send(null) code with just an assert(false); failing assertion. The whole test just runs fine. What's the difference between my assert(...) and the one from the code you just showed? 🤔

Do what you suggested, I think this is a bad test setup, with null-safety, it doesn't even make sense anymore.

plus with this todo, id go with your suggestion

[Goddchen]

Fixed it! After giving it a bit more thought, my finding was that the generated code was actually not doing exactly what it was intended to do. It should handle the null case properly - which it does now.

[Goddchen]

Also the local_tests CHANNEL:master check fails with the message No dart files found at: /private/var/folders/sr/b58hwhtj0jbcf4r09zmg0wlc0000gn/T/cirrus-ci-build/packages/pigeon/lib. Well... Obviously there are Dart files in that folder?! 🤷‍♂️

I still don't get what is wrong with this check... The files are there... What's the problem...

[mitchross]

Also the local_tests CHANNEL:master check fails with the message No dart files found at: /private/var/folders/sr/b58hwhtj0jbcf4r09zmg0wlc0000gn/T/cirrus-ci-build/packages/pigeon/lib. Well... Obviously there are Dart files in that folder?! 🤷‍♂️

I still don't get what is wrong with this check... The files are there... What's the problem...

The all checks have passed by github, I think you are fine...

Can you also explain the fix a bit more? I went deep into this code and I'm very curious!

[Goddchen]

@gaaclarke Still getting this strange No dart files found at: /private/var/folders/sr/b58hwhtj0jbcf4r09zmg0wlc0000gn/T/cirrus-ci-build/packages/pigeon/lib error on local_tests CHANNEL:master. Can you make any sense out of it? I find it especially strange since the same thing runs just fine in the local_tests CHANNEL:stable check 🤔

[gaaclarke]

@Goddchen I'm not sure, I thought I saw that before in the past but I can't remember where or why. FWIW in my recent PR I switched the dartanalyzer call to dart analyze. That might fix your issue if you rebase.

[google-cla]

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

[Goddchen]

Dang, I think it just messed up 🙈 I pulled the master changes instead of rebasing. Can you hint me on how to fix this mess? Sorry 😞

[gaaclarke]

git reset --hard <hash before your merge, d61a774?>
git rebase -i <upstream remote>/master
git push -f <remote for your changes>

[Goddchen]

git reset --hard <hash before your merge, d61a774?>
git rebase -i <upstream remote>/master
git push -f <remote for your changes>

I guess it worked 🥳 And I guess from your prompt reply that you see this too often 😉 Thanks

[Goddchen]

@gaaclarke We're up-to-date and running again 👍

[mitchross]

@jmagman could we get this merged?

[gaaclarke]

@mitchross In order to get it merged Goddchen has to request a code review from someone on the Flutter team (it'd likely be deferred to me). Then they need to go through the review process. I just happened to glance at the code the other day and didn't actually review it. I'm waiting for @Goddchen to be ready.

[Goddchen]

I didn't know that I had to do anything. I used the "re-request review" button at the top now, was that correct? 🤔

[gaaclarke]

Yep, thanks @Goddchen. This is a big review, I'll try to get to it soon. I'm balancing a few different priorities. If a couple weeks pass you won't be bothering me if you ping me.

[gaaclarke]

Hey @Goddchen, looks like you accidentally added some changes for files outside of pigeon.

[gaaclarke]

I got rid of the other file edits by squashing your commits and force pushing. There was something weird going on with the PR. When I squash commits I don't expect to get conflicts, but I did. I'll try rebasing it on master next so are ready to merge, the squashing should have made this easier. Watch out, I'll have to force push again after this.

[gaaclarke]

(I had to rebase one last time to to resolve conflicts from a PR that just landed, hopefully that is the last time.)

[gaaclarke]

Ok, getting closer. I'm going to grab lunch. I think there is a bit of documentation that has to happen before I publish this as well. I'll just write that up in a different PR once this lands. I think I can get it in by EOD.

[gaaclarke]

Nice, thanks for the contribution! I'll clean up the rest of the documentation and stuff in another PR before I publish it.

One thing that would have been helpful if if you tried to keep the migration to null safety and your changes to the output as separate PRs. It was a bit of work to detangle them.

Take care dude!