Skip to content

Commit

Permalink
Record full function position in typing errors
Browse files Browse the repository at this point in the history 
Summary: During type checking we retain the position of the function or method we are currently checking. This diff attaches that information to user errors with a view to providing richer context in extended error reporting.

Reviewed By: andrewjkennedy

Differential Revision: D68557487

fbshipit-source-id: 4bc9287c76e9d3d4562755c5802da656599a7681
  • Loading branch information
@facebook-github-bot
Michael Thomas authored and facebook-github-bot committed Jan 27, 2025
1 parent 417760f commit d30c555
Show file tree
Hide file tree
Showing 14 changed files with 83 additions and 63 deletions.
1 change: 1 addition & 0 deletions hphp/hack/src/client/clientLsp.ml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1162,6 +1162,7 @@ let ide_diagnostics_to_lsp_diagnostics
is_fixmed = _;
flags = _;
quickfixes = _;
function_pos = _;
} =
diagnostic_error
in
Expand Down
1 change: 1 addition & 0 deletions hphp/hack/src/errors/contextual_error_formatter.ml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@ let to_string
quickfixes = _;
flags = _;
is_fixmed = _;
function_pos = _;
} =
error
in
Expand Down
9 changes: 9 additions & 0 deletions hphp/hack/src/errors/errors.ml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ let try_with_result (f1 : unit -> 'res) (f2 : 'res -> error -> 'res) : 'res =
quickfixes;
custom_msgs;
flags;
function_pos;
is_fixmed = _;
} ->
(* Remove bad position sentinel if present: we might be about to add a new primary
Expand All @@ -223,6 +224,7 @@ let try_with_result (f1 : unit -> 'res) (f2 : 'res -> error -> 'res) : 'res =
~quickfixes
~custom_msgs
~flags
?function_pos

let try_with_result_pure ~fail f g =
let error_map_copy = !error_map in
Expand Down Expand Up @@ -326,6 +328,7 @@ let compare_internal (x : ('a, 'b) User_error.t) (y : ('a, 'b) User_error.t) :
quickfixes = _;
is_fixmed = _;
flags = _;
function_pos = _;
} =
x
in
Expand All @@ -340,6 +343,7 @@ let compare_internal (x : ('a, 'b) User_error.t) (y : ('a, 'b) User_error.t) :
quickfixes = _;
is_fixmed = _;
flags = _;
function_pos = _;
} =
y
in
Expand Down Expand Up @@ -707,6 +711,7 @@ let add_error_with_fixme_error error explanation ~fixme_pos =
quickfixes;
custom_msgs;
flags;
function_pos;
is_fixmed = _;
} =
error
Expand All @@ -722,6 +727,7 @@ let add_error_with_fixme_error error explanation ~fixme_pos =
~quickfixes
~custom_msgs
~flags
?function_pos

let add_applied_fixme error =
let error = { error with User_error.is_fixmed = true } in
Expand Down Expand Up @@ -841,6 +847,7 @@ let add_error (error : error) =
quickfixes;
custom_msgs;
flags;
function_pos;
is_fixmed = _;
} =
error
Expand All @@ -856,6 +863,7 @@ let add_error (error : error) =
~quickfixes
~custom_msgs
~flags
?function_pos
in

let pos = fst claim in
Expand Down Expand Up @@ -1334,6 +1342,7 @@ let convert_errors_to_string ?(include_filename = false) (errors : error list) :
quickfixes = _;
is_fixmed = _;
flags = _;
function_pos = _;
} =
err
in
Expand Down
1 change: 1 addition & 0 deletions hphp/hack/src/errors/fmt_plain.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ impl<'a> std::fmt::Display for FmtPlain<'a> {
quickfixes: _,
is_fixmed: _,
flags: _,
function_pos: _,
},
ctx,
) = self;
Expand Down
1 change: 1 addition & 0 deletions hphp/hack/src/errors/fmt_raw.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ impl<'a> std::fmt::Display for FmtRaw<'a> {
quickfixes: _,
is_fixmed: _,
flags: _,
function_pos: _,
},
ctx,
is_term,
Expand Down
1 change: 1 addition & 0 deletions hphp/hack/src/errors/highlighted_error_formatter.ml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,7 @@ let to_string (error : Errors.finalized_error) : string =
quickfixes = _;
flags = _;
is_fixmed = _;
function_pos = _;
} =
error
in
Expand Down
1 change: 1 addition & 0 deletions hphp/hack/src/errors/raw_error_formatter.ml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ let to_string (error : Errors.finalized_error) : string =
custom_msgs = _;
is_fixmed = _;
flags = _;
function_pos = _;
} =
error
in
Expand Down
109 changes: 49 additions & 60 deletions hphp/hack/src/errors/user_error.ml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ type ('prim_pos, 'pos) t = {
custom_msgs: string list;
is_fixmed: bool;
flags: User_error_flags.t;
function_pos: 'prim_pos option;
}
[@@deriving eq, hash, ord, show]

