Skip to content

Add checks for 'late' initialized finals in dartdoc and clean up #2069

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

Merged
merged 1 commit into from
Nov 13, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions analysis_options.yaml
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
include: package:pedantic/analysis_options.1.8.0.yaml

analyzer:
# enable for analysis on test package sources.
enable-experiment:
- extension-methods
exclude:
- 'doc/**'
- 'lib/src/third_party/pkg/**'
Expand Down
4 changes: 4 additions & 0 deletions lib/src/model/field.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import 'package:analyzer/dart/element/element.dart';
import 'package:analyzer/src/dart/element/element.dart';
import 'package:dartdoc/src/element_type.dart';
import 'package:dartdoc/src/model/model.dart';

class Field extends ModelElement
Expand Down Expand Up @@ -184,6 +185,9 @@ class Field extends ModelElement
}
}

@override
CallableElementType get modelType => super.modelType;

@override
Inheritable get overriddenElement => null;
}
3 changes: 1 addition & 2 deletions lib/src/model/package_builder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,7 @@ class PackageBuilder {
AnalysisOptionsImpl options = AnalysisOptionsImpl();

// TODO(jcollins-g): pass in an ExperimentStatus instead?
options.enabledExperiments = config.enableExperiment
..add('extension-methods');
options.enabledExperiments = config.enableExperiment;

// TODO(jcollins-g): Make use of currently not existing API for managing
// many AnalysisDrivers
Expand Down
4 changes: 2 additions & 2 deletions pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ author: Dart Team <[email protected]>
description: A documentation generator for Dart.
homepage: https://github.com/dart-lang/dartdoc
environment:
sdk: '>=2.2.0 <3.0.0'
sdk: '>=2.6.0 <3.0.0'

dependencies:
analyzer: ^0.39.0
analyzer: ^0.39.1
args: '>=1.5.0 <2.0.0'
collection: ^1.2.0
crypto: ^2.0.6
Expand Down
13 changes: 9 additions & 4 deletions test/dartdoc_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,9 @@ void main() {
expect(p.name, 'test_package');
expect(p.hasDocumentationFile, isTrue);
// Total number of public libraries in test_package.
expect(packageGraph.defaultPackage.publicLibraries, hasLength(16));
// +2 since we are not manually excluding anything.
expect(packageGraph.defaultPackage.publicLibraries,
hasLength(kTestPackagePublicLibraries + 2));
expect(packageGraph.localPackages.length, equals(1));
});

Expand Down Expand Up @@ -313,8 +315,8 @@ void main() {
PackageGraph p = results.packageGraph;
expect(p.defaultPackage.name, 'test_package');
expect(p.defaultPackage.hasDocumentationFile, isTrue);
expect(p.libraries, hasLength(1));
expect(p.libraries.map((lib) => lib.name), contains('fake'));
expect(p.localPublicLibraries, hasLength(1));
expect(p.localPublicLibraries.map((lib) => lib.name), contains('fake'));
});

