From 3b6c1d72cd0e38a8c28e8574fa62d541a66cceba Mon Sep 17 00:00:00 2001 From: Sam Rawlins Date: Wed, 15 Jul 2020 16:17:27 -0700 Subject: [PATCH 1/2] Respect allowed experiments list, via Analyzer's APIs. Analyzer's LibraryElement.isNonNullableByDefault has already taken allowed experiments into account. Additionally, rewrite all code and comments which refer to "NNBD" to instead refer to "Null safety". --- analysis_options.yaml | 1 + lib/src/element_type.dart | 15 +++++----- lib/src/model/feature_set.dart | 4 +-- lib/src/model/library.dart | 17 +++++------ lib/src/package_meta.dart | 47 ------------------------------ test/model_special_cases_test.dart | 47 +++++++++++++++--------------- 6 files changed, 42 insertions(+), 89 deletions(-) diff --git a/analysis_options.yaml b/analysis_options.yaml index d7d1a23a47..d20e7335a0 100644 --- a/analysis_options.yaml +++ b/analysis_options.yaml @@ -6,6 +6,7 @@ analyzer: errors: unused_import: warning unused_shown_name: warning + todo: ignore exclude: - 'doc/**' - 'lib/src/third_party/pkg/**' diff --git a/lib/src/element_type.dart b/lib/src/element_type.dart index 2ab5fa0021..fc0b8408bb 100644 --- a/lib/src/element_type.dart +++ b/lib/src/element_type.dart @@ -79,9 +79,9 @@ abstract class ElementType extends Privacy { /// Return a dartdoc nullability suffix for this type. String get nullabilitySuffix { - if (library.isNNBD && !type.isVoid && !type.isBottom) { - /// If a legacy type appears inside the public interface of a - /// NNBD library, we pretend it is nullable for the purpose of + if (library.isNullSafety && !type.isVoid && !type.isBottom) { + /// If a legacy type appears inside the public interface of a Null + /// safety library, we pretend it is nullable for the purpose of /// documentation (since star-types are not supposed to be public). if (type.nullabilitySuffix == NullabilitySuffix.question || type.nullabilitySuffix == NullabilitySuffix.star) { @@ -406,11 +406,12 @@ abstract class CallableElementTypeMixin implements ParameterizedElementType { /// Return the [TypeParameterType] with the legacy nullability for the given /// type parameter [element]. /// - /// TODO(scheglov) This method is a work around that fact that DartDoc + /// TODO(scheglov): This method is a work around that fact that DartDoc /// currently represents both type formals and uses of them as actual types, - /// as [TypeParameterType]s. This was not perfect, but worked before NNBD. - /// With NNBD types have nullability suffixes, but type formals should not. - /// Eventually we should separate models for type formals and types. + /// as [TypeParameterType]s. This was not perfect, but worked before Null + /// safety. With Null safety, types have nullability suffixes, but type + /// formals should not. Eventually we should separate models for type formals + /// and types. static TypeParameterType _legacyTypeParameterType( TypeParameterElement element, ) { diff --git a/lib/src/model/feature_set.dart b/lib/src/model/feature_set.dart index acfeb1d1ad..65fb3f32eb 100644 --- a/lib/src/model/feature_set.dart +++ b/lib/src/model/feature_set.dart @@ -16,7 +16,7 @@ mixin FeatureSet { Iterable get displayedLanguageFeatures sync* { // TODO(jcollins-g): Implement mixed-mode handling and the tagging of // legacy interfaces. - if (isNNBD) { + if (isNullSafety) { yield LanguageFeature( 'Null safety', packageGraph.rendererFactory.featureRenderer); } @@ -26,5 +26,5 @@ mixin FeatureSet { // TODO(jcollins-g): This is an approximation and not strictly true for // inheritance/reexports. - bool get isNNBD => library.isNNBD; + bool get isNullSafety => library.isNullSafety; } diff --git a/lib/src/model/library.dart b/lib/src/model/library.dart index a60e69ab71..d8c42a16dc 100644 --- a/lib/src/model/library.dart +++ b/lib/src/model/library.dart @@ -108,17 +108,14 @@ class Library extends ModelElement with Categorization, TopLevelContainer { List _allOriginalModelElementNames; /// Return true if this library is in a package configured to be treated as - /// as non-nullable by default and is itself NNBD. - // TODO(jcollins-g): packageMeta.allowsNNBD may be redundant after package - // allow list support is published in analyzer - bool get _allowsNNBD => - element.isNonNullableByDefault && packageMeta.allowsNNBD; - - /// Return true if this library should be documented as non-nullable. - /// A library may be NNBD but not documented that way. + /// as using Null safety and itself uses Null safety. + bool get _allowsNullSafety => element.isNonNullableByDefault; + + /// Return true if this library should be documented as using Null safety. + /// A library may use Null safety but not documented that way. @override - bool get isNNBD => - config.enableExperiment.contains('non-nullable') && _allowsNNBD; + bool get isNullSafety => + config.enableExperiment.contains('non-nullable') && _allowsNullSafety; bool get isInSdk => element.isInSdk; diff --git a/lib/src/package_meta.dart b/lib/src/package_meta.dart index 1a66f5e283..d776ef4ad3 100644 --- a/lib/src/package_meta.dart +++ b/lib/src/package_meta.dart @@ -10,7 +10,6 @@ import 'dart:io'; import 'package:analyzer/dart/element/element.dart'; import 'package:dartdoc/dartdoc.dart'; import 'package:path/path.dart' as path; -import 'package:pub_semver/pub_semver.dart'; import 'package:yaml/yaml.dart'; import 'logging.dart'; @@ -107,14 +106,6 @@ abstract class PackageMeta { String get homepage; - /// If true, libraries in this package can be be read as non-nullable by - /// default. Whether they will be will be documented that way will depend - /// on the experiment flag. - /// - /// A package property, as this depends in part on the pubspec version - /// constraint and/or the package allow list. - bool get allowsNNBD; - FileContents getReadmeContents(); FileContents getLicenseContents(); @@ -281,7 +272,6 @@ class _FilePackageMeta extends PubPackageMeta { FileContents _license; FileContents _changelog; Map _pubspec; - Map _analysisOptions; _FilePackageMeta(Directory dir) : super(dir) { var f = File(path.join(dir.path, 'pubspec.yaml')); @@ -290,12 +280,6 @@ class _FilePackageMeta extends PubPackageMeta { } else { _pubspec = {}; } - f = File(path.join(dir.path, 'analysis_options.yaml')); - if (f.existsSync()) { - _analysisOptions = loadYaml(f.readAsStringSync()); - } else { - _analysisOptions = {}; - } } bool _setHostedAt = false; @@ -376,29 +360,6 @@ class _FilePackageMeta extends PubPackageMeta { @override String get homepage => _pubspec['homepage']; - /// This is a magic range that triggers detection of [allowsNNBD]. - static final _nullableRange = - VersionConstraint.parse('>=2.9.0-dev.0 <2.10.0') as VersionRange; - - @override - bool get allowsNNBD { - // TODO(jcollins-g): override/add to with allow list once that exists - var sdkConstraint; - if (_pubspec?.containsKey('sdk') ?? false) { - // TODO(jcollins-g): VersionConstraint.parse returns [VersionRange]s right - // now, but the interface doesn't guarantee that. - sdkConstraint = VersionConstraint.parse(_pubspec['sdk']) as VersionRange; - } - if (sdkConstraint == _nullableRange && - (_analysisOptions['analyzer']?.containsKey('enable-experiment') ?? - false) && - _analysisOptions['analyzer']['enable-experiment'] - .contains('non-nullable')) { - return true; - } - return false; - } - @override bool get requiresFlutter => _pubspec['environment']?.containsKey('flutter') == true || @@ -462,14 +423,6 @@ class _SdkMeta extends PubPackageMeta { sdkReadmePath = path.join(dir.path, 'lib', 'api_readme.md'); } - /// 2.9.0-9.0.dev is the first unforked SDK, and therefore the first version - /// of the SDK it makes sense to allow NNBD documentation for. - static final _sdkNullableRange = VersionConstraint.parse('>=2.9.0-9.0.dev'); - - @override - // TODO(jcollins-g): There should be a better way to determine this. - bool get allowsNNBD => _sdkNullableRange.allows(Version.parse(version)); - @override String get hostedAt => null; diff --git a/test/model_special_cases_test.dart b/test/model_special_cases_test.dart index 99687834f2..8f1666c06b 100644 --- a/test/model_special_cases_test.dart +++ b/test/model_special_cases_test.dart @@ -30,19 +30,19 @@ void main() { exit(1); } - // This doesn't have the `max` because NNBD is supposed to work after this - // version, and if the `max` is placed here we'll silently pass 2.10 stable - // if we haven't figured out how to switch on NNBD outside of `dev` builds - // as specified in #2148. - final _nnbdExperimentAllowed = + // This doesn't have the `max` because Null safety is supposed to work after + // this version, and if the `max` is placed here we'll silently pass 2.10 + // stable if we haven't figured out how to switch on Null safety outside of + // dev builds as specified in #2148. + final _nullSafetyExperimentAllowed = VersionRange(min: Version.parse('2.9.0-9.0.dev'), includeMin: true); - // Experimental features not yet enabled by default. Move tests out of this block - // when the feature is enabled by default. + // Experimental features not yet enabled by default. Move tests out of this + // block when the feature is enabled by default. group('Experiments', () { Library lateFinalWithoutInitializer, - nnbdClassMemberDeclarations, - optOutOfNnbd, + nullSafetyClassMemberDeclarations, + optOutOfNullSafety, nullableElements; Class b; @@ -50,22 +50,23 @@ void main() { lateFinalWithoutInitializer = (await utils.testPackageGraphExperiments) .libraries .firstWhere((lib) => lib.name == 'late_final_without_initializer'); - nnbdClassMemberDeclarations = (await utils.testPackageGraphExperiments) + nullSafetyClassMemberDeclarations = (await utils + .testPackageGraphExperiments) .libraries .firstWhere((lib) => lib.name == 'nnbd_class_member_declarations'); - optOutOfNnbd = (await utils.testPackageGraphExperiments) + optOutOfNullSafety = (await utils.testPackageGraphExperiments) .libraries .firstWhere((lib) => lib.name == 'opt_out_of_nnbd'); nullableElements = (await utils.testPackageGraphExperiments) .libraries .firstWhere((lib) => lib.name == 'nullable_elements'); - b = nnbdClassMemberDeclarations.allClasses + b = nullSafetyClassMemberDeclarations.allClasses .firstWhere((c) => c.name == 'B'); }); - test('isNNBD is set correctly for libraries', () { - expect(lateFinalWithoutInitializer.isNNBD, isTrue); - expect(optOutOfNnbd.isNNBD, isFalse); + test('isNullSafety is set correctly for libraries', () { + expect(lateFinalWithoutInitializer.isNullSafety, isTrue); + expect(optOutOfNullSafety.isNullSafety, isFalse); }); test('method parameters with required', () { @@ -118,8 +119,8 @@ void main() { var cField = c.instanceFields.firstWhere((f) => f.name == 'cField'); var dField = c.instanceFields.firstWhere((f) => f.name == 'dField'); - // If nnbd isn't enabled, fields named 'late' come back from the analyzer - // instead of setting up 'isLate'. + // If Null safety isn't enabled, fields named 'late' come back from the + // analyzer instead of setting up 'isLate'. expect(c.instanceFields.any((f) => f.name == 'late'), isFalse); expect(a.modelType.returnType.name, equals('dynamic')); @@ -147,10 +148,10 @@ void main() { expect(initializeMe.features, contains('late')); }); - test('Opt out of NNBD', () { - var notOptedIn = optOutOfNnbd.publicProperties + test('Opt out of Null safety', () { + var notOptedIn = optOutOfNullSafety.publicProperties .firstWhere((v) => v.name == 'notOptedIn'); - expect(notOptedIn.isNNBD, isFalse); + expect(notOptedIn.isNullSafety, isFalse); expect(notOptedIn.modelType.nullabilitySuffix, isEmpty); }); @@ -161,7 +162,7 @@ void main() { .firstWhere((f) => f.name == 'aComplexType'); var aComplexSetterOnlyType = complexNullableMembers.allFields .firstWhere((f) => f.name == 'aComplexSetterOnlyType'); - expect(complexNullableMembers.isNNBD, isTrue); + expect(complexNullableMembers.isNullSafety, isTrue); expect( complexNullableMembers.nameWithGenerics, equals( @@ -186,7 +187,7 @@ void main() { .firstWhere((f) => f.name == 'methodWithNullables'); var operatorStar = nullableMembers.publicInstanceOperators .firstWhere((f) => f.name == 'operator *'); - expect(nullableMembers.isNNBD, isTrue); + expect(nullableMembers.isNullSafety, isTrue); expect( nullableField.linkedReturnType, equals( @@ -206,7 +207,7 @@ void main() { 'NullableMembers? nullableOther')); }); }, - skip: (!_nnbdExperimentAllowed.allows(_platformVersion) && + skip: (!_nullSafetyExperimentAllowed.allows(_platformVersion) && !_platformVersionString.contains('edge'))); group('HTML Injection when allowed', () { From 5156d41d3c9ebb6bd8783b462ebbd58070d96420 Mon Sep 17 00:00:00 2001 From: Sam Rawlins Date: Wed, 15 Jul 2020 16:21:32 -0700 Subject: [PATCH 2/2] Oops TODO --- analysis_options.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/analysis_options.yaml b/analysis_options.yaml index d20e7335a0..d7d1a23a47 100644 --- a/analysis_options.yaml +++ b/analysis_options.yaml @@ -6,7 +6,6 @@ analyzer: errors: unused_import: warning unused_shown_name: warning - todo: ignore exclude: - 'doc/**' - 'lib/src/third_party/pkg/**'