Skip to content

Remove support for deprecated leading new in comment references #3529

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
Oct 19, 2023
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
55 changes: 1 addition & 54 deletions lib/src/comment_references/model_comment_reference.dart
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,7 @@ import 'package:analyzer/file_system/file_system.dart';
import 'package:dartdoc/src/comment_references/parser.dart';

abstract class ModelCommentReference {
/// Does the structure of the reference itself imply a possible unnamed
/// constructor?
bool get allowUnnamedConstructor;
bool get allowUnnamedConstructorParameter;
String get codeRef;
bool get hasConstructorHint;
bool get hasCallableHint;
List<String> get referenceBy;
Element? get staticElement;
Expand All @@ -33,63 +28,15 @@ abstract class ModelCommentReference {
/// information needed for Dartdoc. Drops link to the [CommentReference]
/// and [ResourceProvider] after construction.
class _ModelCommentReferenceImpl implements ModelCommentReference {
void _initAllowCache() {
final referencePieces =
parsed.whereType<IdentifierNode>().toList(growable: false);
_allowUnnamedConstructor = false;
_allowUnnamedConstructorParameter = false;
if (referencePieces.length >= 2) {
for (var i = 0; i <= referencePieces.length - 2; i++) {
if (referencePieces[i].text == referencePieces[i + 1].text) {
if (i + 2 == referencePieces.length) {
// This looks like an old-style reference to an unnamed
// constructor, e.g. [lib_name.C.C].
_allowUnnamedConstructor = true;
} else {
// This could be a reference to a parameter or type parameter of
// an unnamed/new-declared constructor.
_allowUnnamedConstructorParameter = true;
}
}
}
// e.g. [C.new], which may be the unnamed constructor.
if (referencePieces.isNotEmpty && referencePieces.last.text == 'new') {
_allowUnnamedConstructor = true;
}
}
}

bool? _allowUnnamedConstructor;
@override
bool get allowUnnamedConstructor {
if (_allowUnnamedConstructor == null) {
_initAllowCache();
}
return _allowUnnamedConstructor!;
}

bool? _allowUnnamedConstructorParameter;
@override
bool get allowUnnamedConstructorParameter {
if (_allowUnnamedConstructorParameter == null) {
_initAllowCache();
}
return _allowUnnamedConstructorParameter!;
}

@override
final String codeRef;

@override
bool get hasCallableHint =>
parsed.isNotEmpty &&
(parsed.first is ConstructorHintStartNode ||
((parsed.length > 1 && parsed.last.text == 'new') ||
parsed.last is CallableHintEndNode);

@override
bool get hasConstructorHint =>
parsed.isNotEmpty && parsed.first is ConstructorHintStartNode;

@override
List<String> get referenceBy => parsed
.whereType<IdentifierNode>()
Expand Down
26 changes: 2 additions & 24 deletions lib/src/comment_references/parser.dart
Original file line number Diff line number Diff line change
Expand Up @@ -157,29 +157,20 @@ class CommentReferenceParser {
return children;
}

static const _constructorHintPrefix = 'new';
static const _ignorePrefixes = ['const', 'final', 'var'];

/// Implement parsing a prefix to a comment reference.
///
/// ```text
/// <prefix> ::= <constructorPrefixHint>
/// | <leadingJunk>
/// <prefix> ::= <leadingModifiers>
///
/// <constructorPrefixHint> ::= 'new '
///
/// <leadingJunk> ::= ('const' | 'final' | 'var')(' '+)
/// <leadingModifiers> ::= ('const' | 'final' | 'var')(' '+)
/// ```
_PrefixParseResult _parsePrefix() {
if (_atEnd) {
return _PrefixParseResult.endOfFile;
}
_walkPastWhitespace();
if (_tryMatchLiteral(_constructorHintPrefix,
requireTrailingNonidentifier: true)) {
return _PrefixParseResult.ok(
ConstructorHintStartNode(_constructorHintPrefix));
}
if (_ignorePrefixes
.any((p) => _tryMatchLiteral(p, requireTrailingNonidentifier: true))) {
return _PrefixParseResult.junk;
Expand Down Expand Up @@ -387,9 +378,6 @@ class _PrefixParseResult {

const _PrefixParseResult._(this.type, this.node);

factory _PrefixParseResult.ok(ConstructorHintStartNode node) =>
_PrefixParseResult._(_PrefixResultType.parsedConstructorHint, node);

static const _PrefixParseResult endOfFile =
_PrefixParseResult._(_PrefixResultType.endOfFile, null);

Expand Down Expand Up @@ -484,16 +472,6 @@ sealed class CommentReferenceNode {
String get text;
}

class ConstructorHintStartNode extends CommentReferenceNode {
@override
final String text;

ConstructorHintStartNode(this.text);

@override
String toString() => 'ConstructorHintStartNode["$text"]';
}

class CallableHintEndNode extends CommentReferenceNode {
@override
final String text;
Expand Down
43 changes: 11 additions & 32 deletions lib/src/markdown_processor.dart
Original file line number Diff line number Diff line change
Expand Up @@ -160,10 +160,6 @@ bool _rejectUnnamedAndShadowingConstructors(CommentReferable? referable) {
bool _requireCallable(CommentReferable? referable) =>
referable is ModelElement && referable.isCallable;

/// Returns false unless the passed [referable] represents a constructor.
bool _requireConstructor(CommentReferable? referable) =>
referable is Constructor;

MatchingLinkResult _getMatchingLinkElement(
String referenceText, Warnable element) {
var commentReference = ModelCommentReference.synthetic(referenceText);
Expand All @@ -174,38 +170,21 @@ MatchingLinkResult _getMatchingLinkElement(
// An "allow tree" filter to be used by [CommentReferable.referenceBy].
bool Function(CommentReferable?) allowTree;

// Constructor references are pretty ambiguous by nature since they can be
// declared with the same name as the class they are constructing, and even
// if they don't use field-formal parameters, sometimes have parameters
// named the same as members.
// Maybe clean this up with inspiration from constructor tear-off syntax?
if (commentReference.allowUnnamedConstructor) {
allowTree = (_) => true;
// Neither reject, nor require, a unnamed constructor in the event the
// comment reference structure implies one. (We can not require it in case
// a library name is the same as a member class name and the class is the
// intended lookup). For example, '[FooClass.FooClass]' structurally
// "looks like" an unnamed constructor, so we should allow it here.
filter = commentReference.hasCallableHint ? _requireCallable : (_) => true;
} else if (commentReference.hasConstructorHint &&
commentReference.hasCallableHint) {
allowTree = (_) => true;
// This takes precedence over the callable hint if both are present --
// pick a constructor if and only constructor if we see `new`.
filter = _requireConstructor;
} else if (commentReference.hasCallableHint) {
if (commentReference.hasCallableHint) {
allowTree = (_) => true;
// Trailing parens indicate we are looking for a callable.
filter = _requireCallable;
} else {
if (!commentReference.allowUnnamedConstructorParameter) {
allowTree = _rejectUnnamedAndShadowingConstructors;
} else {
allowTree = (_) => true;
}
// Without hints, reject unnamed constructors and their parameters to force
// resolution to the class.
filter = _rejectUnnamedAndShadowingConstructors;
allowTree = (_) => true;
// Neither reject, nor require, an unnamed constructor in the event the
// comment reference structure implies one. (We cannot require it in case a
// library name is the same as a member class name and the class is the
// intended lookup).
filter = commentReference.hasCallableHint
? _requireCallable
// Without hints, reject unnamed constructors and their parameters to
// force resolution to the class.
: filter = _rejectUnnamedAndShadowingConstructors;
}
var lookupResult = element.referenceBy(commentReference.referenceBy,
allowTree: allowTree, filter: filter);
Expand Down
5 changes: 1 addition & 4 deletions lib/src/matching_link_result.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
// BSD-style license that can be found in the LICENSE file.

import 'package:dartdoc/src/model/comment_referable.dart';
import 'package:dartdoc/src/model/model.dart';

class MatchingLinkResult {
final CommentReferable? commentReferable;
Expand All @@ -19,8 +18,6 @@ class MatchingLinkResult {

@override
String toString() {
// TODO(srawlins): Scrap the 'new' keyword?
final newKeyword = commentReferable is Constructor ? 'new ' : '';
return 'element: [$newKeyword${commentReferable?.fullyQualifiedName}]';
return 'element: [${commentReferable?.fullyQualifiedName}]';
}
}
10 changes: 1 addition & 9 deletions test/comment_referable/parser_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,10 @@ void main() {
void expectParseEquivalent(String codeRef, List<String> parts,
{bool constructorHint = false, bool callableHint = false}) {
var result = CommentReferenceParser(codeRef).parse();
var hasConstructorHint =
result.isNotEmpty && result.first is ConstructorHintStartNode;
var hasCallableHint =
result.isNotEmpty && result.last is CallableHintEndNode;
var stringParts = result.whereType<IdentifierNode>().map((i) => i.text);
expect(stringParts, equals(parts));
expect(hasConstructorHint, equals(constructorHint));
expect(hasCallableHint, equals(callableHint));
}

Expand Down Expand Up @@ -52,15 +49,12 @@ void main() {
test('Check that basic references parse', () {
expectParseEquivalent('Funvas.u', ['Funvas', 'u']);
expectParseEquivalent('valid', ['valid']);
expectParseEquivalent('new valid', ['valid'], constructorHint: true);
expectParseEquivalent('valid()', ['valid'], callableHint: true);
expectParseEquivalent('const valid()', ['valid'], callableHint: true);
expectParseEquivalent('final valid', ['valid']);
expectParseEquivalent('this.is.valid', ['this', 'is', 'valid']);
expectParseEquivalent('this.is.valid()', ['this', 'is', 'valid'],
callableHint: true);
expectParseEquivalent('new this.is.valid', ['this', 'is', 'valid'],
constructorHint: true);
expectParseEquivalent('const this.is.valid', ['this', 'is', 'valid']);
expectParseEquivalent('final this.is.valid', ['this', 'is', 'valid']);
expectParseEquivalent('var this.is.valid', ['this', 'is', 'valid']);
Expand All @@ -79,10 +73,8 @@ void main() {
expectParsePassthrough('operatorThingy');
expectParseEquivalent('operator+', ['+']);
expectParseError('const()');
// TODO(jcollins-g): might need to revisit these two with constructor
// tearoffs?
expectParsePassthrough('new');
expectParseError('new()');
expectParseEquivalent('new()', ['new'], callableHint: true);
});

test('Check that operator references parse', () {
Expand Down
24 changes: 0 additions & 24 deletions test/end2end/model_special_cases_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -112,40 +112,16 @@ void main() {
});

test('reference regression', () {
expect(referenceLookup(constructorTearoffs, 'A.A'),
equals(MatchingLinkResult(Anew)));
expect(referenceLookup(constructorTearoffs, 'new A()'),
equals(MatchingLinkResult(Anew)));
expect(referenceLookup(constructorTearoffs, 'A()'),
equals(MatchingLinkResult(Anew)));
expect(referenceLookup(constructorTearoffs, 'B.B'),
equals(MatchingLinkResult(Bnew)));
expect(referenceLookup(constructorTearoffs, 'new B()'),
equals(MatchingLinkResult(Bnew)));
expect(referenceLookup(constructorTearoffs, 'B()'),
equals(MatchingLinkResult(Bnew)));
expect(referenceLookup(constructorTearoffs, 'C.C'),
equals(MatchingLinkResult(Cnew)));
expect(referenceLookup(constructorTearoffs, 'new C()'),
equals(MatchingLinkResult(Cnew)));
expect(referenceLookup(constructorTearoffs, 'C()'),
equals(MatchingLinkResult(Cnew)));
expect(referenceLookup(constructorTearoffs, 'D.D'),
equals(MatchingLinkResult(Dnew)));
expect(referenceLookup(constructorTearoffs, 'new D()'),
equals(MatchingLinkResult(Dnew)));
expect(referenceLookup(constructorTearoffs, 'D()'),
equals(MatchingLinkResult(Dnew)));
expect(referenceLookup(constructorTearoffs, 'E.E'),
equals(MatchingLinkResult(Enew)));
expect(referenceLookup(constructorTearoffs, 'new E()'),
equals(MatchingLinkResult(Enew)));
expect(referenceLookup(constructorTearoffs, 'E()'),
equals(MatchingLinkResult(Enew)));
expect(referenceLookup(constructorTearoffs, 'F.F'),
equals(MatchingLinkResult(Fnew)));
expect(referenceLookup(constructorTearoffs, 'new F()'),
equals(MatchingLinkResult(Fnew)));
expect(referenceLookup(constructorTearoffs, 'F()'),
equals(MatchingLinkResult(Fnew)));
});
Expand Down
Loading