test('generate docs excluding a single library', () async {
Expand All @@ -327,7 +329,10 @@ void main() {
PackageGraph p = results.packageGraph;
expect(p.defaultPackage.name, 'test_package');
expect(p.defaultPackage.hasDocumentationFile, isTrue);
expect(p.localPublicLibraries, hasLength(15));
// Plus 1 here because we're excluding only two libraries (the specified
// one and the <nodoc> 'excluded' library) instead of three.
expect(
p.localPublicLibraries, hasLength(kTestPackagePublicLibraries + 1));
expect(p.localPublicLibraries.map((lib) => lib.name).contains('fake'),
isFalse);
});
Expand Down
59 changes: 18 additions & 41 deletions test/model_special_cases_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -29,49 +29,26 @@ void main() {
// Experimental features not yet enabled by default. Move tests out of this block
// when the feature is enabled by default.
group('Experiments', () {
Library main;
TopLevelVariable aComplexSet,
inferredTypeSet,
specifiedSet,
untypedMap,
typedSet;

Library lateFinalWithoutInitializer;
setUpAll(() async {
main = (await utils.testPackageGraphExperiments)
lateFinalWithoutInitializer = (await utils.testPackageGraphExperiments)
.libraries
.firstWhere((lib) => lib.name == 'main');
aComplexSet = main.constants.firstWhere((v) => v.name == 'aComplexSet');
inferredTypeSet =
main.constants.firstWhere((v) => v.name == 'inferredTypeSet');
specifiedSet = main.constants.firstWhere((v) => v.name == 'specifiedSet');
untypedMap = main.constants.firstWhere((v) => v.name == 'untypedMap');
typedSet = main.constants.firstWhere((v) => v.name == 'typedSet');
});

test('Set literals test', () {
expect(aComplexSet.modelType.name, equals('Set'));
expect(aComplexSet.modelType.typeArguments.map((a) => a.name).toList(),
equals(['AClassContainingLiterals']));
expect(aComplexSet.constantValue,
equals('const {const AClassContainingLiterals(3, 5)}'));
expect(inferredTypeSet.modelType.name, equals('Set'));
expect(
inferredTypeSet.modelType.typeArguments.map((a) => a.name).toList(),
equals(['num']));
expect(inferredTypeSet.constantValue, equals('const {1, 2.5, 3}'));
expect(specifiedSet.modelType.name, equals('Set'));
expect(specifiedSet.modelType.typeArguments.map((a) => a.name).toList(),
equals(['int']));
expect(specifiedSet.constantValue, equals('const {}'));
expect(untypedMap.modelType.name, equals('Map'));
expect(untypedMap.modelType.typeArguments.map((a) => a.name).toList(),
equals(['dynamic', 'dynamic']));
expect(untypedMap.constantValue, equals('const {}'));
expect(typedSet.modelType.name, equals('Set'));
expect(typedSet.modelType.typeArguments.map((a) => a.name).toList(),
equals(['String']));
expect(typedSet.constantValue,
matches(RegExp(r'const &lt;String&gt;\s?{}')));
.firstWhere((lib) => lib.name == 'late_final_without_initializer');
});

test('Late finals test', () {
Class c = lateFinalWithoutInitializer.allClasses
.firstWhere((c) => c.name == 'C');
Field a = c.allFields.firstWhere((f) => f.name == 'a');
Field b = c.allFields.firstWhere((f) => f.name == 'b');
Field cField = c.allFields.firstWhere((f) => f.name == 'cField');
Field dField = c.allFields.firstWhere((f) => f.name == 'dField');
// If nnbd isn't enabled, fields named 'late' come back from the analyzer.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it useful to test for late when it is invalid to use?
I expected to see a NNBD enabled test, with late displayed as a modifier.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I this the test is what you expected, and that this is just ensuring that the library was analyzed correctly as being opted in. The comment indicates what analyzer would have produced if NNBD were not enabled, and hence what's being tested for.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the late initialization of a field matters enough to mention it from the perspective of the API of a class -- though even as I say that I'm starting to wonder if that's true. I think it would usually be bad design to declare as a final field something you wanted users of the class to initialize externally, but I can think of some cases where people might desire that. My current thought is to tell users who do that to explicitly describe what's necessary in the doc for the field.

expect(c.allFields.any((f) => f.name == 'late'), isFalse);
expect(a.modelType.returnType.name, equals('dynamic'));
expect(b.modelType.returnType.name, equals('int'));
expect(cField.modelType.returnType.name, equals('dynamic'));
expect(dField.modelType.returnType.name, equals('double'));
});
});

Expand Down
91 changes: 73 additions & 18 deletions test/model_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,55 @@ void main() {
packageGraph.libraries.firstWhere((lib) => lib.name == 'base_class');
});

group('Set Literals', () {
Library set_literals;
TopLevelVariable aComplexSet,
inferredTypeSet,
specifiedSet,
untypedMap,
typedSet;

setUpAll(() async {
set_literals = packageGraph.libraries
.firstWhere((lib) => lib.name == 'set_literals');
aComplexSet =
set_literals.constants.firstWhere((v) => v.name == 'aComplexSet');
inferredTypeSet =
set_literals.constants.firstWhere((v) => v.name == 'inferredTypeSet');
specifiedSet =
set_literals.constants.firstWhere((v) => v.name == 'specifiedSet');
untypedMap =
set_literals.constants.firstWhere((v) => v.name == 'untypedMap');
typedSet = set_literals.constants.firstWhere((v) => v.name == 'typedSet');
});

test('Set literals test', () {
expect(aComplexSet.modelType.name, equals('Set'));
expect(aComplexSet.modelType.typeArguments.map((a) => a.name).toList(),
equals(['AClassContainingLiterals']));
expect(aComplexSet.constantValue,
equals('const {const AClassContainingLiterals(3, 5)}'));
expect(inferredTypeSet.modelType.name, equals('Set'));
expect(
inferredTypeSet.modelType.typeArguments.map((a) => a.name).toList(),
equals(['num']));
expect(inferredTypeSet.constantValue, equals('const {1, 2.5, 3}'));
expect(specifiedSet.modelType.name, equals('Set'));
expect(specifiedSet.modelType.typeArguments.map((a) => a.name).toList(),
equals(['int']));
expect(specifiedSet.constantValue, equals('const {}'));
expect(untypedMap.modelType.name, equals('Map'));
expect(untypedMap.modelType.typeArguments.map((a) => a.name).toList(),
equals(['dynamic', 'dynamic']));
expect(untypedMap.constantValue, equals('const {}'));
expect(typedSet.modelType.name, equals('Set'));
expect(typedSet.modelType.typeArguments.map((a) => a.name).toList(),
equals(['String']));
expect(typedSet.constantValue,
matches(RegExp(r'const &lt;String&gt;\s?{}')));
});
});

group('Tools', () {
Class toolUser;
Class _NonCanonicalToolUser, CanonicalToolUser, PrivateLibraryToolUser;
Expand Down Expand Up @@ -291,10 +340,6 @@ void main() {
]));
expect(packageCategories.map((c) => c.libraries.length).toList(),
orderedEquals([0, 2, 3, 1, 0, 0]));
expect(
packageGraph
.localPackages.first.defaultCategory.publicLibraries.length,
equals(9));
});

