Improve inline fragment suggestions for abstract types (#4453)

Summary:
Previously the LSP would suggest the abstract type itself (why?) and either the members of a union or the object types implementing an interface when writing an inline fragment for an abstract type.

I've removed the suggestion of the abstract type itself and am instead including all the interfaces that are implemented by the object types that are primarily suggested. This is great for patterns like the Stage 6a mutation error pattern, where you have a union where each member implicitly implements a shared interface. With this change this shared interface would be suggested by the LSP as well, instead of just the concrete "errors".

Pull Request resolved: #4453

Reviewed By: captbaritone

Differential Revision: D49956467

Pulled By: alunyov

fbshipit-source-id: db4581532f2dd74d1262eb8f9a0c4952480ccee8

compiler/crates/relay-lsp/src/completion/mod.rs

@@ -6,7 +6,6 @@
  */
 
 //! Utilities for providing the completion language feature
-use std::iter::once;
 
 use common::ArgumentName;
 use common::DirectiveName;
@@ -51,6 +50,8 @@ use lsp_types::MarkupKind;
 use schema::Argument as SchemaArgument;
 use schema::Directive as SchemaDirective;
 use schema::InputObject;
+use schema::InterfaceID;
+use schema::ObjectID;
 use schema::SDLSchema;
 use schema::Schema;
 use schema::Type;
@@ -671,7 +672,6 @@ fn completion_items_for_request(
     program: &Program,
 ) -> Option<Vec<CompletionItem>> {
     let kind = request.kind;
-    debug!("completion_items_for_request: {:?}", kind);
     match kind {
         CompletionKind::FragmentSpread => {
             let leaf_type = request.type_path.resolve_leaf_type(schema)?;
@@ -971,25 +971,13 @@ fn resolve_completion_items_for_inline_fragment(
     match type_ {
         Type::Interface(id) => {
             let interface = schema.interface(id);
-            once(type_)
-                .chain(
-                    interface
-                        .implementing_objects
-                        .iter()
-                        .filter_map(|id| schema.get_type(schema.object(*id).name.item.0)),
-                )
-                .collect()
+
+            get_abstract_type_suggestions(schema, &interface.implementing_objects, Some(&id))
         }
         Type::Union(id) => {
             let union = schema.union(id);
-            once(type_)
-                .chain(
-                    union
-                        .members
-                        .iter()
-                        .filter_map(|id| schema.get_type(schema.object(*id).name.item.0)),
-                )
-                .collect()
+
+            get_abstract_type_suggestions(schema, &union.members, None)
         }
         Type::Enum(_) | Type::Object(_) | Type::InputObject(_) | Type::Scalar(_) => vec![],
     }
@@ -1404,5 +1392,44 @@ fn make_markdown_table_documentation(
     })
 }
 
+fn get_abstract_type_suggestions(
+    schema: &SDLSchema,
+    objects: &[ObjectID],
+    base_interface_id: Option<&InterfaceID>,
+) -> Vec<Type> {
+    let object_types: Vec<_> = objects.iter().map(|id| schema.object(*id)).collect();
+
+    let mut interfaces = Vec::new();
+    let mut types = Vec::new();
+
+    for object_type in &object_types {
+        if let Some(t) = schema.get_type(object_type.name.item.0) {
+            types.push(t);
+        }
+
+        for interface_id in &object_type.interfaces {
+            let interface_type = schema.interface(*interface_id);
+
+            if let Some(base_id) = base_interface_id {
+                if interface_id == base_id || !interface_type.interfaces.contains(base_id) {
+                    continue;
+                }
+            }
+
+            if let Some(t) = schema.get_type(interface_type.name.item.0) {
+                if interfaces.contains(&t) {
+                    continue;
+                }
+
+                interfaces.push(t);
+            }
+        }
+    }
+
+    types.extend(interfaces);
+
+    types
+}
+
 #[cfg(test)]
 mod test;

compiler/crates/relay-lsp/src/completion/test.rs

@@ -244,7 +244,6 @@ fn whitespace_in_interface() {
             "source",
             "node",
             "__typename",
-            "... on CommentsEdgeInterface",
             "... on CommentsEdge",
             "...ImplementingFragment",
             "...InterfaceFragment",
@@ -280,11 +279,10 @@ fn whitespace_in_union() {
         items.unwrap(),
         vec![
             "__typename",
-            "... on CommentBody",
-            "... on PlainCommentBody",
             "... on MarkdownCommentBody",
-            "...UnionFragment",
+            "... on PlainCommentBody",
             "...UnionVariantFragment",
+            "...UnionFragment",
         ],
     )
 }
@@ -312,9 +310,28 @@ fn inline_fragment_on_interface() {
         "#,
         None,
     );
+    assert_labels(items.unwrap(), vec!["... on SimpleNamed", "... on User"]);
+}
+
+#[test]
+fn inline_fragment_on_interface_objects_implement_interface_implementing_base_interface() {
+    let items = parse_and_resolve_completion_items(
+        r#"
+            fragment Test on UserNameRenderable {
+                ... on |a
+            }
+        "#,
+        None,
+    );
+
     assert_labels(
         items.unwrap(),
-        vec!["... on Named", "... on User", "... on SimpleNamed"],
+        vec![
+            "... on PlainUserNameRenderer",
+            "... on ImplementsImplementsUserNameRenderableAndUserNameRenderable",
+            "... on MarkdownUserNameRenderer",
+            "... on ImplementsUserNameRenderable",
+        ],
     );
 }
 
@@ -328,7 +345,7 @@ fn inline_fragment_on_interface_with_existing_inline_fragment() {
         "#,
         None,
     );
-    assert_labels(items.unwrap(), vec!["Named", "User", "SimpleNamed"]);
+    assert_labels(items.unwrap(), vec!["User", "SimpleNamed"]);
 }
 
 #[test]
@@ -344,10 +361,12 @@ fn inline_fragment_on_union() {
     assert_labels(
         items.unwrap(),
         vec![
-            "... on MaybeNode",
-            "... on Story",
             "... on FakeNode",
+            "... on FeedUnit",
+            "... on Node",
             "... on NonNode",
+            "... on Story",
+            "... on MaybeNodeInterface",
         ],
     );
 }
@@ -364,7 +383,14 @@ fn inline_fragment_on_union_with_existing_inline_fragment() {
     );
     assert_labels(
         items.unwrap(),
-        vec!["MaybeNode", "Story", "FakeNode", "NonNode"],
+        vec![
+            "Node",
+            "Story",
+            "FakeNode",
+            "NonNode",
+            "MaybeNodeInterface",
+            "FeedUnit",
+        ],
     );
 }