From 29c4124eed7438376a624bd9d2656f12a3362a89 Mon Sep 17 00:00:00 2001 From: Wilfred Hughes Date: Mon, 24 Jun 2019 11:19:17 -0700 Subject: [PATCH] Improve error message on misplaced async modifiers Summary: For syntax errors due to a misplaced `async` modifier, inform the user what they should do. Also improve the error message so it's specific to the current code (interface methods versus abstract methods). Remove the corresponding NastCheck for async modifiers, as it was dead code (we stop at the syntax error). Differential Revision: D15920020 fbshipit-source-id: 3aa80a68ade2dafafae3d65f22403c73f372698b --- hphp/hack/src/errors/error_codes.ml | 2 +- hphp/hack/src/errors/errors.ml | 4 --- hphp/hack/src/errors/errors.mli | 1 - .../hack/src/parser/full_fidelity_parser_errors.ml | 40 ++++++++++++---------- hphp/hack/src/parser/full_fidelity_syntax_error.ml | 5 +-- .../hack/src/parser/full_fidelity_syntax_error.mli | 2 +- hphp/hack/src/parser/syntax_error.rs | 11 +++--- hphp/hack/src/typing/nast_check/interface_check.ml | 10 +----- hphp/hack/test/errors/error_map.ml | 2 +- .../cases/test_async_errors.php.errors.exp | 6 ++-- .../test/typecheck/interface_async_method.php.exp | 2 +- hphp/test/slow/parser/async_bad_class.php.expectf | 2 +- hphp/test/slow/parser/async_bad_if.php.expectf | 2 +- hphp/test/slow/parser/async_bad_tr.php.expectf | 2 +- 14 files changed, 43 insertions(+), 48 deletions(-) diff --git a/hphp/hack/src/errors/error_codes.ml b/hphp/hack/src/errors/error_codes.ml index 7f56b250d6c..35d45704dc0 100644 --- a/hphp/hack/src/errors/error_codes.ml +++ b/hphp/hack/src/errors/error_codes.ml @@ -193,7 +193,7 @@ module NastCheck = struct | MultipleXhpCategory | OptionalShapeFieldsNotSupportedDEPRECATED | AwaitNotAllowed - | AsyncInInterface + | AsyncInInterfaceDEPRECATED | AwaitInCoroutine | YieldInCoroutine | SuspendOutsideOfCoroutine diff --git a/hphp/hack/src/errors/errors.ml b/hphp/hack/src/errors/errors.ml index 804462c8685..c21f827706f 100644 --- a/hphp/hack/src/errors/errors.ml +++ b/hphp/hack/src/errors/errors.ml @@ -1414,10 +1414,6 @@ let await_not_allowed p = "await is only permitted as a statement, expression in a return statement \ or as a right hand side in top level assignment." -let async_in_interface p = - add (NastCheck.err_code NastCheck.AsyncInInterface) p - "async is only meaningful when it modifies a method body" - let await_in_coroutine p = add (NastCheck.err_code NastCheck.AwaitInCoroutine) p "await is not allowed in coroutines." diff --git a/hphp/hack/src/errors/errors.mli b/hphp/hack/src/errors/errors.mli index ab45d9a20ce..1b051b13246 100644 --- a/hphp/hack/src/errors/errors.mli +++ b/hphp/hack/src/errors/errors.mli @@ -290,7 +290,6 @@ val toplevel_continue : Pos.t -> unit val continue_in_switch : Pos.t -> unit val await_in_sync_function : Pos.t -> unit val await_not_allowed : Pos.t -> unit -val async_in_interface : Pos.t -> unit val await_in_coroutine : Pos.t -> unit val yield_in_coroutine : Pos.t -> unit val suspend_outside_of_coroutine : Pos.t -> unit diff --git a/hphp/hack/src/parser/full_fidelity_parser_errors.ml b/hphp/hack/src/parser/full_fidelity_parser_errors.ml index 564022b1369..bc4af04d5a3 100644 --- a/hphp/hack/src/parser/full_fidelity_parser_errors.ml +++ b/hphp/hack/src/parser/full_fidelity_parser_errors.ml @@ -895,11 +895,6 @@ let is_inside_interface context = let is_inside_trait context = Option.is_some (first_parent_classish_node TokenKind.Trait context) -(* Tests if md_node is either explicitly declared abstract or is - * defined inside an interface *) -let is_generalized_abstract_method md_node context = - is_abstract_declaration md_node || is_inside_interface context - (* Returns the 'async'-annotation syntax node from the methodish_declaration * node. The returned node may have syntax kind 'Missing', but it will only be * None if something other than a methodish_declaration node was provided as @@ -909,6 +904,22 @@ let extract_async_node md_node = |> Option.value_map ~default:[] ~f:syntax_to_list_no_separators |> List.find ~f:is_async +let is_abstract_and_async_method md_node _context = + let async_node = extract_async_node md_node in + match async_node with + | None -> false + | Some async_node -> + is_abstract_declaration md_node + && not (is_missing async_node) + +let is_interface_and_async_method md_node context = + let async_node = extract_async_node md_node in + match async_node with + | None -> false + | Some async_node -> + is_inside_interface context + && not (is_missing async_node) + let get_params_for_enclosing_callable context = context.active_callable |> Option.bind ~f:(fun callable -> match syntax callable with @@ -1014,16 +1025,6 @@ let check_type_hint env node names errors = in check (names, errors) node -(* Given a node and its context, tests if the node declares a method that is - * both abstract and async. *) -let is_abstract_and_async_method md_node context = - let async_node = extract_async_node md_node in - match async_node with - | None -> false - | Some async_node -> - is_generalized_abstract_method md_node context - && not (is_missing async_node) - let extract_callconv_node node = match syntax node with | ParameterDeclaration { parameter_call_convention; _ } -> @@ -1492,12 +1493,15 @@ let methodish_errors env node errors = node SyntaxError.error2045 modifiers in let errors = methodish_memoize_lsb_on_non_static node errors in + let async_annotation = Option.value (extract_async_node node) ~default:node in + let errors = + produce_error errors + (is_interface_and_async_method node) env.context + (SyntaxError.error2046 "a method in an interface") async_annotation in let errors = - let async_annotation = Option.value (extract_async_node node) - ~default:node in produce_error errors (is_abstract_and_async_method node) env.context - SyntaxError.error2046 async_annotation in + (SyntaxError.error2046 "an abstract method") async_annotation in let errors = if is_typechecker env then produce_error errors diff --git a/hphp/hack/src/parser/full_fidelity_syntax_error.ml b/hphp/hack/src/parser/full_fidelity_syntax_error.ml index e3be56759fc..98b7e49ef9f 100644 --- a/hphp/hack/src/parser/full_fidelity_syntax_error.ml +++ b/hphp/hack/src/parser/full_fidelity_syntax_error.ml @@ -204,8 +204,9 @@ let error2044 class_name method_name = Printf.sprintf ("Classes cannot both " ^^ "contain abstract methods and be non-abstract. Either declare 'abstract " ^^ "class %s', or make 'function %s' non-abstract.") class_name method_name let error2045 = "No method inside an interface may be declared 'abstract'." -let error2046 = "The 'async' annotation cannot be used on 'abstract' methods " ^ - "or methods inside of interfaces." +let error2046 method_type = Printf.sprintf + "'async' cannot be used on %s. Use an Awaitable<...> return type instead." + method_type let error2047 visibility_modifier = "Methods inside of interfaces may not be " ^ "marked '" ^ visibility_modifier ^ "'; only 'public' visibility is allowed." let error2048 = "Expected group use prefix to end with '\\'" diff --git a/hphp/hack/src/parser/full_fidelity_syntax_error.mli b/hphp/hack/src/parser/full_fidelity_syntax_error.mli index 58d1411f36d..2a19a3d19a5 100644 --- a/hphp/hack/src/parser/full_fidelity_syntax_error.mli +++ b/hphp/hack/src/parser/full_fidelity_syntax_error.mli @@ -142,7 +142,7 @@ val error2042 : string val error2043 : string val error2044 : string -> string -> string val error2045 : string -val error2046 : string +val error2046 : string -> string val error2047 : string -> string val error2048 : string val error2049 : string diff --git a/hphp/hack/src/parser/syntax_error.rs b/hphp/hack/src/parser/syntax_error.rs index 33966e215cf..4f95faa0d4f 100644 --- a/hphp/hack/src/parser/syntax_error.rs +++ b/hphp/hack/src/parser/syntax_error.rs @@ -211,10 +211,13 @@ pub const error2042: Error = Cow::Borrowed("Interfaces may not be declared 'abst pub const error2043: Error = Cow::Borrowed("Traits may not be declared 'abstract'."); pub const error2045: Error = Cow::Borrowed("No method inside an interface may be declared 'abstract'."); -pub const error2046: Error = Cow::Borrowed(concat!( - "The 'async' annotation cannot be used on 'abstract' methods ", - "or methods inside of interfaces." -)); +pub fn error2046(method_type: &str) -> Error { + Cow::Owned(format!( + "'async' cannot be used on {}. Use an Awaitable<...> return type instead.", + method_type.to_string(), + )) +} + pub const error2048: Error = Cow::Borrowed("Expected group use prefix to end with '\\'"); pub const error2049: Error = Cow::Borrowed("A namespace use clause may not specify the kind here."); pub const error2050: Error = diff --git a/hphp/hack/src/typing/nast_check/interface_check.ml b/hphp/hack/src/typing/nast_check/interface_check.ml index 278c469b48d..4866a75d90e 100644 --- a/hphp/hack/src/typing/nast_check/interface_check.ml +++ b/hphp/hack/src/typing/nast_check/interface_check.ml @@ -17,13 +17,6 @@ let enforce_no_body m = then Errors.not_public_or_protected_interface (fst m.m_name) | _ -> Errors.abstract_body (fst m.m_name) -(* make sure that interface methods are not async, in line with HHVM *) -let enforce_not_async m = - match m.m_fun_kind with - | Ast.FAsync -> Errors.async_in_interface (fst m.m_name) - | Ast.FAsyncGenerator -> Errors.async_in_interface (fst m.m_name) - | _ -> () - let check_interface c = List.iter c.c_uses (fun (p, _) -> Errors.interface_use_trait p); @@ -50,8 +43,7 @@ let check_interface c = end; (* make sure that interfaces only have empty public methods *) - List.iter ~f:enforce_no_body c.c_methods; - List.iter ~f:enforce_not_async c.c_methods + List.iter ~f:enforce_no_body c.c_methods let handler = object inherit Nast_visitor.handler_base diff --git a/hphp/hack/test/errors/error_map.ml b/hphp/hack/test/errors/error_map.ml index b1d8699ba6e..c3f63b764e9 100644 --- a/hphp/hack/test/errors/error_map.ml +++ b/hphp/hack/test/errors/error_map.ml @@ -192,7 +192,7 @@ InterfaceWithPartialTypeconst = 3031 MultipleXhpCategory = 3032 OptionalShapeFieldsNotSupportedDEPRECATED = 3033 AwaitNotAllowed = 3034 -AsyncInInterface = 3035 +AsyncInInterfaceDEPRECATED = 3035 AwaitInCoroutine = 3036 YieldInCoroutine = 3037 SuspendOutsideOfCoroutine = 3038 diff --git a/hphp/hack/test/full_fidelity/cases/test_async_errors.php.errors.exp b/hphp/hack/test/full_fidelity/cases/test_async_errors.php.errors.exp index e853bd6bb7e..7ad6ab0d2f5 100644 --- a/hphp/hack/test/full_fidelity/cases/test_async_errors.php.errors.exp +++ b/hphp/hack/test/full_fidelity/cases/test_async_errors.php.errors.exp @@ -1,3 +1,3 @@ -(5,10)-(5,14) The 'async' annotation cannot be used on 'abstract' methods or methods inside of interfaces. -(11,19)-(11,23) The 'async' annotation cannot be used on 'abstract' methods or methods inside of interfaces. -(17,19)-(17,23) The 'async' annotation cannot be used on 'abstract' methods or methods inside of interfaces. +(5,10)-(5,14) 'async' cannot be used on a method in an interface. Use an Awaitable<...> return type instead. +(11,19)-(11,23) 'async' cannot be used on an abstract method. Use an Awaitable<...> return type instead. +(17,19)-(17,23) 'async' cannot be used on an abstract method. Use an Awaitable<...> return type instead. diff --git a/hphp/hack/test/typecheck/interface_async_method.php.exp b/hphp/hack/test/typecheck/interface_async_method.php.exp index 3a0f0fb6309..729ebfa4b2e 100644 --- a/hphp/hack/test/typecheck/interface_async_method.php.exp +++ b/hphp/hack/test/typecheck/interface_async_method.php.exp @@ -1,2 +1,2 @@ File "interface_async_method.php", line 4, characters 10-13: -The 'async' annotation cannot be used on 'abstract' methods or methods inside of interfaces. (Parsing[1002]) +'async' cannot be used on a method in an interface. Use an Awaitable<...> return type instead. (Parsing[1002]) diff --git a/hphp/test/slow/parser/async_bad_class.php.expectf b/hphp/test/slow/parser/async_bad_class.php.expectf index f7d74252670..1ba5a5acefa 100644 --- a/hphp/test/slow/parser/async_bad_class.php.expectf +++ b/hphp/test/slow/parser/async_bad_class.php.expectf @@ -1,2 +1,2 @@ -Fatal error: The 'async' annotation cannot be used on 'abstract' methods or methods inside of interfaces. in %s/async_bad_class.php on line 3 +Fatal error: 'async' cannot be used on an abstract method. Use an Awaitable<...> return type instead. in %s/async_bad_class.php on line 3 diff --git a/hphp/test/slow/parser/async_bad_if.php.expectf b/hphp/test/slow/parser/async_bad_if.php.expectf index cf5a76e329a..58a90f6cbfa 100644 --- a/hphp/test/slow/parser/async_bad_if.php.expectf +++ b/hphp/test/slow/parser/async_bad_if.php.expectf @@ -1,2 +1,2 @@ -Fatal error: The 'async' annotation cannot be used on 'abstract' methods or methods inside of interfaces. in %s/async_bad_if.php on line 3 +Fatal error: 'async' cannot be used on a method in an interface. Use an Awaitable<...> return type instead. in %s/async_bad_if.php on line 3 diff --git a/hphp/test/slow/parser/async_bad_tr.php.expectf b/hphp/test/slow/parser/async_bad_tr.php.expectf index e705b3971f6..8ff74f7bf8a 100644 --- a/hphp/test/slow/parser/async_bad_tr.php.expectf +++ b/hphp/test/slow/parser/async_bad_tr.php.expectf @@ -1,2 +1,2 @@ -Fatal error: The 'async' annotation cannot be used on 'abstract' methods or methods inside of interfaces. in %s/async_bad_tr.php on line 3 +Fatal error: 'async' cannot be used on an abstract method. Use an Awaitable<...> return type instead. in %s/async_bad_tr.php on line 3 -- 2.11.4.GIT