From af8eba43af3ee90cc247ae239e85138768033386 Mon Sep 17 00:00:00 2001 From: Sam Rawlins Date: Mon, 29 Mar 2021 14:59:06 -0700 Subject: [PATCH 1/3] Simplify logic in features rendering. The default template calls `featuresAsString` twice, once to see if it is empty, then again to render the poor String. `featuresAsString` involves: 1. Iterating over all annotations, 2. finding canonical elements, 3. HTML-escaping the source of each one, 4. sorting the results into a list, by a custom sort. This change introduces a new `hasFeatures` getter. Using this in templates: 1. avoids HTML-escaping, 2. avoids sorting, 3. short-circuits annotations if the element is `final` or `late`, 4. returns `true` when the first displayable element is found, sometimes avoiding iterating. Additionally, the HtmlEscape is extracted into a static const. A `realElement` getter is introduced on ElementAnnotation, which is used in a number of places. Annotations are not HTML-escaped unless we definitely want to display them. --- lib/src/model/model_element.dart | 65 ++++++++++++++++++++----------- lib/templates/html/_features.html | 2 +- lib/templates/md/_features.md | 2 +- 3 files changed, 45 insertions(+), 24 deletions(-) diff --git a/lib/src/model/model_element.dart b/lib/src/model/model_element.dart index b145af31fe..df4a8a1bf5 100644 --- a/lib/src/model/model_element.dart +++ b/lib/src/model/model_element.dart @@ -384,6 +384,8 @@ abstract class ModelElement extends Canonicalization throw 'Unknown type ${e.runtimeType}'; } + static const _htmlEscape = HtmlEscape(); + // Stub for mustache, which would otherwise search enclosing elements to find // names for members. bool get hasCategoryNames => false; @@ -412,30 +414,11 @@ abstract class ModelElement extends Canonicalization var annotationStrings = []; if (md == null) return annotationStrings; for (var a in md) { - var annotation = (const HtmlEscape()).convert(a.toSource()); - var annotationElement = a.element; - - if (annotationElement is ConstructorElement) { - // TODO(srawlins): I think we should actually link to the constructor, - // which may have details about parameters. For example, given the - // annotation `@Immutable('text')`, the constructor documents what the - // parameter is, and the class only references `immutable`. It's a - // lose-lose cycle of mis-direction. - annotationElement = - (annotationElement as ConstructorElement).returnType.element; - } else if (annotationElement is PropertyAccessorElement) { - annotationElement = - (annotationElement as PropertyAccessorElement).variable; - } - if (annotationElement is Member) { - annotationElement = (annotationElement as Member).declaration; - } - - // Some annotations are intended to be invisible (such as `@pragma`). - if (!_shouldDisplayAnnotation(annotationElement)) continue; + if (!_shouldDisplayAnnotation(a.realElement)) continue; + var annotation = _htmlEscape.convert(a.toSource()); var annotationModelElement = - packageGraph.findCanonicalModelElementFor(annotationElement); + packageGraph.findCanonicalModelElementFor(a.realElement); if (annotationModelElement != null) { annotation = annotation.replaceFirst( annotationModelElement.name, annotationModelElement.linkedName); @@ -445,6 +428,10 @@ abstract class ModelElement extends Canonicalization return annotationStrings; } + /// Returns whether [annotationElement] should be displayed when rendering + /// this element. + /// + /// Some annotations are intended to be invisible (such as `@pragma`). bool _shouldDisplayAnnotation(Element annotationElement) { if (annotationElement is ClassElement) { var annotationClass = @@ -529,6 +516,16 @@ abstract class ModelElement extends Canonicalization 'deprecated' }; + /// Returns whether this has any displayable features. + bool get hasFeatures { + if (isFinal) return true; + if (isLate) return true; + + return element.metadata + .where((e) => !_specialFeatures.contains(e.element?.name)) + .any((e) => _shouldDisplayAnnotation(e.realElement)); + } + Set get features { return { ...annotationsFromMetadata(element.metadata @@ -540,6 +537,8 @@ abstract class ModelElement extends Canonicalization }; } + /// Returns [features] as a single String, sorted [byFeatureOrdering], joined + /// with commas. String get featuresAsString { var allFeatures = features.toList()..sort(byFeatureOrdering); return allFeatures.join(', '); @@ -1222,3 +1221,25 @@ abstract class ModelElement extends Canonicalization }); } } + +extension on ElementAnnotation { + /// Returns this annotation's "real" element, passing through unused, + /// intermediate elements. + Element get realElement { + var element = this.element; + if (element is ConstructorElement) { + // TODO(srawlins): I think we should actually link to the constructor, + // which may have details about parameters. For example, given the + // annotation `@Immutable('text')`, the constructor documents what the + // parameter is, and the class only references `immutable`. It's a + // lose-lose cycle of mis-direction. + return element.returnType.element; + } else if (element is PropertyAccessorElement) { + return element.variable; + } else if (element is Member) { + return element.declaration; + } else { + return element; + } + } +} diff --git a/lib/templates/html/_features.html b/lib/templates/html/_features.html index 507bd0d8a7..963ef94cbe 100644 --- a/lib/templates/html/_features.html +++ b/lib/templates/html/_features.html @@ -1 +1 @@ -{{ #featuresAsString.isNotEmpty }}
{{{featuresAsString}}}
{{ /featuresAsString.isNotEmpty }} +{{ #hasFeatures }}
{{{featuresAsString}}}
{{ /hasFeatures }} \ No newline at end of file diff --git a/lib/templates/md/_features.md b/lib/templates/md/_features.md index 2083ee0c19..7e4b58d52c 100644 --- a/lib/templates/md/_features.md +++ b/lib/templates/md/_features.md @@ -1 +1 @@ -{{ #featuresAsString.isNotEmpty }}_{{{featuresAsString}}}_{{ /featuresAsString.isNotEmpty }} +{{ #hasFeatures }}_{{{featuresAsString}}}_{{ /hasFeatures }} From d0ab4459a212a1c059018ed86be5a08f3ba35407 Mon Sep 17 00:00:00 2001 From: Sam Rawlins Date: Mon, 29 Mar 2021 17:55:59 -0700 Subject: [PATCH 2/3] Undo hasFeatures --- lib/src/generator/templates.renderers.dart | 12 ------------ lib/src/model/container_member.dart | 9 ++++----- lib/src/model/getter_setter_combo.dart | 2 ++ lib/src/model/inheritable.dart | 13 ++++++------- lib/src/model/model_element.dart | 10 ---------- lib/templates/html/_features.html | 2 +- lib/templates/md/_features.md | 2 +- 7 files changed, 14 insertions(+), 36 deletions(-) diff --git a/lib/src/generator/templates.renderers.dart b/lib/src/generator/templates.renderers.dart index 1a3cb162bb..4baae0f571 100644 --- a/lib/src/generator/templates.renderers.dart +++ b/lib/src/generator/templates.renderers.dart @@ -8143,18 +8143,6 @@ class _Renderer_GetterSetterCombo extends RendererBase { parent: r); }, ), - 'comboFeatures': Property( - getValue: (CT_ c) => c.comboFeatures, - renderVariable: (CT_ c, Property self, - List remainingNames) => - self.renderSimpleVariable( - c, remainingNames, 'Set'), - renderIterable: - (CT_ c, RendererBase r, List ast) { - return c.comboFeatures.map( - (e) => _render_String(e, ast, r.template, parent: r)); - }, - ), 'constantInitializer': Property( getValue: (CT_ c) => c.constantInitializer, renderVariable: (CT_ c, Property self, diff --git a/lib/src/model/container_member.dart b/lib/src/model/container_member.dart index 64c69b089d..110eb17aa5 100644 --- a/lib/src/model/container_member.dart +++ b/lib/src/model/container_member.dart @@ -25,11 +25,10 @@ mixin ContainerMember on ModelElement implements EnclosedElement { } @override - Set get features { - var _features = super.features; - if (isExtended) _features.add('extended'); - return _features; - } + Set get features => { + ...super.features, + if (isExtended) 'extended', + }; bool _canonicalEnclosingContainerIsSet = false; Container _canonicalEnclosingContainer; diff --git a/lib/src/model/getter_setter_combo.dart b/lib/src/model/getter_setter_combo.dart index fa5ff1cfb1..aef7100a39 100644 --- a/lib/src/model/getter_setter_combo.dart +++ b/lib/src/model/getter_setter_combo.dart @@ -12,6 +12,7 @@ import 'package:dartdoc/src/element_type.dart'; import 'package:dartdoc/src/model/model.dart'; import 'package:dartdoc/src/utils.dart'; import 'package:dartdoc/src/warnings.dart'; +import 'package:meta/meta.dart'; /// Mixin for top-level variables and fields (aka properties) mixin GetterSetterCombo on ModelElement { @@ -25,6 +26,7 @@ mixin GetterSetterCombo on ModelElement { } } + @protected Set get comboFeatures { var allFeatures = {}; if (hasExplicitGetter && hasPublicGetter) { diff --git a/lib/src/model/inheritable.dart b/lib/src/model/inheritable.dart index 7c4e03ccd5..f1656519ec 100644 --- a/lib/src/model/inheritable.dart +++ b/lib/src/model/inheritable.dart @@ -28,13 +28,12 @@ mixin Inheritable on ContainerMember { bool get isCovariant; @override - Set get features { - var _features = super.features; - if (isOverride) _features.add('override'); - if (isInherited) _features.add('inherited'); - if (isCovariant) _features.add('covariant'); - return _features; - } + Set get features => { + ...super.features, + if (isOverride) 'override', + if (isInherited) 'inherited', + if (isCovariant) 'covariant', + }; @override Library get canonicalLibrary => canonicalEnclosingContainer?.canonicalLibrary; diff --git a/lib/src/model/model_element.dart b/lib/src/model/model_element.dart index df4a8a1bf5..f55db50163 100644 --- a/lib/src/model/model_element.dart +++ b/lib/src/model/model_element.dart @@ -516,16 +516,6 @@ abstract class ModelElement extends Canonicalization 'deprecated' }; - /// Returns whether this has any displayable features. - bool get hasFeatures { - if (isFinal) return true; - if (isLate) return true; - - return element.metadata - .where((e) => !_specialFeatures.contains(e.element?.name)) - .any((e) => _shouldDisplayAnnotation(e.realElement)); - } - Set get features { return { ...annotationsFromMetadata(element.metadata diff --git a/lib/templates/html/_features.html b/lib/templates/html/_features.html index 963ef94cbe..14379183cb 100644 --- a/lib/templates/html/_features.html +++ b/lib/templates/html/_features.html @@ -1 +1 @@ -{{ #hasFeatures }}
{{{featuresAsString}}}
{{ /hasFeatures }} \ No newline at end of file +{{ #features }}
{{{featuresAsString}}}
{{ /features }} diff --git a/lib/templates/md/_features.md b/lib/templates/md/_features.md index 7e4b58d52c..d02fc49854 100644 --- a/lib/templates/md/_features.md +++ b/lib/templates/md/_features.md @@ -1 +1 @@ -{{ #hasFeatures }}_{{{featuresAsString}}}_{{ /hasFeatures }} +{{ #features }}_{{{featuresAsString}}}_{{ /features }} From b2b3f9bf2d54b6c67fbba1dc6ca9f99fefa07f8f Mon Sep 17 00:00:00 2001 From: Sam Rawlins Date: Mon, 29 Mar 2021 18:14:50 -0700 Subject: [PATCH 3/3] Tidy logic --- lib/src/model/model_element.dart | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/src/model/model_element.dart b/lib/src/model/model_element.dart index f55db50163..47fca3717c 100644 --- a/lib/src/model/model_element.dart +++ b/lib/src/model/model_element.dart @@ -1216,20 +1216,21 @@ extension on ElementAnnotation { /// Returns this annotation's "real" element, passing through unused, /// intermediate elements. Element get realElement { + Element getDeclaration(Element e) => e is Member ? e.declaration : e; + var element = this.element; + if (element is ConstructorElement) { // TODO(srawlins): I think we should actually link to the constructor, // which may have details about parameters. For example, given the // annotation `@Immutable('text')`, the constructor documents what the // parameter is, and the class only references `immutable`. It's a // lose-lose cycle of mis-direction. - return element.returnType.element; + return getDeclaration(element.returnType.element); } else if (element is PropertyAccessorElement) { - return element.variable; - } else if (element is Member) { - return element.declaration; + return getDeclaration(element.variable); } else { - return element; + return getDeclaration(element); } } }