dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.31k stars 1.59k forks source link

Elements names are shadowed on autocomplete and usability #56821

Open FMorschel opened 2 months ago

FMorschel commented 2 months ago

Partially inspired by https://github.com/Dart-Code/Dart-Code/issues/5242.

If you have the following code:

class C {
  void foo(int value) {
    bar                   // <<-- Referencing line
  }

  static void bar() {}
}

extension EC on C {
  int get bar => 0;
}

int bar() => 0;

There is no current indication that either the top declaration bar exists or that we could do EC(this).bar to use the declaration of bar for the extension.

This is all working as intended, of course, but I'd like to see a differentiation on the autocomplete options so the user can easily see that there is name shadowing here.

This is even more crucial in cases like:

a.dart

class C {}
class MyClass {}

thing.dart

class A {
  late int bar;
}

other.dart

import 'a.dart' as bar;
import 'thing.dart';

class C extends A {
  void foo(int value) {
    this.bar.isEven;
    bar
  }
}

This is the current output of the autocomplete:

image

If I add the .

image

In this case, we could easily surpass this by adding the this. at the start similar to what we do with aliased imports and its elements, but we have no indication of it currently.

As a reference, I stumbled upon this issue in the last case, so it took me a while to figure out that my variable was being shadowed by the import.


I'm not sure what we could do for:

import 'a.dart' as bar;

class C {
  late int bar;
}

Since both are valid things but in this case if I was trying to use the import inside C I would never be able to unless I renamed it (which we would probably want) but then it is not easy to see that this is what the user is supposed to do.


@DanTup

dart-github-bot commented 2 months ago

Summary: The issue reports that Dart's autocomplete doesn't clearly indicate name shadowing, making it difficult for users to identify and resolve conflicts between variables, imports, and extensions. The user suggests improving autocomplete by differentiating shadowed elements, potentially by adding a visual cue or a "this." prefix.

bwilkerson commented 2 months ago

There is no current indication that either the top declaration bar exists ...

There's no completion suggestion because there's no way to access it. That is, there's no valid code you can write at that location to get around the fact that the local declaration hides the top-level declaration.

... or that we could do EC(this).bar to use the declaration of bar for the extension.

Yes, we could add a case for that. I would only want to suggest that form when the extension method can't be reached because it's hidden. It's not a form of code that we want to promote.

I'm not sure code completion is written in such as way as to make it performant to add this support. Something that would require some investigation.

In this case, we could easily surpass this by adding the this. at the start ...

I'd be ok with that, but again, only in cases where the this. is necessary to access something that would otherwise be hidden.

FMorschel commented 2 months ago

There's no completion suggestion because there's no way to access it. That is, there's no valid code you can write at that location to get around the fact that the local declaration hides the top-level declaration.

Yes, I know.

The point I was trying to bring here was that we could maybe think of a way for us to tell the user things are being hidden. I'm not sure how, but that is the gist of this issue.

Also, if someone can think of a better name for this please feel free to update it, I could not think of a name that would satisfy me.

DanTup commented 2 months ago

I like the idea of including this.bar in the case where bar would be something different (possibly super. would also work). Same for E(this) for extension members too.

I'm less sure there's a simple solution to accessing the local function though. I think there is a way it could work, but it's weird and I don't know if it would be used enough to be worthwhile making it work.

You can use import ''; to import the current file. That that means you can use import '' as me; to import the current library as me, which would then allow accessing me.bar(). So in theory, we could include me.bar() in the completion, and have an auto-import that adds it (at least, if we ignore for a minute that we don't currently support prefixes on completion auto-imports).

I'm not sure if it would be very obvious to users if they saw me.bar (or whatever we called me), and if they did accept it, I'm not sure they would understand why they have an import '' because I doubt using it is very common (probably it could also be its own filename too, though I'm not certain if they're always equivalent). Maybe it'd be slightly nicer if Dart had an identifier that resolved to the current library without needing that extra import... that would avoid the need for any additional import. I think this might be similar to C#'s global::.

bwilkerson commented 2 months ago

If the problem is shadowing, then we could potentially introduce a lint to flag all shadowing (we do have lints for some specific kinds of shadowing).

Or we could add a highlight modifier to allow users to color declarations that shadow another declaration.

But I don't think that code completion is the right way to solve this particular problem.

DanTup commented 2 months ago

I like both of those ideas - a lint would allow you to forbid having conflicting names if you wanted, or you could use colouring if you just wanted them to be more visible in code :-)

FMorschel commented 2 months ago

But I don't think that code completion is the right way to solve this particular problem.

It could solve part of this problem, as we discussed, but probably you're right that it will not solve all of it. Maybe a lint, as you said, when the user writes bar for the above code on the scope that has shadowing to let them know there are other possibilities? Because of things like the second example (import and variable inside a superclass) there is no way of knowing about that I don't think.


FYI:

You can use import ''; to import the current file. That that means you can use import '' as me; to import the current library as me, which would then allow accessing me.bar(). So in theory, we could include me.bar() in the completion, and have an auto-import that adds it (at least, if we ignore for a minute that we don't currently support prefixes on completion auto-imports).

There is a discussion on using this kind of workarounds here: Should "self" importing trigger unnecessary_import