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.27k stars 1.58k forks source link

Code-completion should recognize enum-like classes and constructor like functions. #34847

Open lrhn opened 6 years ago

lrhn commented 6 years ago

The current code completion suggests EnumClass.foo in an EnumClass typed context and Foo.constructorName in a Foo typed context. For all other static members of a class, you have to first write the ClassName before code completion will suggest the static members.

I suggest expanding this favoritism:

That will treat any final static value (or even just the const ones) on a class with that class's type as a canonical instance of the class, and treat any static method on a class returning that class's type as a factory method.

This would allow user written enum classes to match language level enums in discoverability and usability, and it would allow allow factory functions to be recognized without them needing to be factory constructors.

It will also, for example, mean that double x = will suggest double.nan and double.parse, which are both classical examples of double expression you might want to complete with. Parse functions currently have worse discoverability than constructors (say, for Uri), even if they are likely to be the correct function to call in many cases.

zoechi commented 6 years ago

What about static final or const fields of type int, double, num, String, bool for enum-like classes of those types?

lrhn commented 6 years ago

@zoechi I don't see how to find those. The hint provided above is the context type, so you can check that exact type for constants or factories. If the context type is int, I have no clue where to look for constants of type int (there are likely many).

If we had type aliases with static members, say;

typedef MyAliasForInt = int {
   static const foo = 1;
   static const bar = -1;
}

