Emit disallow_required_on_non_null_field as compiler warnings

Reviewed By: captbaritone Differential Revision: D69548326 fbshipit-source-id: 1b30c2406218b1bcb85d98dfeb727328e0c6d8e2
facebook · Feb 18, 2025 · 207b6d0 · 207b6d0
1 parent caf95e9
commit 207b6d0
Show file tree

Hide file tree

Showing 16 changed files with 69 additions and 65 deletions.
diff --git a/compiler/crates/relay-codemod/src/codemod.rs b/compiler/crates/relay-codemod/src/codemod.rs
@@ -55,9 +55,9 @@ pub async fn run_codemod(
                 }
             }
             AvailableCodemod::RemoveUnnecessaryRequiredDirectives => {
-                match disallow_required_on_non_null_field(&programs.source) {
-                    Ok(e) => e,
-                    Err(e) => panic!("{:?}", e),
+                match disallow_required_on_non_null_field(&programs.reader) {
+                    Ok(_) => vec![],
+                    Err(e) => e,
                 }
             }
         })

diff --git a/compiler/crates/relay-compiler/src/build_project/validate.rs b/compiler/crates/relay-compiler/src/build_project/validate.rs
@@ -15,6 +15,7 @@ use graphql_ir::Program;
 use relay_config::ProjectConfig;
 use relay_transforms::disallow_circular_no_inline_fragments;
 use relay_transforms::disallow_readtime_features_in_mutations;
+use relay_transforms::disallow_required_on_non_null_field;
 use relay_transforms::disallow_reserved_aliases;
 use relay_transforms::disallow_typename_on_root;
 use relay_transforms::validate_assignable_directive;
@@ -46,11 +47,16 @@ pub fn validate_reader(
     project_config: &ProjectConfig,
     additional_validations: &Option<AdditionalValidations>,
 ) -> DiagnosticsResult<WithDiagnostics<()>> {
-    let output = try_all(vec![if let Some(ref validate) = additional_validations {
-        validate(program, project_config)
-    } else {
-        Ok(())
-    }]);
+    let output = try_all(vec![
+        // This validation is in this list because it depends upon
+        // metadata added by the required_directive transform.
+        disallow_required_on_non_null_field(program),
+        if let Some(ref validate) = additional_validations {
+            validate(program, project_config)
+        } else {
+            Ok(())
+        },
+    ]);
 
     transform_errors(output, project_config)
 }

diff --git a/compiler/crates/relay-compiler/src/status_reporter.rs b/compiler/crates/relay-compiler/src/status_reporter.rs
@@ -78,6 +78,9 @@ impl ConsoleStatusReporter {
         match severity {
             DiagnosticSeverity::ERROR => error!("{}", output),
             DiagnosticSeverity::WARNING => warn!("{}", output),
+            DiagnosticSeverity::HINT => {
+                // Opting to omit, not emit, hints in the CLI output.
+            }
             _ => info!("{}", output),
         }
     }

diff --git a/compiler/crates/relay-lsp/src/lib.rs b/compiler/crates/relay-lsp/src/lib.rs
@@ -126,11 +126,11 @@ pub fn diagnostics_to_code_actions(
     let mut all_actions = vec![];
     for param in published_params {
         for diagnostic in param.diagnostics {
-            if let Some(action) = get_code_actions_from_diagnostic(&param.uri, diagnostic)
-                .unwrap()
-                .first()
-            {
-                all_actions.push(action.clone());
+            if let Some(actions) = get_code_actions_from_diagnostic(&param.uri, diagnostic) {
+                // If there are multiple actions for a diagnostic, we only send the first one.
+                if let Some(action) = actions.first() {
+                    all_actions.push(action.clone());
+                }
             }
         }
     }

diff --git a/compiler/crates/relay-lsp/src/server/lsp_state.rs b/compiler/crates/relay-lsp/src/server/lsp_state.rs
@@ -273,19 +273,11 @@ impl<TPerfLogger: PerfLogger + 'static, TSchemaDocumentation: SchemaDocumentatio
                         let mut warnings = vec![];
                         for document in documents {
                             // Today the only warnings we check for are deprecated
-                            // fields and unnecessary @requireds, but in the future
-                            // we could check for more things here by making this
-                            // more generic.
+                            // fields, but in the future we could check for more
+                            // things here by making this more generic.
                             warnings.extend(deprecated_fields_for_executable_definition(
                                 &schema, &document,
                             )?);
-
-                            // Disabling this warning for now, to see if it resolves LSP crashing
-                            // warnings.extend(
-                            //     disallow_required_on_non_null_field_for_executable_definition(
-                            //         &schema, &document,
-                            //     )?,
-                            // );
                         }
                         Ok(warnings)
                     };

diff --git a/compiler/crates/relay-lsp/src/server/lsp_state_resources.rs b/compiler/crates/relay-lsp/src/server/lsp_state_resources.rs
@@ -17,6 +17,7 @@ use graphql_watchman::WatchmanFileSourceSubscriptionNextChange;
 use log::debug;
 use rayon::iter::ParallelIterator;
 use relay_compiler::build_project::get_project_asts;
+use relay_compiler::build_project::validate_reader_program;
 use relay_compiler::build_project::BuildMode;
 use relay_compiler::build_project::ProjectAstData;
 use relay_compiler::build_project::ProjectAsts;
@@ -538,7 +539,7 @@ impl<TPerfLogger: PerfLogger + 'static, TSchemaDocumentation: SchemaDocumentatio
             })
         })?;
 
