Gate @RelayResolver usage behind allow_legacy_relay_resolver_tag in relay-schema-generation

Reviewed By: evanyeung

Differential Revision: D94138342

fbshipit-source-id: 38f196089c191e1e587d0b69b00c988c2d0f5007

Author: captbaritone

compiler/crates/docblock-shared/src/lib.rs

@@ -15,6 +15,30 @@ use intern::string_key::StringKey;
 use lazy_static::lazy_static;
 pub use resolver_source_hash::ResolverSourceHash;
 
+/// Check if a string contains any resolver docblock tag
+/// (`@RelayResolver`, `@relayType`, or `@relayField`).
+///
+/// This performs a single pass over the string, avoiding the cost of
+/// multiple `str::contains` calls which would each iterate the full text.
+pub fn contains_resolver_tag(text: &str) -> bool {
+    let bytes = text.as_bytes();
+    let len = bytes.len();
+    let mut i = 0;
+    while i < len {
+        if bytes[i] == b'@' {
+            let remaining = &text[i + 1..];
+            if remaining.starts_with("RelayResolver")
+                || remaining.starts_with("relayType")
+                || remaining.starts_with("relayField")
+            {
+                return true;
+            }
+        }
+        i += 1;
+    }
+    false
+}
+
 lazy_static! {
     /// Resolver fields and types get their schema definitions annotated with
     /// a directive using this name to signal to the rest of Relay that they are backed by

compiler/crates/extract-graphql/src/lib.rs

@@ -116,7 +116,7 @@ fn contains_resolver_tag(text: &str) -> bool {
     false
 }
 
-/// Extract graphql`text` literals and @RelayResolver comments from JS-like code.
+/// Extract graphql`text` literals and Relay Resolver docblock comments from JS-like code.
 // This should work for Flow or TypeScript alike.
 pub fn extract(input: &str) -> Vec<JavaScriptSourceFeature> {
     let mut res = Vec::new();

compiler/crates/relay-compiler/src/build_project/build_resolvers_schema/extract_docblock_ir.rs

@@ -103,6 +103,7 @@ fn extract_docblock_ir_for_project(
             &project_config.typegen_config.custom_scalar_types,
             compiler_state,
             graphql_asts,
+            &project_config.feature_flags.allow_legacy_relay_resolver_tag,
         )?;
         type_irs.extend(extracted_types);
         field_irs.extend(extracted_fields);

compiler/crates/relay-compiler/src/config.rs

@@ -17,6 +17,7 @@ use std::vec;
 use async_trait::async_trait;
 use common::DiagnosticsResult;
 use common::DirectiveName;
+use common::FeatureFlag;
 use common::FeatureFlags;
 use common::Rollout;
 use common::ScalarName;
@@ -108,6 +109,7 @@ type CustomExtractRelayResolvers = Box<
             &FnvIndexMap<ScalarName, CustomType>,
             &CompilerState,
             Option<&GraphQLAsts>,
+            &FeatureFlag,
         ) -> DiagnosticsResult<(Vec<DocblockIr>, Vec<DocblockIr>)>
         // (Types, Fields)
         + Send

compiler/crates/relay-schema-generation/src/errors.rs

@@ -5,6 +5,10 @@
  * LICENSE file in the root directory of this source tree.
  */
 
+use common::DiagnosticDisplay;
+use common::WithDiagnosticData;
+use docblock_shared::RELAY_FIELD_FIELD;
+use docblock_shared::RELAY_TYPE_FIELD;
 use intern::string_key::StringKey;
 use thiserror::Error;
 
@@ -38,16 +42,16 @@ pub enum SchemaGenerationError {
     ExpectedTypeAliasToBeObject,
     #[error("Expected object definition to include fields")]
     ExpectedWeakObjectToHaveFields,
-    #[error("@RelayResolver annotation is expected to be on a named export")]
+    #[error("Relay Resolver annotation is expected to be on a named export")]
     ExpectedNamedExport,
-    #[error("@RelayResolver annotation is expected to be on a named function or type alias")]
+    #[error("Relay Resolver annotation is expected to be on a named function or type alias")]
     ExpectedFunctionOrTypeAlias,
     #[error(
-        "Types used in @RelayResolver definitions should be imported using named or default imports (without using a `*`)"
+        "Types used in Relay Resolver definitions should be imported using named or default imports (without using a `*`)"
     )]
     UseNamedOrDefaultImport,
     #[error(
