Renaming a symbol renames unrelated doc references all over the place.

[gspencergoog]

Description

If I have code like this in file1.dart:

class MyClass {
  int field;
}

And code like this in file2.dart:

class MyOtherClass {
  static int field;
}

And a comment like this in file3.dart:

/// Refer to [MyOtherClass.field].

And, crucially, file3.dart does not include file2.dart (but it's still a valid doc reference).

If I then rename MyClass.field to be MyClass.somethingElse using the IDE, I get the following change in file3.dart:

/// Refer to [MyOtherClass.somethingElse].

For a real world example of this, open the file packages/flutter/lib/src/material/debug.dart in the Flutter repo and insert this after the imports at the top:

class MyWidget extends StatelessWidget {
  const MyWidget({this.children = const <Widget>[]});

  final List<Widget> children;

  @override
  Widget build(BuildContext context) {
    return Column(children: children);
  }
}

Then rename children to orphans, and look at the diffs in the repo. You'll find that it modified at least four other files that are completely unrelated. This has bit me a bunch of times when I go to submit a change and see unrelated changes where I have to scratch my head and wonder where they came from.

I suspect this is because the analyzer is looking for the symbols, and since the symbols aren't imported into the file, it's just doing something "smart" with the references it finds that (sort of) match. I find it interesting that even if the symbol is a qualified symbol (e.g. [MyOtherClass.field]), it will still rename it.

I wish it wouldn't do these unknown symbol doc comment link renames, or at least that it would ask me before doing it.

IntelliJ doesn't have this problem, I think because if it can't tell for sure that a symbol matches (no analyzer link for it, I think), it'll go to an intermediary mode where it asks for confirmation and you can skip some of the renames. It groups them into "comments", "doc comments" and "code" too, so you can just skip the unrelated comment changes. It's annoying to hit that confirmation mode, but I haven't had problems with unrelated changes like this.

[DanTup]

And, crucially, file3.dart does not include file2.dart (but it's still a valid doc reference).

My understanding is that these references are not resolved (at least by the analyzer). For example you can't ctrl-click on them to navigate (there's an open issue about this at dart-lang/sdk#27471).

However, that's ofcourse no reason for them to be renamed. I was able to repro this issue with any unresolved symbol even in a single file:

class A {
  int? field; // Rename this.
}

class B {
  int? field;
}

/// Resolved reference to [B.field] is correctly not renamed.
/// Unresolved reference to [C.field] is incorrectly renamed.
var a;

[DanTup]

It seems the server specifically computes these edits for unresolved identifiers with the same name. I started on a fix to exclude them from LSP while there's no UI to be able to opt-in to them (something we might be able to do if we adopt LSP's annotated edits) because it feels odd to rename things that are not known to be references to the renamed symbol:

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

Although, now I'm wondering whether that's the right call. Generally I wouldn't expect there to be many unresolved identifiers. Perhaps they're sometimes in the code you're typing (where you're doing the rename) and they might not be included without this. It's showing up here because the dartdoc reference is unresolved, and that seems like something that should be fixed (by adding an import, or later if dart-lang/sdk#27471 is supported).

@gspencergoog @bwilkerson any thoughts/opinions on the default behaviour here (while we have no way for the user to choose on a per-rename basis)?

[bwilkerson]

The legacy protocol has a potentialEdits field for this very reason.

Generally I wouldn't expect there to be many unresolved identifiers.

That's true, though it used to be less true in Dart 1.0 when dynamic was much more common. But that doesn't completely negate the issue that sometimes the receiver is dynamic and we have no way of knowing whether the reference is to the element being renamed.

It's probably more common in comment references because the analyzer and dartdoc have diverged in the way they handle comment references.

I don't know whether it's more common for an unresolved identifier to need to be renamed or need to be left unchanged. I suspect, though, that overall it would be less surprising to users for questionable references to remain unchanged.

[DanTup]

Ah, I forgot that Dart 1 was more dynamic and thought potential edits might have been more to handle incomplete/invalid code, but I guess it was more for that.

Not renaming sounds sensible to me then, I've sent the CL for review. Thanks!

[DanTup]

Fixed by dart-lang/sdk@75b17a1 (will ship in an SDK update).

[gspencergoog]

In Flutter, we have these kinds of unresolved symbols all over the place, since we refer to "layer breaking" symbols from documentation that can't import the packages that the symbol is part of. For instance, in the WidgetsApp documentation, there are references like:

/// It is used by both [MaterialApp] and [CupertinoApp] to implement base
/// functionality for an app.

We don't want to import the later libraries (material and cupertino) into the file since that violates our layering, but we do want to refer to them in docs.

If we stop renaming these unresolved references, it will probably make for more instances where we have to search for them ourselves after doing the rename, but at least it won't be surprising.

Ideally, I'd really like for the analyzer to analyze dartdoc references the same way as dartdoc does, although I realize that is a huge lift: it would mean analyzing document comments in an entirely different context from the rest of the file. Maybe this will be easier later when dartdoc operations are more integrated with the analyzer.

[DanTup]

We don't want to import the later libraries (material and cupertino) into the file since that violates our layering, but we do want to refer to them in docs.

Thanks for the explanation, that makes sense. With the change landed above, I think it will behave as you expect (although for now, if you rename the actual fields referenced in those docs, they won't be updated - that might change if dart-lang/sdk#27471 is implemented and the references are updated to whatever format that requires).