-
Notifications
You must be signed in to change notification settings - Fork 117
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
Resolve non-imported symbols in comments [#1153] #1298
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
||
|
@@ -28,32 +22,30 @@ const List<String> _oneLinerSkipTags = const ["code", "pre"]; | |
|
||
final List<md.InlineSyntax> _markdown_syntaxes = [new _InlineCodeSyntax()]; | ||
|
||
class MatchingLinkResult { | ||
final ModelElement element; | ||
final String label; | ||
MatchingLinkResult(this.element, this.label); | ||
} | ||
|
||
// TODO: this is in the wrong place | ||
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) { | ||
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; | ||
|
@@ -67,84 +59,105 @@ NodeList<CommentReference> _getCommentRefs(ModelElement modelElement) { | |
} | ||
|
||
/// Returns null if element is a parameter. | ||
ModelElement _getMatchingLinkElement( | ||
String codeRef, ModelElement element, List<CommentReference> commentRefs, | ||
MatchingLinkResult _getMatchingLinkElement(String codeRef, ModelElement element, List<CommentReference> commentRefs, | ||
{bool isConstructor: false}) { | ||
if (commentRefs == null) return null; | ||
if (commentRefs == null) return new MatchingLinkResult(null, null); | ||
|
||
Element refElement; | ||
bool isEnum = false; | ||
|
||
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; | ||
} | ||
} | ||
} | ||
|
||
// Did not find an element in scope | ||
if (refElement == null) return null; | ||
if (refElement == null) { | ||
return _findRefElementInLibrary(codeRef, element, commentRefs); | ||
} | ||
|
||
if (refElement is PropertyAccessorElement) { | ||
// 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; | ||
} | ||
} | ||
|
||
if (refElement is ParameterElement) return null; | ||
if (refElement is ParameterElement) return new MatchingLinkResult(null, null); | ||
|
||
// bug! this can fail to find the right library name if the element's name | ||
// we're looking for is the same as a name that comes in from an imported | ||
// library. | ||
// | ||
// 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? | ||
// Seems like we're creating too many objects this way. | ||
if (isEnum) { | ||
return new EnumField(refElement, refLibrary); | ||
return new MatchingLinkResult(new EnumField(refElement, refLibrary), null); | ||
} | ||
return new ModelElement.from(refElement, refLibrary); | ||
return new MatchingLinkResult(new ModelElement.from(refElement, refLibrary), null); | ||
} | ||
return new MatchingLinkResult(null, null); | ||
} | ||
|
||
MatchingLinkResult _findRefElementInLibrary(String codeRef, ModelElement element, List<CommentReference> commentRefs) { | ||
final Package package = element.library.package; | ||
final Map<String, ModelElement> result = {}; | ||
package.allModelElements.forEach((modelElement) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
// 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); | ||
} | ||
return null; | ||
} | ||
|
||
String _linkDocReference(String reference, ModelElement element, | ||
NodeList<CommentReference> commentRefs) { | ||
ModelElement linkedElement; | ||
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') { | ||
linkedElement = _getMatchingLinkElement(refs[1], element, commentRefs, | ||
isConstructor: true); | ||
result = _getMatchingLinkElement(refs[1], element, commentRefs, isConstructor: true); | ||
} else { | ||
linkedElement = _getMatchingLinkElement(reference, element, commentRefs); | ||
result = _getMatchingLinkElement(reference, element, commentRefs); | ||
} | ||
final ModelElement linkedElement = result.element; | ||
final String label = result.label ?? reference; | ||
if (linkedElement != null) { | ||
var classContent = ''; | ||
if (linkedElement.isDeprecated) { | ||
classContent = 'class="deprecated" '; | ||
} | ||
// this would be linkedElement.linkedName, but link bodies are slightly | ||
// different for doc references. sigh. | ||
return '<a ${classContent}href="${linkedElement.href}">$reference</a>'; | ||
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(reference)}</code>'; | ||
return '<code>${HTML_ESCAPE.convert(label)}</code>'; | ||
} | ||
} | ||
|
||
|
@@ -154,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 { | ||
|
@@ -181,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; | ||
|
@@ -201,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>'; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1425,6 +1425,26 @@ abstract class ModelElement implements Comparable, Nameable, Documentable { | |
return (_fullyQualifiedName ??= _buildFullyQualifiedName()); | ||
} | ||
|
||
String get sourceFileName { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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> { | ||
|
@@ -2128,6 +2173,8 @@ abstract class SourceCodeMixin { | |
} | ||
} | ||
|
||
int get lineNumber; | ||
|
||
Element get element; | ||
|
||
bool get hasSourceCode => config.includeSource && sourceCode.isNotEmpty; | ||
|
@@ -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; | ||
} | ||
|
There was a problem hiding this comment.
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?