diff --git a/Changelog.md b/Changelog.md index d9a7e36..a5b9685 100644 --- a/Changelog.md +++ b/Changelog.md @@ -1,5 +1,32 @@ # ReScript Linter Changelog +### 2026-02-09 - v0.4.2 +* Improved error handling for config file parsing + * Added `ConfigParseError` exception with detailed error messages + * Config parsing errors now show: + * The problematic rule name + * Expected field names for each rule type + * The full rule configuration that failed to parse + * Example error message: + ``` + Config Error: Error parsing rule 'DisallowModule': Expected string, got null + Expected fields: disallowed_module, suggested_module + Rule config: + { + "rule": "DisallowModule", + "options": { + "disallowed_function": "Js.Json", + "suggested_function": "JSON" + } + } + ``` + * Unknown rules now list all valid rule names +* Added comprehensive tests for config parsing errors + * Tests for invalid field names + * Tests for unknown rules + * Tests for missing required fields + * Tests for valid configs + ### 2026-02-02 - v0.4.1 * Update DisallowModuleRule and DisallowedFunctionRule diff --git a/dune-project b/dune-project index e1c3091..6cb7718 100644 --- a/dune-project +++ b/dune-project @@ -2,7 +2,7 @@ (name rescript_linter) -(version 0.4.1) +(version 0.4.2) (generate_opam_files true) diff --git a/lib/ConfigReader.ml b/lib/ConfigReader.ml index 46aabf5..86d2423 100644 --- a/lib/ConfigReader.ml +++ b/lib/ConfigReader.ml @@ -1,5 +1,7 @@ exception RuleDoesNotExist +exception ConfigParseError of string + type t = {rules: (module Rule.HASRULE) list} let createDisallowOperatorRule options (linterOptions : (module Rule.LinterOptions)) = @@ -111,42 +113,75 @@ let parseConfig path = let json = Yojson.Basic.from_file path in let open Yojson.Basic.Util in let filter_rule json = - (* Parse in the linter options that are common to every rule *) - let warning = - match json |> member "warning" with `Null -> false (* default to errors *) | json -> to_bool json - in - let linterOptions = - ( module struct - let warning = warning - end : Rule.LinterOptions ) - in - (* Parse the rule name, this is used to identify the rule being configured - * as each rule can have different parsing options - *) - let rule = json |> member "rule" |> to_string in - (* Not all rules are explicitly required to have options - * but perhaps in the future we can clean up the code duplication - * of parsing options for the rules that do have options - *) - match rule with - | "DisallowOperator" -> - let options = json |> member "options" in - createDisallowOperatorRule options linterOptions - | "DisallowFunction" -> - let options = json |> member "options" in - createDisallowFunctionRule options linterOptions - | "NoReactComponent" -> - let options = json |> member "options" in - createNoReactComponentRule options linterOptions - | "DisallowModule" -> - let options = json |> member "options" in - createDisallowModuleRule options linterOptions - | "DisallowEmbeddedRegexLiteral" -> - let options = json |> member "options" in - createDisallowEmbeddedRegexLiteralRule options linterOptions - | "DisallowAttribute" -> - let options = json |> member "options" in - createDisallowAttributeRule options linterOptions - | _ -> raise RuleDoesNotExist + try + (* Parse in the linter options that are common to every rule *) + let warning = + match json |> member "warning" with `Null -> false (* default to errors *) | json -> to_bool json + in + let linterOptions = + ( module struct + let warning = warning + end : Rule.LinterOptions ) + in + (* Parse the rule name, this is used to identify the rule being configured + * as each rule can have different parsing options + *) + let rule = json |> member "rule" |> to_string in + (* Not all rules are explicitly required to have options + * but perhaps in the future we can clean up the code duplication + * of parsing options for the rules that do have options + *) + match rule with + | "DisallowOperator" -> + let options = json |> member "options" in + createDisallowOperatorRule options linterOptions + | "DisallowFunction" -> + let options = json |> member "options" in + createDisallowFunctionRule options linterOptions + | "NoReactComponent" -> + let options = json |> member "options" in + createNoReactComponentRule options linterOptions + | "DisallowModule" -> + let options = json |> member "options" in + createDisallowModuleRule options linterOptions + | "DisallowEmbeddedRegexLiteral" -> + let options = json |> member "options" in + createDisallowEmbeddedRegexLiteralRule options linterOptions + | "DisallowAttribute" -> + let options = json |> member "options" in + createDisallowAttributeRule options linterOptions + | _ -> raise RuleDoesNotExist + with + | RuleDoesNotExist -> + let rule_name = try json |> member "rule" |> to_string with _ -> "" in + raise + (ConfigParseError + (Printf.sprintf + "Unknown rule '%s' in config file.\n\ + Valid rules are: DisallowOperator, DisallowFunction, DisallowModule, NoReactComponent, \ + DisallowEmbeddedRegexLiteral, DisallowAttribute" + rule_name ) ) + | Type_error (msg, _) -> + let rule_name = try json |> member "rule" |> to_string with _ -> "" in + let options_hint = + match rule_name with + | "DisallowOperator" -> "Expected fields: disallowed_operator, suggested_operator" + | "DisallowFunction" -> "Expected fields: disallowed_function, suggested_function" + | "DisallowModule" -> "Expected fields: disallowed_module, suggested_module" + | "NoReactComponent" -> "Expected fields: component, suggested_component" + | "DisallowEmbeddedRegexLiteral" -> "Expected fields: test_directory" + | "DisallowAttribute" -> "Expected fields: attribute, suggestion (optional)" + | _ -> "Please check the rule configuration" + in + raise + (ConfigParseError + (Printf.sprintf "Error parsing rule '%s': %s\n%s\nRule config:\n%s" rule_name msg options_hint + (Yojson.Basic.pretty_to_string json) ) ) + | e -> + let rule_name = try json |> member "rule" |> to_string with _ -> "" in + raise + (ConfigParseError + (Printf.sprintf "Unexpected error parsing rule '%s': %s\nRule config:\n%s" rule_name + (Printexc.to_string e) (Yojson.Basic.pretty_to_string json) ) ) in json |> member "rules" |> to_list |> List.map filter_rule diff --git a/lib/Linter.ml b/lib/Linter.ml index d2634b6..d5d66b3 100644 --- a/lib/Linter.ml +++ b/lib/Linter.ml @@ -48,7 +48,12 @@ let lintInfo_of_tuple_with_message_kind (message_kind : Printer.PrettyPrint.mess type jsonOutput = {errors: lintInfo list; warnings: lintInfo list} [@@deriving yojson] let run configPath path (outputJson : bool) = - let rules = ConfigReader.parseConfig configPath in + let rules = + try ConfigReader.parseConfig configPath + with ConfigReader.ConfigParseError msg -> + Printf.eprintf "Config Error: %s\n" msg ; + exit 1 + in if not outputJson then Format.fprintf Format.std_formatter "Linting rules:\n%s" (String.concat "\n" diff --git a/rescript_linter.opam b/rescript_linter.opam index a0a7644..fb6f1c5 100644 --- a/rescript_linter.opam +++ b/rescript_linter.opam @@ -1,6 +1,6 @@ # This file is generated by dune, edit dune-project instead opam-version: "2.0" -version: "0.4.0" +version: "0.4.2" synopsis: "AST-based linter for ReScript" description: "AST-based linter for ReScript" maintainer: ["Shulhi Sapli"] diff --git a/test/rescript_linter_test.ml b/test/rescript_linter_test.ml index 3b01fa8..dafcbee 100644 --- a/test/rescript_linter_test.ml +++ b/test/rescript_linter_test.ml @@ -379,6 +379,43 @@ module Tests = struct (List.map (fun (str, loc) -> Printf.sprintf "\t* %s\n\t\t -> %s" str (loc_to_string loc)) errors ) ) + + (* Config parsing error tests *) + let config_invalid_field_names_test () = + try + let _rules = ConfigReader.parseConfig "testData/config_invalid_disallow_module.json" in + Alcotest.fail "Should have raised ConfigParseError for invalid field names" + with ConfigReader.ConfigParseError msg -> + if contains_substring msg "DisallowModule" && contains_substring msg "disallowed_module" then + Alcotest.(check pass) "Correct error message" [] [] + else Alcotest.fail ("Error message should mention DisallowModule and disallowed_module, but got: " ^ msg) + + let config_unknown_rule_test () = + try + let _rules = ConfigReader.parseConfig "testData/config_unknown_rule.json" in + Alcotest.fail "Should have raised ConfigParseError for unknown rule" + with ConfigReader.ConfigParseError msg -> + if contains_substring msg "Unknown rule" && contains_substring msg "NonExistentRule" then + Alcotest.(check pass) "Correct error message" [] [] + else Alcotest.fail ("Error message should mention unknown rule, but got: " ^ msg) + + let config_missing_field_test () = + try + let _rules = ConfigReader.parseConfig "testData/config_missing_field.json" in + Alcotest.fail "Should have raised ConfigParseError for missing field" + with ConfigReader.ConfigParseError msg -> + if contains_substring msg "DisallowOperator" && contains_substring msg "suggested_operator" then + Alcotest.(check pass) "Correct error message" [] [] + else + Alcotest.fail ("Error message should mention DisallowOperator and suggested_operator, but got: " ^ msg) + + let config_valid_test () = + try + let rules = ConfigReader.parseConfig "testData/config_valid.json" in + match rules with + | [_; _] -> Alcotest.(check pass) "Valid config parsed successfully" [] [] + | _ -> Alcotest.fail "Should have parsed 2 rules" + with ConfigReader.ConfigParseError msg -> Alcotest.fail ("Should not have raised error: " ^ msg) end (* Run it *) @@ -407,4 +444,9 @@ let () = ; test_case "module prefix matching (Belt.*)" `Quick Tests.disallow_module_test_5 ] ) ; ( "Disallowed embedded regex literal" , [test_case "Disallowed embedded regex literal" `Quick Tests.disallowed_embedded_regex_literal_test] ) - ; ("Disallowed dead code", [test_case "Disallowed dead code" `Quick Tests.disallowed_dead_code_test]) ] + ; ("Disallowed dead code", [test_case "Disallowed dead code" `Quick Tests.disallowed_dead_code_test]) + ; ( "Config parsing errors" + , [ test_case "Invalid field names" `Quick Tests.config_invalid_field_names_test + ; test_case "Unknown rule" `Quick Tests.config_unknown_rule_test + ; test_case "Missing required field" `Quick Tests.config_missing_field_test + ; test_case "Valid config" `Quick Tests.config_valid_test ] ) ] diff --git a/test/testData/config_invalid_disallow_module.json b/test/testData/config_invalid_disallow_module.json new file mode 100644 index 0000000..61dad1c --- /dev/null +++ b/test/testData/config_invalid_disallow_module.json @@ -0,0 +1,11 @@ +{ + "rules": [ + { + "rule": "DisallowModule", + "options": { + "disallowed_function": "Js.Json", + "suggested_function": "JSON" + } + } + ] +} diff --git a/test/testData/config_missing_field.json b/test/testData/config_missing_field.json new file mode 100644 index 0000000..5d98a1a --- /dev/null +++ b/test/testData/config_missing_field.json @@ -0,0 +1,10 @@ +{ + "rules": [ + { + "rule": "DisallowOperator", + "options": { + "disallowed_operator": "|>" + } + } + ] +} diff --git a/test/testData/config_unknown_rule.json b/test/testData/config_unknown_rule.json new file mode 100644 index 0000000..fb2ea49 --- /dev/null +++ b/test/testData/config_unknown_rule.json @@ -0,0 +1,10 @@ +{ + "rules": [ + { + "rule": "NonExistentRule", + "options": { + "some_field": "value" + } + } + ] +} diff --git a/test/testData/config_valid.json b/test/testData/config_valid.json new file mode 100644 index 0000000..4860796 --- /dev/null +++ b/test/testData/config_valid.json @@ -0,0 +1,18 @@ +{ + "rules": [ + { + "rule": "DisallowModule", + "options": { + "disallowed_module": "Belt.Result", + "suggested_module": "Result" + } + }, + { + "rule": "DisallowFunction", + "options": { + "disallowed_function": "string_of_int", + "suggested_function": "Int.toString" + } + } + ] +}