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.24k stars 1.57k forks source link

code completion ranks type names over constructors for named params #43854

Open pq opened 4 years ago

pq commented 4 years ago

Example:

//a.dart
class A { 
  A child;
  A({this.child});
}
''');
import 'a.dart';

void main(List<String> args) {
  var a = A(
    child: ^
  );  
}

In this case, the type name A ranks higher (57) than the constructor A() (40).

bwilkerson commented 4 years ago

It isn't entirely clear to me that this is a bug, so I'll explain why.

The relevance of these two elements is largely controlled by the element_kind feature. That feature uses the location in which an element is being referenced and the kind of element being referenced in order to look up the relevance score in a table. Given the location in the example above, the portion of the relevance table being used is here (https://github.com/dart-lang/sdk/blob/master/pkg/analysis_server/lib/src/services/completion/dart/relevance_tables.g.dart#L47). According to the table, a constructor occurs in this location approximately 13% of the time and a class occurs approximately 17% of the time. So, either the table is correct and classes should appear higher in the list or the table is incorrect.

The table is generated by performing a statistical analysis of a corpus of code. I can think of a couple of reasons why the table might be incorrect.

First, it's possible that the corpus of code we used is not representative of real code which could lead to the probabilities encoded in the table being wrong. It's been a while since the table was generated, so it might be worthwhile regenerating the table from an updated (and possibly expanded) corpus. If that changes the relative order of those two element types, then the problem will be solved. If not, then we might consider using a different and more representative corpus of code.

Second, the quality of the table is strongly influenced by the selection of the locations. Right now the location of interest is "the first element in a named argument in a constructor invocation". There's no distinction being made as to whether the constructor is the constructor for a Flutter widget. I believe that we ran an experiment in which such a distinction was made and found that it didn't change the relative ordering of elements, but I might be mistaken and it's possible that with an updated or more appropriate corpus the results would be different. That would need to be investigated.

Third, it's possible that the statistical analysis code that determines which element to record at each location has a bug that causes a class to be recorded more often than is appropriate. It's worth investigating this option as well.

pq commented 4 years ago

Thanks for the clarification! I do remember you explaining this a while back when I brought this up before but it didn't completely stick. From an end-user's perspective, I think what makes the ranking feel wrong is that A (the class) makes no sense in this context. Accepting the first completion will give you a static type error. Moreover, since A has no static members that produce an instance assignable to A, there is no way to use the class to get to something that makes sense.

Does ranking do any look-ahead? That is, is there any way to rank classes w/ members (or members that produce) an object of an assignable type higher than ones that don't?

For example:

class A {
  static a = A();
  static A create() => A();
}

vs.

class A {
}

Does ranking know about type assignability?

bwilkerson commented 4 years ago

Does ranking do any look-ahead? That is, is there any way to rank classes w/ members (or members that produce) an object of an assignable type higher than ones that don't?

Not currently, no. My expectation (which might be wrong) is that it will be too expensive. In part because we might have to traverse and unbounded number of "links". You might have a class A that has a static member that returns a B, which has a static member that returns a C, ..., that has a static member that returns an A.

But just for the record, in an in-person discussion we decided that it would be worth exploring a new feature based on whether a candidate class has any non-constructor static members. Those for which there are no static members seem less likely to be useful completions than any constructors that those classes might declare. The feature would probably need to only apply in places where a type annotation isn't allowed, but I think that's also doable.

Does ranking know about type assignability?

Yes, there's a feature for that (context_type).

CMMT20 commented 2 years ago

Hello, i have the same issue, so i put it here, but i think i made more elaborated explanation, i hope it helps to go to the solution.

you can see it here https://github.com/dart-lang/sdk/issues/48600#issue-1172631973

scheglov commented 8 months ago

I completely agree, in many cases there is no reason to suggest type names. This is especially true in the Flutter example above. So, I'm going to send some time to gather statistics to improve our relevance computing heuristics.