From 6d1087411cc50c75dca0305d7ee1d803112f2433 Mon Sep 17 00:00:00 2001 From: Sam Rawlins Date: Mon, 16 Oct 2023 11:52:46 -0700 Subject: [PATCH] Remove support for deprecated leading new in comment references --- .../model_comment_reference.dart | 55 +--------- lib/src/comment_references/parser.dart | 26 +---- lib/src/markdown_processor.dart | 43 ++------ lib/src/matching_link_result.dart | 5 +- test/comment_referable/parser_test.dart | 10 +- test/end2end/model_special_cases_test.dart | 24 ---- test/end2end/model_test.dart | 103 ++++++++++-------- testing/test_package/lib/fake.dart | 7 +- 8 files changed, 76 insertions(+), 197 deletions(-) diff --git a/lib/src/comment_references/model_comment_reference.dart b/lib/src/comment_references/model_comment_reference.dart index ff39175c01..4139628199 100644 --- a/lib/src/comment_references/model_comment_reference.dart +++ b/lib/src/comment_references/model_comment_reference.dart @@ -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 get referenceBy; Element? get staticElement; @@ -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().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 get referenceBy => parsed .whereType() diff --git a/lib/src/comment_references/parser.dart b/lib/src/comment_references/parser.dart index 69ce653da1..b21be90184 100644 --- a/lib/src/comment_references/parser.dart +++ b/lib/src/comment_references/parser.dart @@ -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 - /// ::= - /// | + /// ::= /// - /// ::= 'new ' - /// - /// ::= ('const' | 'final' | 'var')(' '+) + /// ::= ('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; @@ -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); @@ -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; diff --git a/lib/src/markdown_processor.dart b/lib/src/markdown_processor.dart index 0fb9611e80..9853c34d26 100644 --- a/lib/src/markdown_processor.dart +++ b/lib/src/markdown_processor.dart @@ -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); @@ -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); diff --git a/lib/src/matching_link_result.dart b/lib/src/matching_link_result.dart index 98a3de446d..b4a25261b2 100644 --- a/lib/src/matching_link_result.dart +++ b/lib/src/matching_link_result.dart @@ -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; @@ -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}]'; } } diff --git a/test/comment_referable/parser_test.dart b/test/comment_referable/parser_test.dart index 4eae8647dd..f01b1e9c75 100644 --- a/test/comment_referable/parser_test.dart +++ b/test/comment_referable/parser_test.dart @@ -9,13 +9,10 @@ void main() { void expectParseEquivalent(String codeRef, List 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().map((i) => i.text); expect(stringParts, equals(parts)); - expect(hasConstructorHint, equals(constructorHint)); expect(hasCallableHint, equals(callableHint)); } @@ -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']); @@ -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', () { diff --git a/test/end2end/model_special_cases_test.dart b/test/end2end/model_special_cases_test.dart index 10506e52d2..246b0386c5 100644 --- a/test/end2end/model_special_cases_test.dart +++ b/test/end2end/model_special_cases_test.dart @@ -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))); }); diff --git a/test/end2end/model_test.dart b/test/end2end/model_test.dart index 542920a7a7..e34bafb63b 100644 --- a/test/end2end/model_test.dart +++ b/test/end2end/model_test.dart @@ -1329,10 +1329,6 @@ void main() { aFunctionUsingRenamedLib.documentationAsHtml, contains( 'Link to library: renamedLib')); - expect( - aFunctionUsingRenamedLib.documentationAsHtml, - contains( - 'Link to constructor (implied): new renamedLib.YetAnotherHelper()')); expect( aFunctionUsingRenamedLib.documentationAsHtml, contains( @@ -1342,9 +1338,13 @@ void main() { contains( 'Link to class: renamedLib.YetAnotherHelper')); expect( - aFunctionUsingRenamedLib.documentationAsHtml, - contains( - 'Link to constructor (direct): renamedLib.YetAnotherHelper.YetAnotherHelper')); + aFunctionUsingRenamedLib.documentationAsHtml, + contains( + 'Link to constructor (direct): ' + '' + 'renamedLib.YetAnotherHelper.new', + ), + ); expect( aFunctionUsingRenamedLib.documentationAsHtml, contains( @@ -2666,9 +2666,10 @@ void main() { test('in class scope overridden by constructors when specified', () { expect( - referenceLookup(FactoryConstructorThings, - 'new FactoryConstructorThings.anotherName'), - equals(MatchingLinkResult(anotherName))); + referenceLookup(FactoryConstructorThings, + 'FactoryConstructorThings.anotherName()'), + equals(MatchingLinkResult(anotherName)), + ); }); test( @@ -2699,9 +2700,10 @@ void main() { equals(MatchingLinkResult(anotherConstructor))); // A conflicting constructor has to be explicit. expect( - referenceLookup( - anotherName, 'new FactoryConstructorThings.anotherName'), - equals(MatchingLinkResult(anotherName))); + referenceLookup( + anotherName, 'FactoryConstructorThings.anotherName()'), + equals(MatchingLinkResult(anotherName)), + ); }); test('in method scope referring to parameters and variables', () { @@ -2749,18 +2751,22 @@ void main() { }); test( - 'Verify that constructors do not override member fields unless explicitly specified', - () { - expect(referenceLookup(baseForDocComments, 'aConstructorShadowed'), - equals(MatchingLinkResult(aConstructorShadowedField))); + 'Verify that constructors do not override member fields unless ' + 'explicitly specified', () { expect( - referenceLookup( - baseForDocComments, 'BaseForDocComments.aConstructorShadowed'), - equals(MatchingLinkResult(aConstructorShadowedField))); + referenceLookup(baseForDocComments, 'aConstructorShadowed'), + equals(MatchingLinkResult(aConstructorShadowedField)), + ); expect( - referenceLookup(baseForDocComments, - 'new BaseForDocComments.aConstructorShadowed'), - equals(MatchingLinkResult(aConstructorShadowed))); + referenceLookup( + baseForDocComments, 'BaseForDocComments.aConstructorShadowed'), + equals(MatchingLinkResult(aConstructorShadowedField)), + ); + expect( + referenceLookup( + baseForDocComments, 'BaseForDocComments.aConstructorShadowed()'), + equals(MatchingLinkResult(aConstructorShadowed)), + ); }); test('Deprecated lookup styles still function', () { @@ -2770,9 +2776,9 @@ void main() { test('Verify basic linking inside class', () { expect( - referenceLookup( - baseForDocComments, 'BaseForDocComments.BaseForDocComments'), - equals(MatchingLinkResult(defaultConstructor))); + referenceLookup(baseForDocComments, 'BaseForDocComments.new'), + equals(MatchingLinkResult(defaultConstructor)), + ); // We don't want the parameter on the default constructor, here. expect( @@ -3728,20 +3734,21 @@ String? topLevelFunction(int param1, bool param2, Cool coolBeans, test('indentation is not lost inside indented code samples', () { expect( - aProperty.documentation, - equals( - 'This property is quite fancy, and requires sample code to understand.\n' - '\n' - '```dart\n' - 'AClassWithFancyProperties x = new AClassWithFancyProperties();\n' - '\n' - 'if (x.aProperty.contains(\'Hello\')) {\n' - ' print("I am indented!");\n' - ' if (x.aProperty.contains(\'World\')) {\n' - ' print ("I am indented even more!!!");\n' - ' }\n' - '}\n' - '```')); + aProperty.documentation, + equals( + 'This property is quite fancy, and requires sample code to understand.\n' + '\n' + '```dart\n' + 'AClassWithFancyProperties x = AClassWithFancyProperties();\n' + '\n' + 'if (x.aProperty.contains(\'Hello\')) {\n' + ' print("I am indented!");\n' + ' if (x.aProperty.contains(\'World\')) {\n' + ' print ("I am indented even more!!!");\n' + ' }\n' + '}\n' + '```'), + ); }); test('Docs from inherited implicit accessors are preserved', () { @@ -4308,13 +4315,17 @@ String? topLevelFunction(int param1, bool param2, Cool coolBeans, test('calculates comment references to classes vs. constructors correctly', () { expect( - referToADefaultConstructor.documentationAsHtml, - contains( - 'ReferToADefaultConstructor')); + referToADefaultConstructor.documentationAsHtml, + contains( + '' + 'ReferToADefaultConstructor'), + ); expect( - referToADefaultConstructor.documentationAsHtml, - contains( - 'ReferToADefaultConstructor.ReferToADefaultConstructor')); + referToADefaultConstructor.documentationAsHtml, + contains( + '' + 'ReferToADefaultConstructor.new'), + ); }); test('displays generic parameters correctly', () { diff --git a/testing/test_package/lib/fake.dart b/testing/test_package/lib/fake.dart index bce6e6298f..fc0bda08fd 100644 --- a/testing/test_package/lib/fake.dart +++ b/testing/test_package/lib/fake.dart @@ -104,10 +104,9 @@ abstract class BaseThingy2 implements BaseThingy { /// This function has a link to a renamed library class member. /// /// Link to library: [renamedLib] -/// Link to constructor (implied): [new renamedLib.YetAnotherHelper()] /// Link to constructor (implied, no new): [renamedLib.YetAnotherHelper()] /// Link to class: [renamedLib.YetAnotherHelper] -/// Link to constructor (direct): [renamedLib.YetAnotherHelper.YetAnotherHelper] +/// Link to constructor (direct): [renamedLib.YetAnotherHelper.new] /// Link to class member: [renamedLib.YetAnotherHelper.getMoreContents] /// Link to function: [renamedLib.helperFunction] /// Link to overlapping prefix: [renamedLib2.theOnlyThingInTheLibrary] @@ -243,7 +242,7 @@ class AClassWithFancyProperties { /// This property is quite fancy, and requires sample code to understand. /// /// ```dart - /// AClassWithFancyProperties x = new AClassWithFancyProperties(); + /// AClassWithFancyProperties x = AClassWithFancyProperties(); /// /// if (x.aProperty.contains('Hello')) { /// print("I am indented!"); @@ -1030,7 +1029,7 @@ void paintImage2(String fooParam, /// This is to test referring to a constructor. /// /// This should refer to a class: [ReferToADefaultConstructor]. -/// This should refer to the constructor: [ReferToADefaultConstructor.ReferToADefaultConstructor]. +/// This should refer to the constructor: [ReferToADefaultConstructor.new]. class ReferToADefaultConstructor { /// A default constructor. ReferToADefaultConstructor();