Skip to content

We need to migrate to null-safety #2737

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

Closed
jonataslaw opened this issue Aug 12, 2021 · 6 comments
Closed

We need to migrate to null-safety #2737

jonataslaw opened this issue Aug 12, 2021 · 6 comments
Labels
P1 A high priority bug; for example, a single project is unusable or has many test failures type-enhancement A request for a change that isn't a bug

Comments

@jonataslaw
Copy link

jonataslaw commented Aug 12, 2021

Hi, I would like to propose switching to null-safety.
I can contribute to this if there is no blockage.
I noticed that there are many packages in pub.dev that have their score downgraded because of dartdoc issues, and these issues could be easily fixed if we had null security, which prevents new null bugs from being inserted with every update.
Looking at the source, I can see for example that there are many ifs without else that can potentially cause bugs, for example:

https://github.com/dart-lang/dartdoc/blob/master/lib/src/model/type_parameter.dart#L38

  ElementType get boundType {
    if (_boundType == null) {
      var bound = element.bound;
      if (bound != null) {
        _boundType = ElementType.from(bound, library, packageGraph);
      }
    }
    return _boundType;
  }

boundType cannot be null since we use this class's getter all the time, for example:

https://github.com/dart-lang/dartdoc/blob/master/lib/src/model/type_parameter.dart#L71

  @override
  Map<String, CommentReferable> get referenceChildren {
    return _referenceChildren ??= {
      boundType.name: boundType, // boundType.name will cause a null exception
    };
  }

However, let's look at this code:

ElementType get boundType {
    if (_boundType == null) { // _boundType is null
      var bound = element.bound;
      if (bound != null) { // if bound is not null, we add ElementType.from bound
        _boundType = ElementType.from(bound, library, packageGraph);
      }
       /// However if bound is null, we don't assign any value to _boundType
    }
   /// and here we return null.
    return _boundType;
  }

This is causing a ton of errors in pub.dev packages. We can take, for example, the most liked package from pub.dev:
https://pub.dev/packages/get/score

Unhandled exception:
NoSuchMethodError: The getter 'name' was called on null.
Receiver: null
Tried calling: name

This is for the reason mentioned above. The problem is that this isn't the only place we have if without elses delivering null, and the solution to that would be to migrate to null-safety.

Is there a block for this? Otherwise I can do a PR with the migration.

@jcollins-g
Copy link
Contributor

the NNBD migration is ongoing in the nnbd branch on dartdoc. I have just started this process.

@jcollins-g jcollins-g added type-enhancement A request for a change that isn't a bug P1 A high priority bug; for example, a single project is unusable or has many test failures labels Aug 18, 2021
@jcollins-g
Copy link
Contributor

@jonataslaw If you wish to contribute one or more library migrations to the NNBD migration effort, that would be welcome.

Please see #2748 for a simple example of a migration, and target it for the nnbd branch as that PR is targeted, and if possible please announce here which library you intend to migrate as to avoid duplication of effort.

Note that "migrating" in this case may require more extensive API changes, some of which can and probably should be done before the actual migration, e.g. #2745.

@jcollins-g
Copy link
Contributor

my status today: migrating dartdoc_options.dart. Next in the hopper for me after that is likely to be package_meta.dart and/or io_utils.dart -- the strategy I am pursuing is a "bottom up" migration, migrating things that are depended on before migrating things with large numbers of dependencies.

@jcollins-g
Copy link
Contributor

looking at io_utils.dart, utils.dart, and package_meta.dart now.

@bkonyi
Copy link
Contributor

bkonyi commented Dec 17, 2021

@jcollins-g do we have any update on this effort? I'd like to get the CLI migrated to null safety sooner than later and dartdoc is the only dependency that I'm blocked on currently.

@srawlins
Copy link
Member

Complete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 A high priority bug; for example, a single project is unusable or has many test failures type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

4 participants