-        transform_program(
+        let transformed_programs = transform_program(
             project_config,
             Arc::new(base_program),
             Arc::new(base_fragment_names),
@@ -556,6 +557,28 @@ impl<TPerfLogger: PerfLogger + 'static, TSchemaDocumentation: SchemaDocumentatio
                 project_name: project_config.name,
             })
         })?;
+
+        match validate_reader_program(
+            &self.lsp_state.config,
+            project_config,
+            &transformed_programs.reader,
+            log_event,
+        ) {
+            // Non-blocking validation errors
+            Ok(diagnostics) => Err(BuildProjectFailure::Error(
+                BuildProjectError::ValidationErrors {
+                    errors: diagnostics,
+                    project_name: project_config.name,
+                },
+            )),
+            // Compilation-blocking validation errors
+            Err(diagnostics) => Err(BuildProjectFailure::Error(
+                BuildProjectError::ValidationErrors {
+                    errors: diagnostics,
+                    project_name: project_config.name,
+                },
+            )),
+        }?;
         Ok(())
     }
 

diff --git a/compiler/crates/relay-transforms/src/validations.rs b/compiler/crates/relay-transforms/src/validations.rs
@@ -35,7 +35,6 @@ pub use disallow_circular_no_inline_fragments::disallow_circular_no_inline_fragm
 pub use disallow_non_node_id_fields::disallow_non_node_id_fields;
 pub use disallow_readtime_features_in_mutations::disallow_readtime_features_in_mutations;
 pub use disallow_required_on_non_null_field::disallow_required_on_non_null_field;
-pub use disallow_required_on_non_null_field::disallow_required_on_non_null_field_for_executable_definition;
 pub use disallow_reserved_aliases::disallow_reserved_aliases;
 pub use disallow_typename_on_root::disallow_typename_on_root;
 pub use validate_client_schema_extensions_use_catch::validate_client_schema_extensions_use_catch;

diff --git a/compiler/crates/relay-transforms/src/validations/disallow_required_on_non_null_field.rs b/compiler/crates/relay-transforms/src/validations/disallow_required_on_non_null_field.rs
@@ -15,7 +15,6 @@ use common::DirectiveName;
 use common::NamedItem;
 use errors::try2;
 use graphql_ir::reexport::Intern;
-use graphql_ir::ExecutableDefinition;
 use graphql_ir::Field;
 use graphql_ir::FragmentDefinition;
 use graphql_ir::Program;
@@ -25,8 +24,6 @@ use lazy_static::lazy_static;
 use schema::SDLSchema;
 use schema::Schema;
 
-use crate::fragment_alias_directive;
-use crate::required_directive;
 use crate::ValidationMessageWithData;
 use crate::CATCH_DIRECTIVE_NAME;
 use crate::CHILDREN_CAN_BUBBLE_METADATA_KEY;
@@ -39,32 +36,15 @@ lazy_static! {
         DirectiveName("throwOnFieldError".intern());
 }
 
