Merge branch 'topic/1384-2' into 'master'

Fix how visibility is computed for completion items.

Closes #1412, #1407, and #1384

See merge request eng/libadalang/libadalang!1690

ada/nodes.lkt

@@ -930,7 +930,7 @@ class AdaNode implements Node[AdaNode] {
                 other_entity.as[GenericPackageInstantiation]?.info.from_rebound
             ) or other_entity.as[PackageRenamingDecl]?.info.from_rebound or (
                 # The node is not an unit root
-                not other_entity.as[BasicDecl].is_compilation_unit_root()
+                not other_entity.as[BasicDecl]?.is_compilation_unit_root()
             ) or (
                 # Else, check with visibility
                 node.has_with_visibility(other_entity.node.unit())
@@ -1422,7 +1422,10 @@ class AdaNode implements Node[AdaNode] {
     fun complete_items(): Array[CompletionItem] =
         node.env_get_public(node.children_env(), null[Symbol]).map(
             (n) => CompletionItem(
-                decl=n.as[BasicDecl], is_dot_call=n.info.md.dottable_subp, is_visible=node.has_with_visibility(n.unit()), weight=self.complete_item_weight(n.as[BasicDecl])
+                decl=n.as[BasicDecl],
+                is_dot_call=n.info.md.dottable_subp,
+                is_visible=node.has_visibility(n),
+                weight=self.complete_item_weight(n.as[BasicDecl])
             )
         )
 
@@ -1519,7 +1522,10 @@ class AlternativesList: ASTList[AdaNode] {
     @with_dynvars(origin)
     fun complete_items(): Array[CompletionItem] = node.children_env().get(null[Symbol]).map(
         (n) => CompletionItem(
-            decl=n.as[BasicDecl], is_dot_call=n.info.md.dottable_subp, is_visible=node.has_with_visibility(n.unit()), weight=match n {
+            decl=n.as[BasicDecl],
+            is_dot_call=n.info.md.dottable_subp,
+            is_visible=node.has_visibility(n),
+            weight=match n {
                 case eld: EnumLiteralDecl => if self.enum_type() == eld.enum_type() then 100 else 0
                 case _ => 0
             }
@@ -14041,42 +14047,52 @@ class DottedName: Name {
     @with_dynvars(origin)
     fun complete_items(): Array[CompletionItem] = {
         bind origin = node.origin_node();
-
-        {
-            bind env = node.node_env();
-
-            {
-                bind no_visibility = true;
-
-                # In completion we always want to return everything, and flag
-                # invisible things as invisible, so we set the "no_visibility" flag
-                # to True.
-                node.env_get_public(
-                    self.prefix.designated_env(), null[Symbol], LookupKind.flat
-                ).filtermap(
-                    (n) => CompletionItem(
-                        decl=n.as[BasicDecl], is_dot_call=n.info.md.dottable_subp, is_visible=(
-                            # Dottable subprograms are always visible
-                            n.info.md.dottable_subp
-                        ) or (
-                            # Else check visibility on the unit containing n
-                            node.has_with_visibility(n.unit())
-                        ), weight=self.complete_item_weight(n.as[BasicDecl])
-                    ), (n) => (
-                        # Filter elements that are coming from a body that is not
-                        # visible. This can happen with dottable subprograms
-                        # defined in bodies.
-                        # NOTE: We also filter `PrivatePart`s here as they are
-                        # useless from the completion point of view.
-                        # Order matters here, `has_visibility` below should not
-                        # be called with n being a PrivatePart.
-                        not n is PrivatePart
-                    ) and (
-                        n.owning_unit_kind() == AnalysisUnitKind.unit_specification or node.has_visibility(n)
-                    )
-                )
-            }
-        }
+        bind env = node.node_env();
+        val complete_env = {
+            bind no_visibility = true;
+            self.prefix.designated_env()
+        };
+        val visible_env = {
+            bind no_visibility = false;
+            self.prefix.designated_env()
+        };
+        # In completion we always want to return everything, and flag invisible
+        # things as invisible, so we first query `complete_env` to discover all
+        # possible items, and then check whether they are actually visible by
+        # querying `visible_env`.
+        node.env_get_public(
+            complete_env, null[Symbol], LookupKind.flat
+        ).filtermap(
+            (n) => CompletionItem(
+                decl=n.as[BasicDecl],
+                is_dot_call=n.info.md.dottable_subp,
+                is_visible=(
+                    # Dottable subprograms are always visible
+                    n.info.md.dottable_subp
+                ) or (
+                    # Else check visibility on the unit containing n
+                    node.has_visibility(n)
+                    and node.env_get_public(
+                        visible_env,
+                        n.as[BasicDecl].name_symbol,
+                        LookupKind.flat
+                    ).contains(n)
+                ),
+                weight=self.complete_item_weight(n.as[BasicDecl])
+            ), (n) => (
+                # Filter elements that are coming from a body that is not
+                # visible. This can happen with dottable subprograms
+                # defined in bodies.
+                # NOTE: We also filter `PrivatePart`s here as they are
+                # useless from the completion point of view.
+                # Order matters here, `has_visibility` below should not
+                # be called with n being a PrivatePart.
+                not n is PrivatePart
+            ) and (
+                n.owning_unit_kind() == AnalysisUnitKind.unit_specification
+                or node.has_visibility(n)
+            )
+        )
     }
 
     @with_dynvars(origin)
@@ -17652,7 +17668,10 @@ class SubtypeIndication: TypeExpr {
     @with_dynvars(origin)
     fun complete_items(): Array[CompletionItem] = self.children_env().get(null[Symbol]).map(
         (n) => CompletionItem(
-            decl=n.as[BasicDecl], is_dot_call=n.info.md.dottable_subp, is_visible=node.has_with_visibility(n.unit()), weight=match n {
+            decl=n.as[BasicDecl],
+            is_dot_call=n.info.md.dottable_subp,
+            is_visible=node.has_visibility(n),
+            weight=match n {
                 case btd: BaseTypeDecl => # Do not promote self as a possible completion for
                 # itself::
                 #

extensions/src/libadalang-env_hooks.adb

@@ -455,14 +455,6 @@ package body Libadalang.Env_Hooks is
 
             Prepare_Nameres (Unit, PLE_Root_Index);
 
-            --  We're on the last portion of the name: return
-
-            if Is_Last then
-               return;
-            end if;
-
-            --  Else, recurse
-
             declare
                Internal_Name : Symbol_Type_Array_Access :=
                  Create_Symbol_Type_Array (Internal_Symbol_Type_Array (Name));
@@ -491,7 +483,7 @@ package body Libadalang.Env_Hooks is
                        Basic_Decl_P_Fully_Qualified_Name_Array
                          (Unwrap_Node (Target));
                      New_Index     : constant Positive :=
-                       Resolved_Name.Items'Last + 1;
+                       Resolved_Name.Items'Last;
                   begin
                      --  .. and make the next call to step consider the renamed
                      --  package.
@@ -500,15 +492,31 @@ package body Libadalang.Env_Hooks is
                        (Name  => Symbol_Type_Array (Resolved_Name.Items)
                                  & Name (Index + 1 .. Name'Last),
                         Index => New_Index);
+
+                     --  However if that was the last part of the name, we
+                     --  still want to return the renaming unit, and not
+                     --  renamed one. In theory we could have returned before
+                     --  resolving the renaming because we already had the unit
+                     --  we wanted to return, but if we did that we would not
+                     --  have updated the `Referenced_Units` vector to include
+                     --  the fact that `From_Unit` references the renamed unit.
+                     if Is_Last then
+                        Unit := Unwrap_Unit (Comp_Unit.Unit);
+                     end if;
+
                      Free (Resolved_Name);
                   exception
                      when Precondition_Failure | Property_Error =>
                         Free (Resolved_Name);
                         raise;
                   end;
                else
-                  --  Else, just resolve the next portion of the given name
+                  --  We're on the last portion of the name: return
+                  if Is_Last then
+                     return;
+                  end if;
 
+                  --  Else, just resolve the next portion of the given name
                   Step (Name, Index + 1);
                end if;
 

testsuite/tests/name_resolution/with_clause_pkg_renaming/main.adb

@@ -3,5 +3,13 @@ pragma Test_Statement;
 
 procedure Main is
 begin
-   null;
+   Renaming.Foo;
+   pragma Test_Statement;
+   Test.Foo;
+   pragma Test_Statement;
+
+   Renaming.Child.Bar;
+   pragma Test_Statement;
+   Test.Child.Bar;
+   pragma Test_Statement;
 end Main;

testsuite/tests/name_resolution/with_clause_pkg_renaming/test-child.ads

@@ -1,2 +1,3 @@
 package Test.Child is
+   procedure Bar is null;
 end Test.Child;

testsuite/tests/name_resolution/with_clause_pkg_renaming/test.ads

@@ -1,2 +1,3 @@
 package Test is
+   procedure Foo is null;
 end Test;

testsuite/tests/name_resolution/with_clause_pkg_renaming/test.out

@@ -21,5 +21,85 @@ Expr: <Id "Child" main.adb:1:25-1:30>
   type:          None
   expected type: None
 
+Resolving xrefs for node <CallStmt main.adb:6:4-6:17>
+*****************************************************
+
+Expr: <DottedName main.adb:6:4-6:16>
+  references:    <DefiningName "Foo" test.ads:2:14-2:17>
+  type:          None
+  expected type: None
+Expr: <Id "Renaming" main.adb:6:4-6:12>
+  references:    <DefiningName "Renaming" renaming.ads:3:9-3:17>
+  type:          None
+  expected type: None
+Expr: <Id "Foo" main.adb:6:13-6:16>
+  references:    <DefiningName "Foo" test.ads:2:14-2:17>
+  type:          None
+  expected type: None
+
+Resolving xrefs for node <CallStmt main.adb:8:4-8:13>
+*****************************************************
+
+Expr: <DottedName main.adb:8:4-8:12>
+  references:    <DefiningName "Foo" test.ads:2:14-2:17>
+  type:          None
+  expected type: None
+Expr: <Id "Test" main.adb:8:4-8:8>
+  references:    <DefiningName "Test" test.ads:1:9-1:13>
+  type:          None
+  expected type: None
+Expr: <Id "Foo" main.adb:8:9-8:12>
+  references:    <DefiningName "Foo" test.ads:2:14-2:17>
+  type:          None
+  expected type: None
+
+Resolving xrefs for node <CallStmt main.adb:11:4-11:23>
+*******************************************************
+
+Expr: <DottedName main.adb:11:4-11:22>
+  references:    <DefiningName "Bar" test-child.ads:2:14-2:17>
+  type:          None
+  expected type: None
+Expr: <DottedName main.adb:11:4-11:18>
+  references:    <DefiningName "Test.Child" test-child.ads:1:9-1:19>
+  type:          None
+  expected type: None
+Expr: <Id "Renaming" main.adb:11:4-11:12>
+  references:    <DefiningName "Renaming" renaming.ads:3:9-3:17>
+  type:          None
+  expected type: None
+Expr: <Id "Child" main.adb:11:13-11:18>
+  references:    <DefiningName "Test.Child" test-child.ads:1:9-1:19>
+  type:          None
+  expected type: None
+Expr: <Id "Bar" main.adb:11:19-11:22>
+  references:    <DefiningName "Bar" test-child.ads:2:14-2:17>
+  type:          None
+  expected type: None
+
+Resolving xrefs for node <CallStmt main.adb:13:4-13:19>
+*******************************************************
+
+Expr: <DottedName main.adb:13:4-13:18>
+  references:    <DefiningName "Bar" test-child.ads:2:14-2:17>
+  type:          None
+  expected type: None
+Expr: <DottedName main.adb:13:4-13:14>
+  references:    <DefiningName "Test.Child" test-child.ads:1:9-1:19>
+  type:          None
+  expected type: None
+Expr: <Id "Test" main.adb:13:4-13:8>
+  references:    <DefiningName "Test" test.ads:1:9-1:13>
+  type:          None
+  expected type: None
+Expr: <Id "Child" main.adb:13:9-13:14>
+  references:    <DefiningName "Test.Child" test-child.ads:1:9-1:19>
+  type:          None
+  expected type: None
+Expr: <Id "Bar" main.adb:13:15-13:18>
+  references:    <DefiningName "Bar" test-child.ads:2:14-2:17>
+  type:          None
+  expected type: None
+
 
 Done.

testsuite/tests/properties/complete/test.out

@@ -485,7 +485,7 @@ Working on node <CallStmt private.adb:16:4-18:1>
 ================================================
 
 Eval 'list(node.f_call.p_complete)'
-Result: [<CompletionItem decl=<PackageBody ["Q"] private.adb:8:4-13:10> is_dot_call=False is_visible=True weight=0>,
+Result: [<CompletionItem decl=<PackageBody ["Q"] private.adb:8:4-13:10> is_dot_call=False is_visible=False weight=0>,
          <CompletionItem decl=<SubpDecl ["P"] private.adb:3:7-3:33> is_dot_call=False is_visible=True weight=75>]
 
 Working on node <ObjectDecl ["A"] subtype_indication.adb:2:4-2:10>

testsuite/tests/properties/complete_renamed_pkgs/pkg-child.ads

@@ -0,0 +1,3 @@
+package Pkg.Child is
+   procedure Baz is null;
+end Pkg.Child;

testsuite/tests/properties/complete_renamed_pkgs/pkg.adb

@@ -0,0 +1,5 @@
+package body Pkg is
+   package body Gen is
+      procedure Bar is null;
+   end Gen;
+end Pkg;

testsuite/tests/properties/complete_renamed_pkgs/pkg.ads

@@ -0,0 +1,8 @@
+package Pkg is
+   procedure Foo is null;
+
+   generic
+   package Gen is
+      procedure Bar;
+   end Gen;
+end Pkg;

testsuite/tests/properties/complete_renamed_pkgs/renamed.ads

@@ -0,0 +1,3 @@
+with Pkg;
+
+package Renamed renames Pkg;

testsuite/tests/properties/complete_renamed_pkgs/renamed_2.ads

@@ -0,0 +1,5 @@
+with Pkg;
+with Pkg.Child;
+
+package Renamed_2 renames Pkg;
+

testsuite/tests/properties/complete_renamed_pkgs/renamed_3.ads

@@ -0,0 +1,6 @@
+with Pkg;
+with Pkg.Child;
+
+package Renamed_3 is
+   package P renames Pkg;
+end Renamed_3;