Expand All @@ -49,6 +50,7 @@ let make
?(quickfixes = [])
?(custom_msgs = [])
?(flags = User_error_flags.empty)
?function_pos
claim
reasons
explanation =
Expand All @@ -62,17 +64,27 @@ let make
custom_msgs;
is_fixmed;
flags;
function_pos;
}

let make_err
code ?is_fixmed ?quickfixes ?custom_msgs ?flags claim reasons explanation =
code
?is_fixmed
?quickfixes
?custom_msgs
?flags
?function_pos
claim
reasons
explanation =
make
Err
code
?is_fixmed
?quickfixes
?custom_msgs
?flags
?function_pos
claim
reasons
explanation
Expand Down Expand Up @@ -113,12 +125,14 @@ let to_absolute
custom_msgs;
is_fixmed;
flags;
function_pos;
} =
let pos_or_decl_to_absolute_pos pos_or_decl =
let pos = Pos_or_decl.unsafe_to_raw_pos pos_or_decl in
Pos.to_absolute pos
in
let claim = (fst claim |> Pos.to_absolute, snd claim) in
let function_pos = Option.map ~f:Pos.to_absolute function_pos in
let reasons =
List.map reasons ~f:(fun (p, s) -> (pos_or_decl_to_absolute_pos p, s))
in
Expand All @@ -137,39 +151,18 @@ let to_absolute
custom_msgs;
is_fixmed;
flags;
function_pos;
}

let to_relative
{
severity;
code;
claim;
reasons;
explanation;
quickfixes;
custom_msgs;
is_fixmed;
flags;
} : (Pos.t, Pos.t) t =
let claim = (fst claim, snd claim) in
let to_relative ({ reasons; explanation; _ } as t) : (Pos.t, Pos.t) t =
let reasons =
List.map reasons ~f:(fun (p, s) -> (p |> Pos_or_decl.unsafe_to_raw_pos, s))
in
let explanation =
Explanation.map explanation ~f:(fun pos ->
Pos_or_decl.unsafe_to_raw_pos pos)
in
{
severity;
code;
claim;
reasons;
explanation;
quickfixes;
custom_msgs;
is_fixmed;
flags;
}
{ t with reasons; explanation }

let make_absolute severity code = function
| [] -> failwith "an error must have at least one message"
Expand All @@ -184,6 +177,7 @@ let make_absolute severity code = function
custom_msgs = [];
is_fixmed = false;
flags = User_error_flags.empty;
function_pos = None;
}

let to_absolute_for_test
Expand All @@ -197,6 +191,7 @@ let to_absolute_for_test
custom_msgs;
is_fixmed;
flags;
function_pos;
} =
let f (p, s) =
let p = Pos_or_decl.unsafe_to_raw_pos p in
Expand All @@ -217,6 +212,10 @@ let to_absolute_for_test
Pos.to_absolute @@ Pos_or_decl.unsafe_to_raw_pos pos)
in
let quickfixes = List.map quickfixes ~f:Quickfix.to_absolute in
let function_pos =
Option.map function_pos ~f:(fun pos ->
fst @@ f (Pos_or_decl.of_raw_pos pos, ()))
in
{
severity;
code;
Expand All @@ -227,6 +226,7 @@ let to_absolute_for_test
custom_msgs;
is_fixmed;
flags;
function_pos;
}

let error_kind error_code =
Expand Down Expand Up @@ -255,18 +255,7 @@ let error_code_to_string error_code =
And this too
*)
let to_string
report_pos_from_reason
{
severity;
code;
claim;
reasons;
explanation = _;
custom_msgs;
quickfixes = _;
is_fixmed = _;
flags = _;
} =
report_pos_from_reason { severity; code; claim; reasons; custom_msgs; _ } =
let buf = Buffer.create 50 in
let (pos1, msg1) = claim in
Buffer.add_string
Expand Down Expand Up @@ -306,17 +295,7 @@ let to_string