test('Verify libraries with multiple categories show up in multiple places',
Expand All @@ -313,7 +358,8 @@ void main() {
expect(
packageGraph
.localPackages.first.defaultCategory.publicLibraries.length,
equals(9));
// Only 5 libraries have categories, the rest belong in default.
equals(utils.kTestPackagePublicLibraries - 5));
});
});

Expand Down Expand Up @@ -383,7 +429,8 @@ void main() {
});

test('libraries', () {
expect(packageGraph.localPublicLibraries, hasLength(14));
expect(packageGraph.localPublicLibraries,
hasLength(utils.kTestPackagePublicLibraries));
expect(interceptorsLib.isPublic, isFalse);
});

Expand All @@ -398,7 +445,8 @@ void main() {

Package package = packageGraph.localPackages.first;
expect(package.name, 'test_package');
expect(package.publicLibraries, hasLength(14));
expect(package.publicLibraries,
hasLength(utils.kTestPackagePublicLibraries));
});

test('is documented in library', () {
Expand Down Expand Up @@ -710,11 +758,12 @@ void main() {
EnumField enumValue2;

setUpAll(() {
enumWithAnimation = exLibrary.enums.firstWhere((c) => c.name == 'EnumWithAnimation');
enumValue1 = enumWithAnimation.constants
.firstWhere((m) => m.name == 'value1');
enumValue2 = enumWithAnimation.constants
.firstWhere((m) => m.name == 'value2');
enumWithAnimation =
exLibrary.enums.firstWhere((c) => c.name == 'EnumWithAnimation');
enumValue1 =
enumWithAnimation.constants.firstWhere((m) => m.name == 'value1');
enumValue2 =
enumWithAnimation.constants.firstWhere((m) => m.name == 'value2');
dog = exLibrary.classes.firstWhere((c) => c.name == 'Dog');
withAnimation =
dog.allInstanceMethods.firstWhere((m) => m.name == 'withAnimation');
Expand Down Expand Up @@ -775,12 +824,18 @@ void main() {
contains('<video id="outOfOrder"'));
});
test("Enum field animation identifiers are unique.", () {
expect(enumValue1.documentationAsHtml, contains('<video id="animation_1"'));
expect(enumValue1.documentationAsHtml, contains('<video id="animation_2"'));
expect(enumValue2.documentationAsHtml, isNot(contains('<video id="animation_1"')));
expect(enumValue2.documentationAsHtml, isNot(contains('<video id="animation_2"')));
expect(enumValue2.documentationAsHtml, contains('<video id="animation_3"'));
expect(enumValue2.documentationAsHtml, contains('<video id="animation_4"'));
expect(
enumValue1.documentationAsHtml, contains('<video id="animation_1"'));
expect(
enumValue1.documentationAsHtml, contains('<video id="animation_2"'));
expect(enumValue2.documentationAsHtml,
isNot(contains('<video id="animation_1"')));
expect(enumValue2.documentationAsHtml,
isNot(contains('<video id="animation_2"')));
expect(
enumValue2.documentationAsHtml, contains('<video id="animation_3"'));
expect(
enumValue2.documentationAsHtml, contains('<video id="animation_4"'));
});
});

Expand Down
12 changes: 8 additions & 4 deletions test/src/utils.dart
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ import 'package:dartdoc/src/model/model.dart';
import 'package:dartdoc/src/package_meta.dart';
import 'package:path/path.dart' as path;

/// The number of public libraries in testing/test_package, minus 2 for
/// the excluded libraries listed in the initializers for _testPackageGraphMemo
/// and minus 1 for the <nodoc> tag in the 'excluded' library.
const int kTestPackagePublicLibraries = 15;

final RegExp quotables = RegExp(r'[ "\r\n\$]');
final RegExp observatoryPortRegexp =
RegExp(r'^Observatory listening on http://.*:(\d+)');
Expand All @@ -23,15 +28,14 @@ Directory sdkDir = defaultSdkDir;
PackageMeta sdkPackageMeta = PackageMeta.fromDir(sdkDir);

final _testPackageGraphMemo = AsyncMemoizer<PackageGraph>();
Future<PackageGraph> get testPackageGraph =>
_testPackageGraphMemo.runOnce(() => bootBasicPackage(
'testing/test_package', ['css', 'code_in_comments', 'excluded']));
Future<PackageGraph> get testPackageGraph => _testPackageGraphMemo.runOnce(() =>
bootBasicPackage('testing/test_package', ['css', 'code_in_comments']));

final _testPackageGraphExperimentsMemo = AsyncMemoizer<PackageGraph>();
Future<PackageGraph> get testPackageGraphExperiments =>
_testPackageGraphExperimentsMemo.runOnce(() => bootBasicPackage(
'testing/test_package_experiments', [],
additionalArguments: ['--enable-experiment', 'set-literals']));
additionalArguments: ['--enable-experiment', 'non-nullable']));

final _testPackageGraphGinormousMemo = AsyncMemoizer<PackageGraph>();
Future<PackageGraph> get testPackageGraphGinormous =>
Expand Down
4 changes: 4 additions & 0 deletions testing/test_package_experiments/analysis_options.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
analyzer:
# enable for analysis on test package sources.
enable-experiment:
- non-nullable
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Copyright (c) 2019, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

import 'dart:math';

class C {
late final a;
late final b = 0;
late final cField;
late final double dField;

C(double param) {
cField = param * 3.14;
dField = param * 8.854 * pow(10, -12);
}
}