Skip to content
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

Resolve non-imported symbols in comments [#1153] #1298

Merged
merged 3 commits into from
Dec 27, 2016
Merged

Resolve non-imported symbols in comments [#1153] #1298

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b386283
Resolve non-imported symbols in comments [#1153]
astashov Dec 17, 2016
d00d2ea
Address @devoncarew feedback
astashov Dec 20, 2016
6930dc6
Address another round of feedback
astashov Dec 23, 2016
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
99 changes: 28 additions & 71 deletions lib/src/markdown_processor.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,7 @@ import 'dart:convert';

import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/element/element.dart'
show
LibraryElement,
Element,
ConstructorElement,
ClassElement,
ParameterElement,
PropertyAccessorElement;
show LibraryElement, Element, ConstructorElement, ClassElement, ParameterElement, PropertyAccessorElement;
import 'package:html/parser.dart' show parse;
import 'package:markdown/markdown.dart' as md;

Expand All @@ -39,27 +33,19 @@ NodeList<CommentReference> _getCommentRefs(ModelElement modelElement) {
if (modelElement == null) return null;
if (modelElement.documentation == null && modelElement.canOverride()) {
var melement = modelElement.overriddenElement;
if (melement != null &&
melement.element.computeNode() != null &&
melement.element.computeNode() is AnnotatedNode) {
var docComment = (melement.element.computeNode() as AnnotatedNode)
.documentationComment;
if (melement != null && melement.element.computeNode() != null && melement.element.computeNode() is AnnotatedNode) {
var docComment = (melement.element.computeNode() as AnnotatedNode).documentationComment;
if (docComment != null) return docComment.references;
return null;
}
}
if (modelElement.element.computeNode() is AnnotatedNode) {
if ((modelElement.element.computeNode() as AnnotatedNode)
.documentationComment !=
null) {
return (modelElement.element.computeNode() as AnnotatedNode)
.documentationComment
.references;
if ((modelElement.element.computeNode() as AnnotatedNode).documentationComment != null) {
Copy link
Member

Choose a reason for hiding this comment

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

We're doing this cast twice inside this if statement (modelElement.element.computeNode() as AnnotatedNode) - perhaps create a new local variable to host the AnnotatedNode?

return (modelElement.element.computeNode() as AnnotatedNode).documentationComment.references;
}
} else if (modelElement.element is LibraryElement) {
// handle anonymous libraries
if (modelElement.element.computeNode() == null ||
modelElement.element.computeNode().parent == null) {
if (modelElement.element.computeNode() == null || modelElement.element.computeNode().parent == null) {
return null;
}
var node = modelElement.element.computeNode().parent.parent;
Expand All @@ -73,8 +59,7 @@ NodeList<CommentReference> _getCommentRefs(ModelElement modelElement) {
}

/// Returns null if element is a parameter.
MatchingLinkResult _getMatchingLinkElement(
String codeRef, ModelElement element, List<CommentReference> commentRefs,
MatchingLinkResult _getMatchingLinkElement(String codeRef, ModelElement element, List<CommentReference> commentRefs,
{bool isConstructor: false}) {
if (commentRefs == null) return new MatchingLinkResult(null, null);

Expand All @@ -84,8 +69,7 @@ MatchingLinkResult _getMatchingLinkElement(
for (CommentReference ref in commentRefs) {
if (ref.identifier.name == codeRef) {
bool isConstrElement = ref.identifier.staticElement is ConstructorElement;
if (isConstructor && isConstrElement ||
!isConstructor && !isConstrElement) {
if (isConstructor && isConstrElement || !isConstructor && !isConstrElement) {
refElement = ref.identifier.staticElement;
break;
}
Expand All @@ -101,8 +85,7 @@ MatchingLinkResult _getMatchingLinkElement(
// yay we found an accessor that wraps a const, but we really
// want the top-level field itself
refElement = (refElement as PropertyAccessorElement).variable;
if (refElement.enclosingElement is ClassElement &&
(refElement.enclosingElement as ClassElement).isEnum) {
if (refElement.enclosingElement is ClassElement && (refElement.enclosingElement as ClassElement).isEnum) {
isEnum = true;
}
}
Expand All @@ -115,8 +98,7 @@ MatchingLinkResult _getMatchingLinkElement(
//
// Don't search through all libraries in the package, actually search
// in the current scope.
Library refLibrary =
element.package.findLibraryFor(refElement, scopedTo: element);
Library refLibrary = element.package.findLibraryFor(refElement, scopedTo: element);

if (refLibrary != null) {
// Is there a way to pull this from a registry of known elements?
Expand All @@ -130,58 +112,38 @@ MatchingLinkResult _getMatchingLinkElement(
}

MatchingLinkResult _findRefElementInLibrary(String codeRef, ModelElement element, List<CommentReference> commentRefs) {
final package = element.library.package;
final Package package = element.library.package;
final Map<String, ModelElement> result = {};
package.libraries.forEach((library) {
final List<ModelElement> modelElements = []
..addAll(library.allClasses)
..addAll(library.constants)
..addAll(library.enums)
..addAll(library.functions)
..addAll(library.properties)
..addAll(library.typedefs);

library.allClasses.forEach((c) {
modelElements.addAll(c.allInstanceMethods);
modelElements.addAll(c.allInstanceProperties);
modelElements.addAll(c.allOperators);
modelElements.addAll(c.staticMethods);
modelElements.addAll(c.staticProperties);
});

modelElements.forEach((modelElement) {
if (codeRef == modelElement.name || codeRef == modelElement.fullyQualifiedName) {
result[modelElement.fullyQualifiedName] = modelElement;
}
});
package.allModelElements.forEach((modelElement) {
Copy link
Member

Choose a reason for hiding this comment

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

Last comment, I promise :)

If we use a Map here, in the event we have multiple matches, the entry in the map could be overridden. So that would hide the fact that the reference is not unique. Perhaps use a List instead?

And we'll be iterating over a lot of elements - I tend to avoid using a closure for that; a for loop will be more performant.

You might do something thuswise:

List<ModelElement> results = [];

for (ModelElement element in package.allModelElements) {
  if (codeRef.contains('.')) {
    // It's a fully qualified reference.
    if (element.fullyQualifiedName == codeRef) {
      results.add(element);
      break;
    }
  } else {
    if (element.name == codeRef) {
      results.add(element);
    }
  }
}

if (codeRef == modelElement.name || codeRef == modelElement.fullyQualifiedName) {
result[modelElement.fullyQualifiedName] = modelElement;
}
});

if (result.isEmpty) {
return new MatchingLinkResult(null, null);
} else if (result.length == 1) {
return new MatchingLinkResult(result.values.first, result.values.first.name);
} else {
if (_emitWarning) {
print("Abiguous reference '${codeRef}' in '${element.fullyQualifiedName}', " +
"we found it in the following elements: ${result.keys.map((k) => "'${k}'").join(", ")}");
}
// TODO: add --fatal-warning, which would make the app crash in case of ambiguous references
print(
"Ambiguous reference to [${codeRef}] in '${element.fullyQualifiedName}' (${element.sourceFileName}:${element.lineNumber}). " +
"We found matches to the following elements: ${result.keys.map((k) => "'${k}'").join(", ")}");
return new MatchingLinkResult(null, null);
}
}

String _linkDocReference(String reference, ModelElement element,
NodeList<CommentReference> commentRefs) {
String _linkDocReference(String reference, ModelElement element, NodeList<CommentReference> commentRefs) {
// support for [new Constructor] and [new Class.namedCtr]
var refs = reference.split(' ');
MatchingLinkResult result;
if (refs.length == 2 && refs.first == 'new') {
result = _getMatchingLinkElement(refs[1], element, commentRefs,
isConstructor: true);
result = _getMatchingLinkElement(refs[1], element, commentRefs, isConstructor: true);
} else {
result = _getMatchingLinkElement(reference, element, commentRefs);
}
final linkedElement = result.element;
final label = result.label ?? reference;
final ModelElement linkedElement = result.element;
final String label = result.label ?? reference;
if (linkedElement != null) {
var classContent = '';
if (linkedElement.isDeprecated) {
Expand All @@ -192,6 +154,7 @@ String _linkDocReference(String reference, ModelElement element,
return '<a ${classContent}href="${linkedElement.href}">$label</a>';
} else {
if (_emitWarning) {
// TODO: add --fatal-warning, which would make the app crash in case of ambiguous references
print(" warning: unresolved doc reference '$reference' (in $element)");
}
return '<code>${HTML_ESCAPE.convert(label)}</code>';
Expand All @@ -204,8 +167,7 @@ String _renderMarkdownToHtml(String text, [ModelElement element]) {
return new md.Text(_linkDocReference(name, element, commentRefs));
}

return md.markdownToHtml(text,
inlineSyntaxes: _markdown_syntaxes, linkResolver: _linkResolver);
return md.markdownToHtml(text, inlineSyntaxes: _markdown_syntaxes, linkResolver: _linkResolver);
}

class Documentation {
Expand All @@ -231,16 +193,13 @@ class Documentation {
s.remove();
}
for (var pre in asHtmlDocument.querySelectorAll('pre')) {
if (pre.children.isNotEmpty &&
pre.children.length != 1 &&
pre.children.first.localName != 'code') {
if (pre.children.isNotEmpty && pre.children.length != 1 && pre.children.first.localName != 'code') {
continue;
}

if (pre.children.isNotEmpty && pre.children.first.localName == 'code') {
var code = pre.children.first;
pre.classes
.addAll(code.classes.where((name) => name.startsWith('language-')));
pre.classes.addAll(code.classes.where((name) => name.startsWith('language-')));
}

bool specifiesLanguage = pre.classes.isNotEmpty;
Expand All @@ -251,9 +210,7 @@ class Documentation {

// `trim` fixes issue with line ending differences between mac and windows.
var asHtml = asHtmlDocument.body.innerHtml?.trim();
var asOneLiner = asHtmlDocument.body.children.isEmpty
? ''
: asHtmlDocument.body.children.first.innerHtml;
var asOneLiner = asHtmlDocument.body.children.isEmpty ? '' : asHtmlDocument.body.children.first.innerHtml;
if (!asOneLiner.startsWith('<p>')) {
asOneLiner = '<p>$asOneLiner</p>';
}
Expand Down
63 changes: 49 additions & 14 deletions lib/src/model.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1425,6 +1425,26 @@ abstract class ModelElement implements Comparable, Nameable, Documentable {
return (_fullyQualifiedName ??= _buildFullyQualifiedName());
}

String get sourceFileName {
Copy link
Member

Choose a reason for hiding this comment

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

nit: this can use the function shorthand (=>)

return element.source.fullName;
}

int _lineNumber;
bool _isLineNumberComputed = false;
int get lineNumber {
if (!_isLineNumberComputed) {
var node = element.computeNode();
if (node is Declaration && node.element != null) {
var element = node.element;
var lineNumber = lineNumberCache.lineNumber(
element.source.fullName, element.nameOffset);
_lineNumber = lineNumber + 1;
}
_isLineNumberComputed = true;
}
return _lineNumber;
}

bool get hasAnnotations => annotations.isNotEmpty;

@override
Expand Down Expand Up @@ -2047,6 +2067,31 @@ class Package implements Nameable, Documentable {
}
return new Library(e.library, this);
}

List<ModelElement> _allModelElements;
List<ModelElement> get allModelElements {
if (_allModelElements == null) {
_allModelElements = [];
this.libraries.forEach((library) {
_allModelElements
..addAll(library.allClasses)
..addAll(library.constants)
..addAll(library.enums)
..addAll(library.functions)
..addAll(library.properties)
..addAll(library.typedefs);

library.allClasses.forEach((c) {
_allModelElements.addAll(c.allInstanceMethods);
_allModelElements.addAll(c.allInstanceProperties);
_allModelElements.addAll(c.allOperators);
_allModelElements.addAll(c.staticMethods);
_allModelElements.addAll(c.staticProperties);
});
});
}
return _allModelElements;
}
}

class PackageCategory implements Comparable<PackageCategory> {
Expand Down Expand Up @@ -2128,6 +2173,8 @@ abstract class SourceCodeMixin {
}
}

int get lineNumber;

Element get element;

bool get hasSourceCode => config.includeSource && sourceCode.isNotEmpty;
Expand Down Expand Up @@ -2210,21 +2257,9 @@ abstract class SourceCodeMixin {
}

String get _crossdartUrl {
if (_lineNumber != null && _crossdartPath != null) {
if (lineNumber != null && _crossdartPath != null) {
String url = "//www.crossdart.info/p/${_crossdartPath}.html";
return "${url}#line-${_lineNumber}";
} else {
return null;
}
}

int get _lineNumber {
var node = element.computeNode();
if (node is Declaration && node.element != null) {
var element = node.element;
var lineNumber = lineNumberCache.lineNumber(
element.source.fullName, element.nameOffset);
return lineNumber + 1;
return "${url}#line-${lineNumber}";
} else {
return null;
}
Expand Down