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

Activity

1. rmkl

   added

   is bug

   on Sep 24, 2020
2. DanTup

   added

   in lsp/analysis serverSomething to be fixed in the Dart analysis serverSomething to be fixed in the Dart analysis server

   on Sep 24, 2020
3. DanTup

   added this to the v3.16.0 milestone on Sep 24, 2020
4. DanTup commented on Oct 13, 2020

   

   DanTup

   on Oct 13, 2020

   Last edited by DanTup

   Member

   More actions

   @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
   }
   ''');
     }
5. bwilkerson commented on Oct 13, 2020

   

   bwilkerson

   on Oct 13, 2020

   More actions

   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.
6. DanTup commented on Oct 13, 2020

   

   DanTup

   on Oct 13, 2020

   Member

   More actions

   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).
7. bwilkerson commented on Oct 13, 2020

   

   bwilkerson

   on Oct 13, 2020

   More actions

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

   modified the milestones: v3.16.0, v3.17.0 on Oct 26, 2020
9. DanTup

   modified the milestones: v3.17.0, On Deck on Nov 18, 2020
10. DanTup commented on Jan 26, 2021

    

    DanTup

    on Jan 26, 2021

    Member

    More actions

    @scheglov do you have any thoughts on the above? Thanks!
11. scheglov commented on Jan 26, 2021

    

    scheglov

    on Jan 26, 2021

    More actions

    Yes, this looks like a bug in the refactoring or the indexer, I can reproduce it in a unit test.
12. scheglov commented on Jan 26, 2021

    

    scheglov

    on Jan 26, 2021

    More actions

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

    added a commit that references this issue on Jan 26, 2021

    Fix for indexing constructor references in documentation comments.

    ...

    64a8d2a
14. DanTup

    modified the milestones: On Deck, v3.20.0 on Jan 27, 2021
15. DanTup commented on Jan 27, 2021

    

    DanTup

    on Jan 27, 2021

    Member

    More actions

    @scheglov great, thanks!
16. DanTup

    closed this as completedon Jan 27, 2021