[DAS] Fixes completion not showing abstract class with alias declared above it

Fixes: #61244
Change-Id: I0c569653ee9040d8ff0d22135d9be09e7ba4d58d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/444701
Auto-Submit: Felipe Morschel <[email protected]>
Commit-Queue: Brian Wilkerson <[email protected]>
Reviewed-by: Samuel Rawlins <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>

Author: FMorschel

pkg/analysis_server/lib/src/services/completion/dart/declaration_helper.dart

@@ -742,6 +742,12 @@ class DeclarationHelper {
       isNotImported: false,
     );
     for (var element in namespace.definedNames2.values) {
+      if (!visibilityTracker.isVisible(
+        element: element,
+        importData: importData,
+      )) {
+        continue;
+      }
       switch (element) {
         case ClassElement():
           _suggestConstructors(
@@ -947,9 +953,6 @@ class DeclarationHelper {
   /// Adds suggestions for any constructors that are imported into the
   /// [library].
   void _addImportedConstructors(LibraryElement library) {
-    // TODO(brianwilkerson): This will create suggestions for elements that
-    //  conflict with different elements imported from a different library. Not
-    //  sure whether that's the desired behavior.
     for (var importElement in library.firstFragment.libraryImports) {
       var importedLibrary = importElement.importedLibrary;
       if (importedLibrary != null) {
@@ -1793,7 +1796,6 @@ class DeclarationHelper {
         element.constructors,
         importData,
         allowNonFactory: !element.isAbstract,
-        checkVisibility: false,
       );
     }
   }
@@ -1804,7 +1806,6 @@ class DeclarationHelper {
     required ImportData? importData,
     required bool hasClassName,
     required bool isConstructorRedirect,
-    bool checkVisibility = true,
   }) {
     if (mustBeAssignable) {
       return;
@@ -1813,29 +1814,20 @@ class DeclarationHelper {
     if (!element.isVisibleIn(request.libraryElement)) {
       return;
     }
-    if (importData?.isNotImported ?? false) {
-      if (checkVisibility &&
-          !visibilityTracker.isVisible(
-            element: element.enclosingElement,
-            importData: importData,
-          )) {
-        // If the constructor is on a class from a not-yet-imported library and
-        // the class isn't visible, then we shouldn't suggest it.
-        //
-        // We could consider computing a prefix and updating the [importData] in
-        // order to avoid the collision, but we don't currently do that for any
-        // not-yet-imported elements (nor for imported elements that are
-        // shadowed by local declarations).
-        return;
-      }
-    } else {
-      // Add the class to the visibility tracker so that we will know later that
-      // any non-imported elements with the same name are not visible.
-      visibilityTracker.isVisible(
-        element: element.enclosingElement,
-        importData: importData,
-      );
-    }
+    // If the constructor is on a class from a not-yet-imported library and
+    // the class isn't visible, then we shouldn't suggest it.
+    //
+    // We could consider computing a prefix and updating the [importData] in
+    // order to avoid the collision, but we don't currently do that for any
+    // not-yet-imported elements (nor for imported elements that are
+    // shadowed by local declarations).
+
+    // Add the class to the visibility tracker so that we will know later that
+    // any non-imported elements with the same name are not visible.
+    visibilityTracker.isVisible(
+      element: element.enclosingElement,
+      importData: importData,
+    );
 
     // Use the constructor element's name without the interface type to
     // calculate the matcher score for dot shorthands.
@@ -1868,23 +1860,10 @@ class DeclarationHelper {
     List<ConstructorElement> constructors,
     ImportData? importData, {
     bool allowNonFactory = true,
-    bool checkVisibility = true,
   }) {
     if (mustBeAssignable) {
       return;
     }
-    if (checkVisibility &&
-        constructors.isNotEmpty &&
-        !visibilityTracker.isVisible(
-          element: constructors.first.enclosingElement,
-          importData: importData,
-        )) {
-      return;
-    }
-
-    if (checkVisibility) {
-      checkVisibility = false;
-    }
 
     for (var constructor in constructors) {
       if (constructor.isVisibleIn(request.libraryElement) &&
@@ -1894,7 +1873,6 @@ class DeclarationHelper {
           hasClassName: false,
           importData: importData,
           isConstructorRedirect: false,
-          checkVisibility: checkVisibility,
         );
       }
     }
@@ -1972,11 +1950,7 @@ class DeclarationHelper {
       }
       if (!mustBeType) {
         _suggestStaticFields(element.fields, importData);
-        _suggestConstructors(
-          element.constructors,
-          importData,
-          checkVisibility: false,
-        );
+        _suggestConstructors(element.constructors, importData);
       }
     }
   }

pkg/analysis_server/lib/src/services/completion/dart/visibility_tracker.dart

@@ -21,9 +21,12 @@ class VisibilityTracker {
   /// that has the same name.
   ///
   /// If the [importData] indicated that the element is already imported (either
-  /// by being `null` or by returning `false` from `isNotImported`) and the name
-  /// is visible, it will be added to the list of [_declaredNames] so that it
-  /// will shadow any elements of the same name further up the scope chain.
+  /// by being `null` or by returning `false` from `isNotImported`), the name
+  /// is visible.
+  ///
+  /// If it is already imported, it will be added to the list of
+  /// [_declaredNames] so that it will shadow any elements of the same name
+  /// further up the scope chain.
   bool isVisible({required Element? element, required ImportData? importData}) {
     var name = element?.displayName;
     if (name == null) {

pkg/analysis_server/test/lsp/completion_dart_test.dart

@@ -116,6 +116,25 @@ class CompletionDocumentationResolutionTest extends AbstractCompletionTest {
     await initialAnalysis;
   }
 
+  Future<void> test_abstract_class() async {
+    newFile(join(projectFolderPath, 'my_class.dart'), '''
+typedef MyClass2 = MyClass;
+
+abstract class MyClass {}
+''');
+
+    content = '''
+void f() {
+  MyClas^
+}
+''';
+
+    await initializeServer();
+
+    var completion = await getCompletionItem('MyClass');
+    expectDocumentation(completion, isNull);
+  }
+
   Future<void> test_class() async {
     newFile(join(projectFolderPath, 'my_class.dart'), '''
 /// Class.

pkg/analysis_server/test/services/completion/dart/completion_test.dart

@@ -6926,6 +6926,8 @@ class File {
 }
 f() => new Fil^
 ''');
+    // The second `File` constructor that was being hidden previously is from
+    // dart:io
     assertResponse(r'''
 replacement
   left: 3
@@ -6940,6 +6942,8 @@ suggestions
     kind: constructorInvocation
   FileMode._internal1
     kind: constructorInvocation
+  File
+    kind: constructorInvocation
 ''');
   }