Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No completion for nextFragment #60298

Open
scheglov opened this issue Mar 10, 2025 · 6 comments
Open

No completion for nextFragment #60298

scheglov opened this issue Mar 10, 2025 · 6 comments
Labels
area-devexp Developer experience items (DevTools, IDEs, analysis server, completions, refactorings, ...). devexp-completion Issues with the analysis server's code completion feature devexp-completion-correctness Issues with the correctness of the analysis server's code completion feature P2 A bug or feature request we're likely to work on

Comments

@scheglov
Copy link
Contributor

Image

But code does not report any static analysis if I type it myself.

Image

@scheglov scheglov added the area-devexp Developer experience items (DevTools, IDEs, analysis server, completions, refactorings, ...). label Mar 10, 2025
@bwilkerson bwilkerson added devexp-completion Issues with the analysis server's code completion feature P2 A bug or feature request we're likely to work on devexp-completion-correctness Issues with the correctness of the analysis server's code completion feature labels Mar 10, 2025
@scheglov
Copy link
Contributor Author

scheglov commented Mar 10, 2025

Might be related to the fact that lastFragment is a type parameter.

class FragmentedElementBuilder<E extends Element2, F extends Fragment> {
  final E element;
  final F firstFragment;
  F lastFragment;

More code

class ClassElementBuilder
    extends InstanceElementBuilder<ClassElementImpl2, ClassElementImpl> {
  ClassElementBuilder({
    required super.element,
    required super.firstFragment,
  });

  void addFragment(ClassElementImpl fragment) {
    addFields(fragment.fields);
    addConstructors(fragment.constructors);
    addAccessors(fragment.accessors);
    addMethods(fragment.methods);

    if (identical(fragment, firstFragment)) {
      _addFirstFragment();
    } else {
      lastFragment.augmentation = fragment; // changes here
      lastFragment = fragment;

      fragment.augmentedInternal = element;
    }
  }
}

@FMorschel
Copy link
Contributor

FMorschel commented Mar 11, 2025

Not quite, but good guess, it does have a role in the problem not being identified sooner.

The problem is that ClassElementImpl is abstract and implements only the getter for nextFragment because the same thing happened at InterfaceElementImpl (abstract and only getter). The latter does extend InstanceElementImpl which has a specific type for nextFragment (which is InstanceElementImpl?) but at this point, the implementation of the setters was not done (it would need the covariant keyword so maybe this is the reason?).

Probably a bug with abstract + implements here.

This would also happen for the first line here but not the second one (may be a getter):

ClassElementImpl().nextFragmen^ = null;
ClassElementImpl().nextFragmen^

Repro:

class A {
  A? myValue;
}

class O extends A {}

abstract class A2 implements A {
  @override
  O? get myValue;
}

void f(A2 t) {
  t.myValu = null;
  t.myValu
}

@FMorschel
Copy link
Contributor

The problem is actually at: DeclarationHelper._addInstanceMembers in the loop for the members, it prefers getters over setters as the bestMember (pkg/analysis_server/lib/src/services/completion/dart/declaration_helper.dart). I'll see if I can fix this.

@FMorschel
Copy link
Contributor

Working on this at https://dart-review.googlesource.com/c/sdk/+/414920

@scheglov
Copy link
Contributor Author

I think these correspondingGetter and correspondingSetter are low-level properties, and probably should stay as is.
But in general the idea of using full interface of the class makes sense to me.
I'm surprised that DAS does not do it already.
IIRC I saw using inheritanceManager there.

@FMorschel
Copy link
Contributor

FMorschel commented Mar 12, 2025

I did that because of:

void _suggestProperty({
required PropertyAccessorElement2 accessor,
bool ignoreVisibility = false,
ImportData? importData,
InterfaceElement2? referencingInterface,
bool isInDeclaration = false,
}) {
if (ignoreVisibility ||
visibilityTracker.isVisible(
element: accessor,
importData: importData,
)) {
if ((mustBeAssignable &&
accessor is GetterElement &&
accessor.correspondingSetter2 == null) ||
mustBeConstant ||
(mustBeNonVoid && accessor.returnType is VoidType)) {
return;
}
var matcherScore = state.matcher.score(accessor.displayName);
if (matcherScore != -1) {
if (accessor.isSynthetic) {
// Avoid visiting a field twice. All fields induce a getter, but only
// non-final fields induce a setter, so we don't add a suggestion for a
// synthetic setter.
if (accessor is GetterElement) {
var variable = accessor.variable3;
if (variable is FieldElement2) {
var suggestion = FieldSuggestion(

If I send in a syntaic setter (which for the above example and the case you found would be), it would not add the suggestion. If I send in the getter without a correspondingSetter2 while the suggestion mustBeAssignable (which, again, both cases would be) then the suggestion is discarded soon.

If you have a better idea I'm all for it.

The method that calls _suggestProperty (snippet above) is (at the stopping line here it has a if-else tree for method, getter and setter) which for getter and setter will call the above:

void _addInstanceMembers({
required InterfaceType type,
required Set<String> excludedGetters,
required bool includeMethods,
required bool includeSetters,
bool onlySuper = false,
}) {
var substitution = Substitution.fromInterfaceType(type);
var map =
onlySuper
? request.inheritanceManager.getInheritedConcreteMap(type.element3)
: request.inheritanceManager.getInterface2(type.element3).map2;
var membersByName = <String, List<ExecutableElement2>>{};
for (var rawMember in map.values) {
if (_canAccessInstanceMember(rawMember)) {
var name = rawMember.displayName;
membersByName
.putIfAbsent(name, () => <ExecutableElement2>[])
.add(rawMember);
}
}
var referencingInterface = _referencingInterfaceFor(type.element3);
for (var entry in membersByName.entries) {
var members = entry.value;
var rawMember = members.bestMember;
if (rawMember is MethodElement2) {

Image

And the bestMember prefers a getter over a setter because of the first snippet's comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-devexp Developer experience items (DevTools, IDEs, analysis server, completions, refactorings, ...). devexp-completion Issues with the analysis server's code completion feature devexp-completion-correctness Issues with the correctness of the analysis server's code completion feature P2 A bug or feature request we're likely to work on
Projects
None yet
Development

No branches or pull requests

3 participants