[google_sign_in] Migrate to nnbd

[mehmetf]

Description

Migrate google_sign_in interface and the frontend to nnbd.

This is a draft PR. I have not migrated web and frontend seems to depend on it as such I suspect the pub resolution won't work.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
• All existing and new tests are passing.
• I updated/added relevant documentation (doc comments with ///).
• The analyzer (flutter analyze) does not report any problems on my PR.
• I read and followed the Flutter Style Guide.
• The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
• I updated CHANGELOG.md to add a description of the change.
• I signed the CLA.
• I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

• Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).

[cyanglaz]

There are some places I think we can make things non-null. I assume we didn't want breaking changes so we made everything nullable? Also since it is already landed internally, we might want to make it in sync.

@blasten Are we ok to land this as is, and introduce a breaking change later?

[mehmetf]

@cyanglaz In the spirit of tightening things, I will push a change here that I pushed internally: Make id and email non-nullable. Do you agree with that?

[mehmetf]

Note that in the native implementations, all account parameters are nullable but in practice the plugin widely assumes that those two parameters define what an account is and as such they should be non-null.

[cyanglaz]

@cyanglaz In the spirit of tightening things, I will push a change here that I pushed internally: Make id and email non-nullable. Do you agree with that?

Agreed

[mehmetf]

All comments have been implemented.

[cyanglaz]

LGTM! CI seems failing.

[blasten]

just to confirm, hostedDomain is now nullable?

[mehmetf]

I think it has always been nullable. According to docs:

  /// The [hostedDomain] argument specifies a hosted domain restriction. By
  /// setting this, sign in will be restricted to accounts of the user in the
  /// specified domain. By default, the list of accounts will not be restricted.

The default is "do not restrict" which is null value.

[blasten]

got it. I was confused by the @required annotation

[blasten]

I think this should throw since it's used in an async chain

[mehmetf]

I did make that change for token data... I am less sure about user data. I did a sweep of the android/ios code and saw that they never return null user map but there's a test for it:

    test('can sign in after aborted flow', () async {
      responses['signIn'] = null;
      expect(await googleSignIn.signIn(), isNull);
      responses['signIn'] = kUserData;
      expect(await googleSignIn.signIn(), isNotNull);
    });

[blasten]

got it. Yeah, it seems tricky to change.

[blasten]

is the analyzer able to detect the _lastMethodCall == null check earlier?

[mehmetf]

I am a bit annoyed by this as well but I have seen it in many other places. My interpretation is that flow analysis only catches variables that are local to current scope. For instance:

void main() {
  String? foo;
  if (foo == null) {
    print('is null');
  } else {
    print(foo.length);
  }
}

is fine. However:

String? foo;

void main() {
  if (foo == null) {
    print('is null');
  } else {
    print(foo.length);
  }
}

is not. You need the bang operator.

[blasten]

I see, thanks. I learned something new. I wonder if @munificent has any thoughts about whether this is the intended design or a temporary limitation

[munificent]

It's an intended design but a known pain point that we'd like to improve if we can figure out how. The problem is that promoting fields isn't sound. In many cases, the compiler can't prove that there is not a class elsewhere that implements that class's interface and overrides with the field with a getter. Or there could be some re-entrant method call on the instance that changes the field's value between when it is checked for null and when it is later used.

[blasten]

ah. that makes sense. The re-rentrant method call is interesting. Could that occur even if the method that is checking for null isn't async?

[munificent]

Yeah. "Re-entrant" is maybe not the best word, but just consider any case between checking a field for null and accessing it where you could end up running other code on the same instance. This is contrived, but should give you the idea:

class Foo {
  int? field;

  method(Foo someFoo) {
    if (field != null) {
      someFoo.other();
      print(field + 1);
    }
  }

  other() {
    field = null;
  }
}

main() {
  var foo = Foo();
  foo.method(foo);
}

[blasten]

shouldRecoverAuth has a default, can it also accept null?

[mehmetf]

The interface does not define a default. Should it?

Future<GoogleSignInTokenData> getTokens(
      {required String email, bool? shouldRecoverAuth}) async {
    throw UnimplementedError('getTokens() has not been implemented.');
  }

[blasten]

Got it. It looks like null is trying to workaround the fact that not all implementations of the interface need this field.

[blasten]

LGTM

[mehmetf]

Thanks! I am going to resolve the conflicts and submit this. I will then publish the interface and then the main package as null-safe.

[mehmetf]

I am not able to submit this because there's a problem resolving dependencies.

Because mockito >=4.1.2 depends on code_builder ^3.4.0 which depends on built_value ^7.0.0, mockito >=4.1.2 requires built_value ^7.0.0.
And because built_value >=5.5.5 <8.0.0-nullsafety.0 depends on quiver >=0.21.0 <3.0.0, mockito >=4.1.2 requires quiver >=0.21.0 <3.0.0.
So, because google_sign_in_platform_interface depends on both quiver ^3.0.0-nullsafety.2 and mockito ^5.0.0-nullsafety.1, version solving failed.

I think the mockito dependencies are wrong since it transitively depends on a non-null-safe version of built_value. Or built_value version restrictions are too tight (on quiver).

The proper fix doesn't seem to be in the pubspec of this package but somewhere downstream. @blasten @davidmorgan

[davidmorgan]

mockito, code_builder and built_value_generator are not yet published for null safety; there's a circular dependency between built_value_generator and build_runner which makes it harder than usual.

We just updated the ranges in build_runner dart-lang/build@0eab40e next step is code_builder ... next step is code_builder, dart-lang/code_builder#306 then it should be possible to publish built_value_generator and mockito.

[ditman]

This pubspec is declaring a 'default_package: google_sign_in_web' for the 'web' platform (line 16), however it's removing the dependency on the web package here. I don't think this'll work for web users. This should be documented in the CHANGELOG.md entry.

[mehmetf]

We had discussed this somewhere in this PR and decided to remove the default package. I am not familiar with how that needs to be executed upstream. Could you please help with that David?

[ditman]

I'm not really sure if this is used on-demand or validated somehow; I think the web endorsement should be removed (otherwise we're cheating pub.dev into reporting "web" support for the null-safe version, which is not true)

[ditman]

Tracking here: flutter/flutter#72239