then we might be able to complete those in a MyAliasForInt context. There is no MyAliasForInt type at the language level (it's just an alias for int), but static tools may be clever enough to make the distinction.

zoechi commented 6 years ago

@lrhn If it is only about autocompletion, then an annotation could do that

@Enum<int>()
abstract class MyEnum {
  const int foo = 1;
  const int bar = 2;

  const bool otherConstMember = false;

  MyEnum._();
}

MyE should show the suggestions

MyEnum.foo
MyEnum.bar

This would make writing of the already verbose old-style enums even more cumbersome though. But writing usually is less of an issue than discoverability and readability.

Type alias looks good, but I'd still prefer having the full capabilities of Dart classes available for enums.

The ideal situation in my opinion would be if

enum  Color {
  red, green, blue
}

would lead to a class like

abstract class Color implements Enum {
  static const int red = Color(0);
  static const int green = Color(1);
  static const int blue = Color(2);

  static const values = [red, green, blue]; 

  // value could be `index` but might be weird if other types then `int` should be supported here
  final int value;
  const Color._(this.value);

  factory Color.fromValue(int value) => values.firstWhere((v) => e.value == value); // or a switch/case for performance

  @override
  String toString() => '${super.toString()} - $value';
}

and would allow to add everything that a normal class would allow except what would prevent the class from being used as enum like non-const constructors and non-final fields.

enum  Color {
  red = Color(2, 'Red'), 
  green = Color(5, 'Green'), 
  blue = Color(9, 'Blue');

  // additional static members
  static const lightColors = [red, green];

  // additional field 
  final String name;

  // a different type for `value` (index in current Dart enums) might be useful as well not sure 
  // final double value;

  // custom default enum constructor
  // `const` could be implicit
  const Color._(this.value, this.name); 

  // additional factory constructor
  factory Color.fromName(String name) => values.firstWhere((v) => e.name == name);

  // additional getter or method
  String get nameAndValue => '$name - $value' 

  // custom toString
  @override
  String toString() => 'Color $name (value)';
}

For issues discussed in https://github.com/dart-lang/language/issues/50 (I don't know if I fully understand the related requirements though) If int values should be used instead of instance it could be Color.blue.value and foo(Color.fromValue(5)) (referring to https://github.com/dart-lang/language/issues/50#issuecomment-430763406)

I don't know if there are plans to add support for custom casts to Dart that would allow

void foo(Color a) {
  print(a.name);
}

void main() {
  foo(5);
}

to make conversion between int and Color implicit.

ThinkDigitalSoftware commented 5 years ago

What I would like to add to this, if we could annotate classes with enum, is to provide type checking as enums do. If I have a class

class Colors {
  static const String red = "red";
  static const String blue = "blue";
  List<String> values = [red, blue];
}

I would like to run a check to see if the value I'm encountering was from my Colors class, even though it evaluates to a String. Currently, I have to assert that it's a String, which will pass if the user passes "Yellow" as a parameter, even though that's not part of my enum class. Or I have to make a values List that contains all the values and then check to see if it contains that value.

ThinkDigitalSoftware commented 5 years ago

Something like this? I have no idea about any of the technical details about implementation, but being able to extend an enum would be great. https://pub.dartlang.org/documentation/enums/latest/enums/Enum-class.html

DanTup commented 3 years ago

I think this is partially done in #40699. If the servers "Suggestion Sets" are enabled and the class is in another file, the static members are suggested via the suggestion sets:

Screenshot 2021-02-24 at 11 59 16 Screenshot 2021-02-24 at 12 00 24

It doesn't work if they're in the same file (see MyColor2) or imported when suggestion sets are disabled yet though.

scheglov commented 8 months ago

With b253c4a004187f0c33082d17618e509956d048fb (added for the new / current completion protocol) we suggest any static fields.

image

The fields don't have to be declared in the same class as the context type, not A.foo above. Instead we use the type of the field to give it higher relevance if if it matches the context type, so it works mostly automatically.

If you know the name of the constant, you can start typing it.

image

What is still missing.

  1. Maybe suggesting static methods. I suspect that there are not so many of them to significantly increase the number of suggestions. And I like the consistency with (factory) constructors.
  2. Filtering not only by the name of the constant, but also by the name of the class that contains it. So, typing dou^ would keep only constants from double.
  3. It looks that currently we give local variables so much relevance that this pushes type matching suggestions down. At least in some cases this feel unfortunate, e.g. below, see how much I had to scroll and skip local variables to get to the alignment constants. I will probably need to gather some statistics to decide. image
bwilkerson commented 8 months ago
  1. Maybe suggesting static methods.

It would be good to get some data here to see how many additional suggestions we're likely to introduce.

  1. Filtering not only by the name of the constant ...

I'd love to see that. I don't know whether clients currently allow for that, though.

  1. It looks that currently we give local variables so much relevance that this pushes type matching suggestions down.

The context type (type matching) should also be used for local variables, so I would have expected that to improve the situation. (And maybe it does, just not by enough.)

But I agree that we need to do some statistical analysis. The current relevance scores are based on very old-style code and need to be updated.

DanTup commented 8 months ago

Filtering not only by the name of the constant, but also by the name of the class that contains it. So, typing dou^ would keep only constants from double.

I'm not sure if I understand this correctly, do you mean filtering for things where either the class or constant names contain the prefix (so typing dou^ would keep both constants from double and constants containing dou from other classes)? Otherwise it seems like typing "m" (as in your middle screenshot) would no longer work?

I'd love to see that. I don't know whether clients currently allow for that, though.

VS Code will always apply a fuzzy search over the "filterText" (which defaults to the label if we don't set it.. usually we set it to the label minus any signature/details). That means if you type "abc" it would match "A.bc" but we can't tell it that it must match entirely in one side of the dot (this is something I've suggested before - for ex. having filterText accept an array, but sadly nothing has come of it!)

bwilkerson commented 8 months ago

@alexander-doroshko Is this something we could also do in IntelliJ?

alexander-doroshko commented 8 months ago

Is this something we could also do in IntelliJ?

Do you mean showing one text in the completion popup and using another text for filtering? Yes, IntelliJ Platfom API supports it. Here's a nice example from JavaScript:

image
bwilkerson commented 8 months ago

That's cool!

But I think Konstantin's thought was about being able to suggest something like double.maxFinite in such a way that it would match whether the user has started to type double or maxFinite.

scheglov commented 8 months ago

We have alternatives how to filter MyClass.someField:

  1. Filter only by the text after ., so you can write soF. This is what we do now.
  2. Filter only by the text before ., so you can write MyC.
  3. Filter by both separately, so you can write either MyC or soF.
  4. Filter by both merged, full text, so you can write MyF.

The option (4) seem to be most flexible.

One issue with this, is that fuzzy matching implementation that we have in DAS, requires that the first letter must match. So, if we match soF against MyClass.someField, it will always fail. Maybe DAS should re-try with (1)? Or maybe we should make fuzzy matcher not to fail if the first letter does not match.

But this also depends on what clients will do for fuzzy matching. It does not matter what DAS will do, if the client will have different rules and filter out suggestions.

bwilkerson commented 8 months ago

It does not matter what DAS will do, if the client will have different rules and filter out suggestions.

That's not entirely true. :-) If DAS filters out a suggestion then the client can't decide to not filter it out.

So I think on the server side the right behavior is to take into account how the clients will filter. Hence my interest in knowing how the clients will behave.

alexander-doroshko commented 8 months ago

Just BTW, as far as I remember, both IntelliJ and the Analysis Server already support it if all 3 strings are different: to show in the popup, to use for filtering, and to insert if selected.

suggest something like double.maxFinite in such a way that it would match whether the user has started to type double or maxFinite

We have alternatives how to filter MyClass.someField...

I think IntelliJ already supports all 4 alternatives. Here are screenshots (with mocked result from DAS, I changed CompletionSuggestion.getCompletion() to "MyClass.someField")

image image image

If DAS filters out a suggestion then the client can't decide to not filter it out.

true :)