let to_json ~(human_formatter : (_ -> string) option) ~filename_to_string error
=
let {
severity;
code;
claim;
reasons;
explanation = _;
custom_msgs;
quickfixes = _;
is_fixmed = _;
flags;
} =
let { severity; code; claim; reasons; flags; custom_msgs; function_pos; _ } =
error
in
let msgl = claim :: reasons in
Expand All @@ -339,17 +318,27 @@ let to_json ~(human_formatter : (_ -> string) option) ~filename_to_string error
let flags = User_error_flags.to_json flags in
let human_format = Option.map ~f:(fun f -> f error) human_formatter in
let obj =
[
("severity", Hh_json.JSON_String (Severity.to_string severity));
("message", Hh_json.JSON_Array elts);
("custom_messages", Hh_json.JSON_Array custom_msgs);
("flags", flags);
]
in
let obj =
match human_format with
| None -> obj
| Some human_format ->
obj @ [("human_format", Hh_json.JSON_String human_format)]
("severity", Hh_json.JSON_String (Severity.to_string severity))
:: ("message", Hh_json.JSON_Array elts)
:: ("custom_messages", Hh_json.JSON_Array custom_msgs)
:: ("flags", flags)
:: List.filter_opt
[
Option.map human_format ~f:(fun human_format ->
("human_format", Hh_json.JSON_String human_format));
Option.map function_pos ~f:(fun pos ->
let (line, scol, ecol) = Pos.info_pos pos in
( "function_pos",
Hh_json.JSON_Object
[
( "path",
Hh_json.JSON_String
(Pos.filename pos |> filename_to_string) );
("line", Hh_json.int_ line);
("start", Hh_json.int_ scol);
("end", Hh_json.int_ ecol);
("code", Hh_json.int_ code);
] ));
]
in
Hh_json.JSON_Object obj
3 changes: 3 additions & 0 deletions hphp/hack/src/errors/user_error.mli
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ type ('prim_pos, 'pos) t = {
custom_msgs: string list;
is_fixmed: bool;
flags: User_error_flags.t;
function_pos: 'prim_pos option;
}
[@@deriving eq, hash, ord, show]

Expand All @@ -39,6 +40,7 @@ val make :
?quickfixes:'a Quickfix.t list ->
?custom_msgs:string list ->
?flags:User_error_flags.t ->
?function_pos:'a ->
'a Message.t ->
'b Message.t list ->
'b Explanation.t ->
Expand All @@ -51,6 +53,7 @@ val make_err :
?quickfixes:'a Quickfix.t list ->
?custom_msgs:string list ->
?flags:User_error_flags.t ->
?function_pos:'a ->
'a Message.t ->
'b Message.t list ->
'b Explanation.t ->
Expand Down
3 changes: 2 additions & 1 deletion hphp/hack/src/oxidized/gen/user_error.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
// This source code is licensed under the MIT license found in the
// LICENSE file in the "hack" directory of this source tree.
//
// @generated SignedSource<<a8c5c5af170cd95eedd7f7eb5cf480e5>>
// @generated SignedSource<<203d04bb83a85cebe6d06a1f3d91943d>>
//
// To regenerate this file, run:
// hphp/hack/src/oxidized_regen.sh
Expand Down Expand Up @@ -75,4 +75,5 @@ pub struct UserError<PrimPos, Pos> {
pub custom_msgs: Vec<String>,
pub is_fixmed: bool,
pub flags: user_error_flags::UserErrorFlags,
pub function_pos: Option<PrimPos>,
}
3 changes: 3 additions & 0 deletions hphp/hack/src/oxidized/manual/errors_impl.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ impl<PP, P> UserError<PP, P> {
custom_msgs,
is_fixmed: false,
flags,
function_pos: None,
}
}

Expand Down Expand Up @@ -67,6 +68,7 @@ impl<PP: Ord + FileOrd, P: Ord + FileOrd> UserError<PP, P> {
quickfixes: _,
is_fixmed: _,
flags: _,
function_pos: _,
} = self;
let Self {
severity: other_severity,
Expand All @@ -78,6 +80,7 @@ impl<PP: Ord + FileOrd, P: Ord + FileOrd> UserError<PP, P> {
quickfixes: _,
is_fixmed: _,
flags: _,
function_pos: _,
} = other;
let self_phase = self_code / 1000;
let other_phase = other_code / 1000;
Expand Down
Loading

0 comments on commit d30c555

Please sign in to comment.