Skip to content

Prevent private or non-canonical elements from ever generating links #1589

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.

Sign up for GitHub

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 5 commits into from
Jan 11, 2018
Merged

Prevent private or non-canonical elements from ever generating links #1589

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7aa8754
First version
jcollins-g Jan 10, 2018
733bf0a
dartfmt
jcollins-g Jan 10, 2018
5e20b0c
Add a test
jcollins-g Jan 10, 2018
4137e0a
Merge branch 'master' into fix-const-private-classes
jcollins-g Jan 11, 2018
f32a8c8
Merge branch 'master' into fix-const-private-classes
jcollins-g Jan 11, 2018
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
2 changes: 2 additions & 0 deletions lib/src/element_type.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ class ElementType extends Privacy {

DartType get type => _type;

Library get library => element.library;

bool get isDynamic => _type.isDynamic;

bool get isFunctionType => (_type is FunctionType);
Expand Down
149 changes: 92 additions & 57 deletions lib/src/model.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,13 @@ abstract class Inheritable implements ModelElement {
ModelElement get definingEnclosingElement =>
_memoizer.memoized(_definingEnclosingElement);

ModelElement _canonicalEnclosingElement() {
@override
ModelElement _canonicalModelElement() {
return canonicalEnclosingElement?.allCanonicalModelElements
?.firstWhere((m) => m.name == name, orElse: () => null);
}

Class _canonicalEnclosingElement() {
Class canonicalEnclosingClass;
Element searchElement = element;
if (isInherited) {
Expand Down Expand Up @@ -180,7 +186,7 @@ abstract class Inheritable implements ModelElement {
return canonicalEnclosingClass;
}

ModelElement get canonicalEnclosingElement =>
Class get canonicalEnclosingElement =>
_memoizer.memoized(_canonicalEnclosingElement);

List<Class> get inheritance {
Expand Down Expand Up @@ -608,6 +614,7 @@ class Class extends ModelElement
@override
ModelElement get enclosingElement => library;

@override
String get fileName => "${name}-class.html";

String get fullkind {
Expand Down Expand Up @@ -654,8 +661,11 @@ class Class extends ModelElement

@override
String get href {
if (canonicalLibrary == null) return null;
return '${canonicalLibrary.dirName}/$fileName';
if (!identical(canonicalModelElement, this))
Copy link
Member

Choose a reason for hiding this comment

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

nit: braces for the if statement body

Copy link
Member

Choose a reason for hiding this comment

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

Just for clarity, if this element is not the canonical one, we generate a link to the canonical one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. While you could infer that from the code previously with all the "canonicalEnclosingElement" stuff, the modification here is intended to make that more clear and to make the object being linked to actually responsible for its own link generation.

Most of the broken link bugs I've seen either were attributable to bugs in how this worked, or were made harder to fix because of it (like this one). Now elements no longer point blindly into the filesystem hoping that the model really has a canonical element that matches, but we actually look up the element.

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've got a second change on deck to eliminate the copypasta of hrefs everywhere; if possible I'd prefer to fix the braces there.

Copy link
Member

Choose a reason for hiding this comment

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

sgtm

return canonicalModelElement?.href;
assert(canonicalLibrary != null);
assert(canonicalLibrary == library);
return '${library.dirName}/$fileName';
}

/// Returns all the implementors of this class.
Expand Down Expand Up @@ -1132,8 +1142,11 @@ class Constructor extends ModelElement

@override
String get href {
if (canonicalLibrary == null) return null;
return '${canonicalLibrary.dirName}/${_constructor.enclosingElement.name}/$name.html';
if (!identical(canonicalModelElement, this))
return canonicalModelElement?.href;
Copy link
Member

Choose a reason for hiding this comment

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

here as well, and below

assert(canonicalLibrary != null);
assert(canonicalLibrary == library);
return '${enclosingElement.library.dirName}/${enclosingElement.name}/$name.html';
}

@override
Expand Down Expand Up @@ -1346,9 +1359,12 @@ class EnumField extends Field {

@override
String get href {
if (canonicalLibrary == null || canonicalEnclosingElement == null)
return null;
return '${canonicalEnclosingElement.library.dirName}/${(canonicalEnclosingElement as Class).fileName}';
if (!identical(canonicalModelElement, this))
return canonicalModelElement?.href;
assert(!(canonicalLibrary == null || canonicalEnclosingElement == null));
assert(canonicalLibrary == library);
assert(canonicalEnclosingElement == enclosingElement);
return '${enclosingElement.library.dirName}/${(enclosingElement as Class).fileName}';
}

@override
Expand Down Expand Up @@ -1422,19 +1438,12 @@ class Field extends ModelElement

@override
String get href {
String retval;
if (canonicalLibrary == null) return null;
if (enclosingElement is Class) {
if (canonicalEnclosingElement == null) return null;
retval =
'${canonicalEnclosingElement.canonicalLibrary.dirName}/${canonicalEnclosingElement.name}/$_fileName';
} else if (enclosingElement is Library) {
retval = '${canonicalLibrary.dirName}/$_fileName';
} else {
throw new StateError(
'$name is not in a class or library, instead it is a ${enclosingElement.element}');
}
return retval;
if (!identical(canonicalModelElement, this))
return canonicalModelElement?.href;
assert(canonicalLibrary != null);
assert(canonicalEnclosingElement == enclosingElement);
assert(canonicalLibrary == library);
return '${enclosingElement.library.dirName}/${enclosingElement.name}/$fileName';
}

@override
Expand Down Expand Up @@ -1494,7 +1503,8 @@ class Field extends ModelElement

FieldElement get _field => (element as FieldElement);

String get _fileName => isConst ? '$name-constant.html' : '$name.html';
@override
String get fileName => isConst ? '$name-constant.html' : '$name.html';

String _sourceCode() {
// We could use a set to figure the dupes out, but that would lose ordering.
Expand Down Expand Up @@ -1906,6 +1916,7 @@ class Library extends ModelElement {

Iterable<Class> get publicExceptions => filterNonPublic(exceptions);

@override
String get fileName => '$dirName-library.html';

List<ModelFunction> _functions() {
Expand Down Expand Up @@ -1943,8 +1954,9 @@ class Library extends ModelElement {

@override
String get href {
if (canonicalLibrary == null) return null;
return '${canonicalLibrary.dirName}/$fileName';
if (!identical(canonicalModelElement, this))
return canonicalModelElement?.href;
return '${library.dirName}/$fileName';
}

InheritanceManager _inheritanceManager() => new InheritanceManager(element);
Expand Down Expand Up @@ -2275,18 +2287,19 @@ class Method extends ModelElement
return _enclosingClass;
}

String get fileName => "${name}.html";

String get fullkind {
if (_method.isAbstract) return 'abstract $kind';
return kind;
}

@override
String get href {
if (canonicalLibrary == null || canonicalEnclosingElement == null)
return null;
return '${canonicalEnclosingElement.canonicalLibrary.dirName}/${canonicalEnclosingElement.name}/${fileName}';
if (!identical(canonicalModelElement, this))
return canonicalModelElement?.href;
assert(!(canonicalLibrary == null || canonicalEnclosingElement == null));
assert(canonicalLibrary == library);
assert(canonicalEnclosingElement == enclosingElement);
return '${enclosingElement.library.dirName}/${enclosingElement.name}/${fileName}';
}

@override
Expand Down Expand Up @@ -2693,6 +2706,20 @@ abstract class ModelElement extends Canonicalization
bool get canHaveParameters =>
element is ExecutableElement || element is FunctionTypedElement;

ModelElement _canonicalModelElement() {
Class preferredClass;
if (enclosingElement is Class) {
preferredClass = enclosingElement;
}
return package.findCanonicalModelElementFor(element,
preferredClass: preferredClass);
}

// Returns the canonical ModelElement for this ModelElement, or null
// if there isn't one.
ModelElement get canonicalModelElement =>
_memoizer.memoized(_canonicalModelElement);

// TODO(jcollins-g): untangle when mixins can call super
@override
List<ModelElement> get documentationFrom =>
Expand Down Expand Up @@ -2888,6 +2915,8 @@ abstract class ModelElement extends Canonicalization
return '';
}

String get fileName => "${name}.html";

/// Returns the fully qualified name.
///
/// For example: libraryName.className.methodName
Expand Down Expand Up @@ -3559,12 +3588,13 @@ class ModelFunctionTyped extends ModelElement
@override
ModelElement get enclosingElement => library;

String get fileName => "$name.html";

@override
String get href {
if (canonicalLibrary == null) return null;
return '${canonicalLibrary.dirName}/$fileName';
if (!identical(canonicalModelElement, this))
return canonicalModelElement?.href;
assert(canonicalLibrary != null);
assert(canonicalLibrary == library);
return '${library.dirName}/$fileName';
}

@override
Expand Down Expand Up @@ -4182,6 +4212,9 @@ class Package extends Canonicalization with Nameable, Warnable, Memoizeable {
if (e is PropertyAccessorElement) {
searchElement = e.variable;
}
if (e is GenericFunctionTypeElement) {
searchElement = e.enclosingElement;
}

for (Library library in publicLibraries) {
if (library.modelElementsMap.containsKey(searchElement)) {
Expand Down Expand Up @@ -4211,6 +4244,10 @@ class Package extends Canonicalization with Nameable, Warnable, Memoizeable {
/// Tries to find a canonical ModelElement for this element. If we know
/// this element is related to a particular class, pass preferredClass to
/// disambiguate.
///
/// This doesn't know anything about [Package.inheritThrough] and probably
/// shouldn't, so using it with [Inheritable]s without special casing is
/// not advised.
ModelElement findCanonicalModelElementFor(Element e, {Class preferredClass}) {
assert(allLibrariesAdded);
Library lib = findCanonicalLibraryFor(e);
Expand Down Expand Up @@ -4295,7 +4332,9 @@ class Package extends Canonicalization with Nameable, Warnable, Memoizeable {
}

assert(matches.length <= 1);
if (!matches.isEmpty) modelElement = matches.first;
if (matches.isNotEmpty) {
modelElement = matches.first;
}
} else {
if (lib != null) {
Accessor getter;
Expand Down Expand Up @@ -4431,19 +4470,7 @@ class Parameter extends ModelElement implements EnclosedElement {

@override
String get href {
if (canonicalLibrary == null) return null;
var p = _parameter.enclosingElement;

if (p is FunctionElement) {
return '${canonicalLibrary.dirName}/${p.name}.html';
} else {
// TODO: why is this logic here?
var name = Operator.friendlyNames.containsKey(p.name)
? Operator.friendlyNames[p.name]
: p.name;
return '${canonicalLibrary.dirName}/${p.enclosingElement.name}/' +
'${name}.html#${htmlId}';
}
throw new StateError('href not implemented for parameters');
}

@override
Expand Down Expand Up @@ -4636,8 +4663,11 @@ class TopLevelVariable extends ModelElement

@override
String get href {
if (canonicalLibrary == null) return null;
return '${canonicalLibrary.dirName}/$_fileName';
if (!identical(canonicalModelElement, this))
return canonicalModelElement?.href;
assert(canonicalLibrary != null);
assert(canonicalLibrary == library);
return '${library.dirName}/$fileName';
}

@override
Expand All @@ -4664,7 +4694,8 @@ class TopLevelVariable extends ModelElement
return docs;
}

String get _fileName => isConst ? '$name-constant.html' : '$name.html';
@override
String get fileName => isConst ? '$name-constant.html' : '$name.html';

TopLevelVariableElement get _variable => (element as TopLevelVariableElement);
}
Expand All @@ -4678,8 +4709,6 @@ class Typedef extends ModelElement
@override
ModelElement get enclosingElement => library;

String get fileName => '$name.html';

@override
String get nameWithGenerics => '$name${super.genericParameters}';

Expand All @@ -4697,8 +4726,11 @@ class Typedef extends ModelElement

@override
String get href {
if (canonicalLibrary == null) return null;
return '${canonicalLibrary.dirName}/$fileName';
if (!identical(canonicalModelElement, this))
return canonicalModelElement?.href;
assert(canonicalLibrary != null);
assert(canonicalLibrary == library);
return '${library.dirName}/$fileName';
}

// Food for mustache.
Expand Down Expand Up @@ -4730,8 +4762,11 @@ class TypeParameter extends ModelElement {

@override
String get href {
if (canonicalLibrary == null) return null;
return '${canonicalLibrary.dirName}/${_typeParameter.enclosingElement.name}/$name';
if (!identical(canonicalModelElement, this))
return canonicalModelElement?.href;
assert(canonicalLibrary != null);
assert(canonicalLibrary == library);
return '${enclosingElement.library.dirName}/${enclosingElement.name}/$name';
}

@override
Expand Down
7 changes: 7 additions & 0 deletions lib/src/model_utils.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,13 @@ bool hasPrivateName(Element e) {
if (e.name.startsWith('_')) {
return true;
}
// GenericFunctionTypeElements have the name we care about in the enclosing
// element.
if (e is GenericFunctionTypeElement) {
if (e.enclosingElement.name.startsWith('_')) {
return true;
}
}
if (e is LibraryElement &&
(e.identifier.startsWith('dart:_') ||
['dart:nativewrappers'].contains(e.identifier))) {
Expand Down
6 changes: 6 additions & 0 deletions test/model_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1889,6 +1889,7 @@ String topLevelFunction(int param1, bool param2, Cool coolBeans,
group('Constant', () {
TopLevelVariable greenConstant,
cat,
customClassPrivate,
orangeConstant,
prettyColorsConstant,
deprecated;
Expand All @@ -1906,6 +1907,7 @@ String topLevelFunction(int param1, bool param2, Cool coolBeans,
deprecated =
exLibrary.constants.firstWhere((c) => c.name == 'deprecated');
Class Dog = exLibrary.allClasses.firstWhere((c) => c.name == 'Dog');
customClassPrivate = fakeLibrary.constants.firstWhere((c) => c.name == 'CUSTOM_CLASS_PRIVATE');
aStaticConstField =
Dog.allFields.firstWhere((f) => f.name == 'aStaticConstField');
aName = Dog.allFields.firstWhere((f) => f.name == 'aName');
Expand All @@ -1920,6 +1922,10 @@ String topLevelFunction(int param1, bool param2, Cool coolBeans,
expect(aStaticConstField.constantValue, '&quot;A Constant Dog&quot;');
});

test('privately constructed constants are unlinked', () {
Copy link
Member

Choose a reason for hiding this comment

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

👍

expect(customClassPrivate.constantValue, 'const _APrivateConstClass()');
});

test('has a fully qualified name', () {
expect(greenConstant.fullyQualifiedName, 'ex.COLOR_GREEN');
});
Expand Down
6 changes: 6 additions & 0 deletions testing/test_package/lib/fake.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,12 @@ class ConstantClass {
ConstantClass.notConstant(this.value);
}

class _APrivateConstClass {
const _APrivateConstClass();
}

const _APrivateConstClass CUSTOM_CLASS_PRIVATE = const _APrivateConstClass();

// No dart docs on purpose. Also, a non-primitive const class.
const ConstantClass CUSTOM_CLASS = const ConstantClass('custom');

Expand Down
1 change: 1 addition & 0 deletions testing/test_package_docs/fake/AClassUsingASuperMixin-class.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ <h5>fake library</h5>

<li class="section-title"><a href="fake/fake-library.html#constants">Constants</a></li>
<li><a href="fake/CUSTOM_CLASS-constant.html">CUSTOM_CLASS</a></li>
<li><a href="fake/CUSTOM_CLASS_PRIVATE-constant.html">CUSTOM_CLASS_PRIVATE</a></li>
<li><a class="deprecated" href="fake/DOWN-constant.html">DOWN</a></li>
<li><a href="fake/greatAnnotation-constant.html">greatAnnotation</a></li>
<li><a href="fake/greatestAnnotation-constant.html">greatestAnnotation</a></li>
Expand Down
1 change: 1 addition & 0 deletions testing/test_package_docs/fake/AMixinCallingSuper-class.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ <h5>fake library</h5>

<li class="section-title"><a href="fake/fake-library.html#constants">Constants</a></li>
<li><a href="fake/CUSTOM_CLASS-constant.html">CUSTOM_CLASS</a></li>
<li><a href="fake/CUSTOM_CLASS_PRIVATE-constant.html">CUSTOM_CLASS_PRIVATE</a></li>
<li><a class="deprecated" href="fake/DOWN-constant.html">DOWN</a></li>
<li><a href="fake/greatAnnotation-constant.html">greatAnnotation</a></li>
<li><a href="fake/greatestAnnotation-constant.html">greatestAnnotation</a></li>
Expand Down
Loading