-        "Failed to find @RelayResolver type definition for `{entity_name}` using a {export_type} import from module `{module_name}`. Please make sure `{entity_name}` is either defined locally or imported using a named or default import and that it is a resolver type"
+        "Failed to find Relay Resolver type definition for `{entity_name}` using a {export_type} import from module `{module_name}`. Please make sure `{entity_name}` is either defined locally or imported using a named or default import and that it is a resolver type"
     )]
     ModuleNotFound {
         entity_name: StringKey,
@@ -72,7 +76,7 @@ pub enum SchemaGenerationError {
     )]
     IncorrectArgumentsDefinition,
     #[error(
-        "Multiple docblock descriptions found for this @RelayResolver. Please only include one description (a comment in the docblock uninterrupted by a resolver \"@<field>\")"
+        "Multiple docblock descriptions found for this Relay Resolver. Please only include one description (a comment in the docblock uninterrupted by a resolver \"@<field>\")"
     )]
     MultipleDocblockDescriptions,
     #[error(
@@ -107,3 +111,45 @@ pub enum SchemaGenerationError {
     )]
     ExpectedResolverFunctionWithRootFragment,
 }
+
+#[derive(
+    Clone,
+    Debug,
+    Error,
+    Eq,
+    PartialEq,
+    Ord,
+    PartialOrd,
+    Hash,
+    serde::Serialize
+)]
+#[serde(tag = "type")]
+pub enum SchemaGenerationErrorWithData {
+    #[error(
+        "Unexpected `@RelayResolver` for a type definition. Expected `@relayType`. The legacy `@RelayResolver` tag can be enabled with the `allow_legacy_relay_resolver_tag` feature flag."
+    )]
+    UseRelayTypeTag,
+    #[error(
+        "Unexpected `@RelayResolver` for a field definition. Expected `@relayField`. The legacy `@RelayResolver` tag can be enabled with the `allow_legacy_relay_resolver_tag` feature flag."
+    )]
+    UseRelayFieldTag,
+    #[error("Unexpected `@relayType` for a field definition. Expected `@relayField`.")]
+    RelayTypeTagUsedForField,
+    #[error("Unexpected `@relayField` for a type definition. Expected `@relayType`.")]
+    RelayFieldTagUsedForType,
+}
+
+impl WithDiagnosticData for SchemaGenerationErrorWithData {
+    fn get_data(&self) -> Vec<Box<dyn DiagnosticDisplay>> {
+        match self {
+            SchemaGenerationErrorWithData::UseRelayTypeTag => vec![Box::new(*RELAY_TYPE_FIELD)],
+            SchemaGenerationErrorWithData::UseRelayFieldTag => vec![Box::new(*RELAY_FIELD_FIELD)],
+            SchemaGenerationErrorWithData::RelayTypeTagUsedForField => {
+                vec![Box::new(*RELAY_FIELD_FIELD)]
+            }
+            SchemaGenerationErrorWithData::RelayFieldTagUsedForType => {
+                vec![Box::new(*RELAY_TYPE_FIELD)]
+            }
+        }
+    }
+}

compiler/crates/relay-schema-generation/src/lib.rs

@@ -26,17 +26,20 @@ use ::intern::string_key::Intern;
 use ::intern::string_key::StringKey;
 use common::Diagnostic;
 use common::DiagnosticsResult;
+use common::FeatureFlag;
 use common::Location;
 use common::ScalarName;
 use common::SourceLocationKey;
 use common::Span;
 use common::WithLocation;
 use docblock_shared::DEPRECATED_FIELD;
 use docblock_shared::ResolverSourceHash;
+use docblock_shared::contains_resolver_tag;
 use docblock_syntax::DocblockAST;
 use docblock_syntax::DocblockSection;
 use docblock_syntax::parse_docblock_with_offset;
 use errors::SchemaGenerationError;
+use errors::SchemaGenerationErrorWithData;
 use find_resolver_imports::ImportExportVisitor;
 use find_resolver_imports::JSImportType;
 use find_resolver_imports::ModuleResolution;
@@ -138,6 +141,10 @@ pub struct RelayResolverExtractor {
 
     // Used to map Flow types in return/argument types to GraphQL custom scalars
     custom_scalar_map: FnvIndexMap<CustomType, ScalarName>,
+
+    // Feature flag controlling whether the legacy @RelayResolver tag is allowed
+    // in place of @relayType / @relayField
+    allow_legacy_relay_resolver_tag: FeatureFlag,
 }
 
 enum FieldDefinitionInfo {
@@ -152,6 +159,12 @@ enum FieldDefinitionInfo {
     },
 }
 
