Renaming a symbol adds an extra dot in Dart doc comments, breaks link between symbol and reference

[rmkl]

Describe the bug
Renaming a named constructor or a factory results in duplicating the dot in any doc comment that is referencing that symbol. It breaks the link between the symbol and the reference for any subsequent renaming.

To Reproduce
Steps to reproduce the behavior:

1. Create a new class SomeClass
2. Add a named constructor SomeClass.namedConstructor
3. Add a doc comment to the named constructor that includes a Dart doc reference [SomeClass.namedConstructor]
4. Right-click the name of the named constructor in the code
5. Select 'Rename Symbol' (F2)
6. Change the name to 'anotherName'
7. Notice how an extra dot is added to the reference in the doc comment [SomeClass..anotherName]
8. The link between the symbol and the reference is the doc comment is now lost.

• The steps to reproduce the issue for a factory are the same.
• Disabling all extensions, except the Dart extension, does not resolve the problem.

Versions (please complete the following information):

• VS Code version: 1.49.1
• Dart extension version: 3.14.1
• Dart/Flutter SDK version: Flutter 1.21.0-9.2.pre • channel beta, Dart SDK version: 2.10.0-7.3.beta

[DanTup]

@bwilkerson I can reproduce this in a test. The issue seems to be that the source range for a named constructor returned by searchEngine.searchReferences here:

https://github.com/dart-lang/sdk/blob/1134a0f17c84e8af62990aff441363ab635e139c/pkg/analysis_server/lib/src/services/refactoring/rename_constructor.dart#L58

Does not include the leading period. However the declaration reference and the replacement text seem to assume ranges should include the leading period (since the replacement text includes it). It seems like the range is coming from the index here:

https://github.com/dart-lang/sdk/blob/578d0c081fbcb5c516d1574cd6a09166a96f99d3/pkg/analyzer/lib/src/dart/analysis/search.dart#L757

I'm not sure of the best way to fix - changing the result of the search engine seems like it could be risky (if other things expect it to be the name without the period). Do you know what the expectation is, and which bit is likely incorrect? I could update rename_constructor to assume there's no dot, though there are two other places it gets ranges (one is _createDeclarationReference which is easy to update, though the other - in _replaceSynthetic seems a little less straight forward because it comes via utils.prepareNewConstructorLocation.

Another option (though I'm not sure this is really "clean") could be to check the length of the range, and include the period (or not) based on whether its length matches the old name with or without the period.

For reference, here's a test (for test/services/refactoring/rename_constructor_test.dart):

  @soloTest
  Future<void> test_docReference() async {
    await indexTestUnit('''
class SomeClass {
  /// Docs for [SomeClass.namedConstructor].
  SomeClass.namedConstructor(); // marker
}
''');
    // configure refactoring
    _createConstructorDeclarationRefactoring('namedConstructor(); // marker');
    expect(refactoring.refactoringName, 'Rename Constructor');
    expect(refactoring.elementKindName, 'constructor');
    expect(refactoring.oldName, 'namedConstructor');
    // validate change
    refactoring.newName = 'newConstructor';
    return assertSuccessfulRefactoring('''
class SomeClass {
  /// Docs for [SomeClass.newConstructor].
  SomeClass.newConstructor();  // marker
}
''');
  }

[bwilkerson]

The issue seems to be that the source range for a named constructor returned by searchEngine.searchReferences ... does not include the leading period.

That seems appropriate to me. If the search engine is being used to find references to the constructor those ranges will be the range to which the user will navigate, and it seems appropriate that it doesn't include the period.

Do you know what the expectation is, and which bit is likely incorrect?

I believe that the search engine is correct and that it's the refactoring that needs to be updated.

I could update rename_constructor to assume there's no dot ...

It isn't quite that simple, unfortunately, because of unnamed constructors. The rename refactoring allows users to rename a named constructor to a named constructor (both have a period so it should be left as is), a named constructor to an unnamed constructor (the period needs to be removed), or an unnamed constructor to a named constructor (the period needs to be added). It sounds like the bug is in the first case.

[DanTup]

Ah, good point. Though it does seem like there is some inconsistency in the search engine results. I updated my sample to this:

class SomeClass {
  /// Docs for [SomeClass.namedConstructor].
  SomeClass.namedConstructor(); // marker

  final instance = SomeClass.namedConstructor();
}

Here there are two references to the named constructor (one in the dartdoc, one in a call to the constructor for the instance field). When I rename namedConstructor, only the dartdoc is updated incorrectly to end up with two periods. If I examine the matches from the search engine, they have different lengths (the constructor call does include the period - see the expression eval results at the bottom - one length is 16, one is 17):

I don't know if this is expected - should the search engine results be different for these? (I wouldn't have thought so - though if they are, the fix in the refactor would need to account for this difference).

[bwilkerson]

I wouldn't have expected them to be different. That might be a bug in the indexer, but @scheglov might know better.

[DanTup]

@scheglov do you have any thoughts on the above? Thanks!

[scheglov]

Yes, this looks like a bug in the refactoring or the indexer, I can reproduce it in a unit test.

[scheglov]

https://dart-review.googlesource.com/c/sdk/+/181260

[DanTup]

@scheglov great, thanks!