-// NOTE: This validation expects to run on UNTRANSFORMED IR. To the extent that
-// it relies on other transforms, it runs them itself.
-//
-// If you start running this validation on transformed IR, you will want/need to
-// refactor the places that run this transform to pre-apply all required
-// transforms.
-pub fn disallow_required_on_non_null_field(
-    program: &Program,
-) -> DiagnosticsResult<Vec<Diagnostic>> {
-    // This validation depends on metadata directives added by the
-    // required_directive transform. This validation is run on untransformed
-    // versions of IR both in the LSP and as a codemod, so we apply the transform
-    // ourselves locally.
-    let program = fragment_alias_directive(program, &common::FeatureFlag::Disabled)?;
-    let program = required_directive(&program)?;
+pub fn disallow_required_on_non_null_field(program: &Program) -> DiagnosticsResult<()> {
     let mut validator = DisallowRequiredOnNonNullField::new(&program.schema);
-    validator.validate_program(&program)?;
-    Ok(validator.warnings)
-}
+    validator.validate_program(program)?;
 
-pub fn disallow_required_on_non_null_field_for_executable_definition(
-    schema: &Arc<SDLSchema>,
-    definition: &ExecutableDefinition,
-) -> DiagnosticsResult<Vec<Diagnostic>> {
-    let program = Program::from_definitions(Arc::clone(schema), vec![definition.clone()]);
-    disallow_required_on_non_null_field(&program)
+    if validator.warnings.is_empty() {
+        Ok(())
+    } else {
+        Err(validator.warnings)
+    }
 }
 
 struct DisallowRequiredOnNonNullField<'a> {

diff --git a/compiler/crates/relay-transforms/tests/disallow_required_on_non_null_field.rs b/compiler/crates/relay-transforms/tests/disallow_required_on_non_null_field.rs
@@ -15,6 +15,7 @@ use graphql_syntax::parse_executable;
 use graphql_test_helpers::diagnostics_to_sorted_string;
 use relay_test_schema::get_test_schema_with_extensions;
 use relay_transforms::disallow_required_on_non_null_field;
+use relay_transforms::required_directive;
 
 pub async fn transform_fixture(fixture: &Fixture<'_>) -> Result<String, String> {
     let parts: Vec<_> = fixture.content.split("%extensions%").collect();
@@ -28,14 +29,14 @@ pub async fn transform_fixture(fixture: &Fixture<'_>) -> Result<String, String>
         let ir = build(&schema, &ast.definitions)
             .map_err(|diagnostics| diagnostics_to_sorted_string(fixture.content, &diagnostics))?;
         let program = Program::from_definitions(Arc::clone(&schema), ir);
+        let program = required_directive(&program).unwrap();
         let results = disallow_required_on_non_null_field(&program)
-            .map_err(|diagnostics| diagnostics_to_sorted_string(fixture.content, &diagnostics))?;
+            .map_err(|diagnostics| diagnostics_to_sorted_string(fixture.content, &diagnostics));
 
-        Ok(format!(
-            "OK; warnings: {}",
-            diagnostics_to_sorted_string(fixture.content, &results)
-        )
-        .to_owned())
+        match results {
+            Ok(_) => Ok("OK".to_owned()),
+            Err(diagnostics) => Ok(format!("OK; warnings: {}", diagnostics).to_owned()),
+        }
     } else {
         panic!("Expected exactly one %extensions% section marker.")
     }

diff --git a/...non_null_field/fixtures/fragment_with_required_semantic_field_no_explicit_errors.expected b/...non_null_field/fixtures/fragment_with_required_semantic_field_no_explicit_errors.expected
@@ -8,4 +8,4 @@ extend type User {
   some_field: Int @semanticNonNull
 }
 ==================================== OUTPUT ===================================
-OK; warnings:
+OK
diff --git a/...rms/tests/disallow_required_on_non_null_field/fixtures/query_with_required_field.expected b/...rms/tests/disallow_required_on_non_null_field/fixtures/query_with_required_field.expected
@@ -8,4 +8,4 @@ extend type Query {
   some_field: Int
 }
 ==================================== OUTPUT ===================================
-OK; warnings:
+OK
diff --git a/...required_on_non_null_field/fixtures/query_with_required_field_no_explicit_errors.expected b/...required_on_non_null_field/fixtures/query_with_required_field_no_explicit_errors.expected
@@ -8,4 +8,4 @@ extend type Query {
   some_field: Int
 }
 ==================================== OUTPUT ===================================
-OK; warnings:
+OK
diff --git a/...on_non_null_field/fixtures/query_with_required_semantic_field_no_explicit_errors.expected b/...on_non_null_field/fixtures/query_with_required_semantic_field_no_explicit_errors.expected
@@ -8,4 +8,4 @@ extend type Query {
   some_field: Int @semanticNonNull
 }
 ==================================== OUTPUT ===================================
-OK; warnings:
+OK
diff --git a/...rms/tests/disallow_required_on_non_null_field/fixtures/query_with_semantic_field.expected b/...rms/tests/disallow_required_on_non_null_field/fixtures/query_with_semantic_field.expected
@@ -8,4 +8,4 @@ extend type Query {
   some_field: Int @semanticNonNull
 }
 ==================================== OUTPUT ===================================
-OK; warnings:
+OK
diff --git a/...required_on_non_null_field/fixtures/query_with_semantic_field_no_explicit_errors.expected b/...required_on_non_null_field/fixtures/query_with_semantic_field_no_explicit_errors.expected
@@ -8,4 +8,4 @@ extend type Query {
   some_field: Int @semanticNonNull
 }
 ==================================== OUTPUT ===================================
-OK; warnings:
+OK
diff --git a/..._required_on_non_null_field/fixtures/required_bubbles_to_required_semantic_field.expected b/..._required_on_non_null_field/fixtures/required_bubbles_to_required_semantic_field.expected
@@ -14,4 +14,4 @@ type MyType {
   some_field: Int
 }
 ==================================== OUTPUT ===================================
-OK; warnings:
+OK