+enum UsedTag {
+    LegacyResolver,
+    Type,
+    Field,
+}
+
 struct UnresolvedFieldDefinition {
     entity_name: Option<WithLocation<StringKey>>,
     field_name: WithLocation<StringKey>,
@@ -163,21 +176,16 @@ struct UnresolvedFieldDefinition {
     field_info: FieldDefinitionInfo,
 }
 
-impl Default for RelayResolverExtractor {
-    fn default() -> Self {
-        Self::new()
-    }
-}
-
 impl RelayResolverExtractor {
-    pub fn new() -> Self {
+    pub fn new(allow_legacy_relay_resolver_tag: &FeatureFlag) -> Self {
         let mut self_ = Self {
             type_definitions: Default::default(),
             unresolved_field_definitions: Default::default(),
             resolved_field_definitions: vec![],
             module_resolutions: Default::default(),
             current_location: SourceLocationKey::generated(),
             custom_scalar_map: FnvIndexMap::default(),
+            allow_legacy_relay_resolver_tag: allow_legacy_relay_resolver_tag.clone(),
         };
         self_.add_relay_runtime_flow_scalars();
         self_
@@ -209,7 +217,8 @@ impl RelayResolverExtractor {
         source_module_path: &str,
         fragment_definitions: Option<&Vec<ExecutableDefinition>>,
     ) -> DiagnosticsResult<()> {
-        // Assume the caller knows the text contains at least one RelayResolver decorator
+        // Assume the caller knows the text contains at least one resolver docblock tag
+        // (@relayType, @relayField, or the legacy @RelayResolver)
 
         self.current_location = SourceLocationKey::standalone(source_module_path);
 
@@ -262,7 +271,7 @@ impl RelayResolverExtractor {
         let result = try_all(
             attached_comments
                 .into_iter()
-                .filter(|(comment, _, _, _)| comment.contains("@RelayResolver"))
+                .filter(|(comment, _, _, _)| contains_resolver_tag(comment))
                 .map(|(comment, comment_range, node, range)| {
                     // TODO: Handle unwraps
                     // Hermes strips the /* and */ delimiters from the
@@ -274,7 +283,16 @@ impl RelayResolverExtractor {
                         self.current_location,
                         comment_range.start + 2,
                     )?;
-                    let resolver_value = docblock.find_field(intern!("RelayResolver")).unwrap();
+                    let (used_tag, resolver_value) =
+                        if let Some(field) = docblock.find_field(intern!("RelayResolver")) {
+                            (UsedTag::LegacyResolver, field)
+                        } else if let Some(field) = docblock.find_field(intern!("relayType")) {
+                            (UsedTag::Type, field)
+                        } else if let Some(field) = docblock.find_field(intern!("relayField")) {
+                            (UsedTag::Field, field)
+                        } else {
+                            return Ok(());
+                        };
 
                     let deprecated = get_deprecated(&docblock);
                     let description = get_description(&docblock, comment_range)?;
@@ -289,15 +307,50 @@ impl RelayResolverExtractor {
                         }) => {
                             let name = resolver_value.field_value.unwrap_or(field_name);
 
-                            // Heuristic to treat lowercase name as field definition, otherwise object definition
-                            // if there is a `.` in the name, it is the old resolver synatx, e.g. @RelayResolver Client.field,
-                            // we should treat it as a field definition
+                            // Heuristic to treat lowercase name as field definition, otherwise object definition.
+                            // If there is a `.` in the name, it is the old verbose syntax,
+                            // e.g. @relayField Client.field; we should treat it as a field definition
                             let is_field_definition = {
                                 let name_str = name.item.lookup();
                                 let is_lowercase_initial =
                                     name_str.chars().next().unwrap().is_lowercase();
                                 is_lowercase_initial || name_str.contains('.')
                             };
+
+                            match used_tag {
+                                UsedTag::LegacyResolver => {
+                                    if !self
+                                        .allow_legacy_relay_resolver_tag
+                                        .is_enabled_for(name.item)
+                                    {
+                                        return Err(vec![Diagnostic::error_with_data(
+                                            if is_field_definition {
+                                                SchemaGenerationErrorWithData::UseRelayFieldTag
+                                            } else {
+                                                SchemaGenerationErrorWithData::UseRelayTypeTag
+                                            },
+                                            resolver_value.field_name.location,
+                                        )]);
+                                    }
+                                }
+                                UsedTag::Type => {
+                                    if is_field_definition {
+                                        return Err(vec![Diagnostic::error_with_data(
+                                            SchemaGenerationErrorWithData::RelayTypeTagUsedForField,
+                                            resolver_value.field_name.location,
+                                        )]);
+                                    }
+                                }
+                                UsedTag::Field => {
+                                    if !is_field_definition {
+                                        return Err(vec![Diagnostic::error_with_data(
+                                            SchemaGenerationErrorWithData::RelayFieldTagUsedForType,
+                                            resolver_value.field_name.location,
+                                        )]);
+                                    }
+                                }
+                            }
+
                             if is_field_definition {
                                 let entity_name = match entity_type {
                                     Some(entity_type) => {
@@ -340,6 +393,30 @@ impl RelayResolverExtractor {
                             type_alias,
                         }) => {
                             let name = resolver_value.field_value.unwrap_or(field_name);
+
+                            match used_tag {
+                                UsedTag::LegacyResolver => {
+                                    if !self
+                                        .allow_legacy_relay_resolver_tag
+                                        .is_enabled_for(name.item)
+                                    {
+                                        return Err(vec![Diagnostic::error_with_data(
+                                            SchemaGenerationErrorWithData::UseRelayTypeTag,
+                                            resolver_value.field_name.location,
+                                        )]);
+                                    }
+                                }
+                                UsedTag::Field => {
+                                    return Err(vec![Diagnostic::error_with_data(
+                                        SchemaGenerationErrorWithData::RelayFieldTagUsedForType,
+                                        resolver_value.field_name.location,
+                                    )]);
+                                }
+                                UsedTag::Type => {
+                                    // This is the expected tag! No errors to report.
+                                }
+                            }
+
                             let mut prop_visitor = PropertyVisitor::new(
                                 source_module_path,
                                 source_hash,

compiler/crates/relay-schema-generation/tests/docblock.rs

@@ -11,6 +11,7 @@ use std::str::FromStr;
 use std::sync::Arc;
 
 use common::Diagnostic;
+use common::FeatureFlag;
 use common::ScalarName;
 use common::SourceLocationKey;
 use common::TextSource;
@@ -42,7 +43,7 @@ pub async fn transform_fixture(fixture: &Fixture<'_>) -> Result<String, String>
     let project_fixture = ProjectFixture::deserialize(fixture.content);
 
     let custom_scalar_types = get_custom_scalar_types();
-    let mut extractor = RelayResolverExtractor::new();
+    let mut extractor = RelayResolverExtractor::new(&FeatureFlag::Enabled);
     if let Err(err) = extractor.set_custom_scalar_map(&custom_scalar_types) {
         errors.extend(err);
     }

compiler/crates/relay-schema-generation/tests/docblock/fixtures/description.expected

@@ -202,7 +202,7 @@ type Cat @__RelayResolverModel {
 }
 
 
-✖︎ Multiple docblock descriptions found for this @RelayResolver. Please only include one description (a comment in the docblock uninterrupted by a resolver "@<field>")
+✖︎ Multiple docblock descriptions found for this Relay Resolver. Please only include one description (a comment in the docblock uninterrupted by a resolver "@<field>")
 
   module.js:24:1
    22 │ export function name(cat: CatFlowType): ?string {}

compiler/crates/relay-schema-generation/tests/docblock/fixtures/incorrect-export-error.expected

@@ -18,7 +18,7 @@ const MyResolver = {x: number};
 ==================================== OUTPUT ===================================
 
 
-✖︎ @RelayResolver annotation is expected to be on a named export
+✖︎ Relay Resolver annotation is expected to be on a named export
 
   module.js:7:1
     5 │  * @RelayResolver

compiler/crates/relay-schema-generation/tests/docblock/fixtures/incorrect-type-error.expected

@@ -18,7 +18,7 @@ export const MyResolver = string;
 ==================================== OUTPUT ===================================
 
 
-✖︎ @RelayResolver annotation is expected to be on a named function or type alias
+✖︎ Relay Resolver annotation is expected to be on a named function or type alias
 
   module.js:7:1
     5 │  * @RelayResolver