Permalink
Browse files

Compare default values of parameters by re-parsing them.

Summary:
Currently checking default values is as significant missing piece in semdiff - we used to compare them as strings using some minor normalization rules but many aspects that don't affect semantics (like trailing commas) were still reported as errors. In this diff we check default values by parsing and lowering them, normalizing lowered trees and then comparing string representation of syntax trees. Using this approach already allowed to:
- discover corner case in parser related to recognizing `$` token
- find incorrect output for default values containing XHP
- discover issue related to incorrect representation of special characters in default values

All these issues are fixed in this diff.

Reviewed By: nbenton

Differential Revision: D7227814

fbshipit-source-id: 75b71a73d97586736660cc0ecf6ebd7a9110b1c7
  • Loading branch information...
vladima authored and hhvm-bot committed Mar 13, 2018
1 parent f91e264 commit e3fc98ae0315fdae119f9563ec0ede83426145ca
@@ -833,7 +833,7 @@ let string_of_type_info_option tio =
type default_value_printing_env = {
codegen_env : Emit_env.t option;
use_single_quote : bool;
in_xhp: bool;
}
let rec string_of_afield ~env = function
@@ -1030,17 +1030,18 @@ and string_of_statement ~env ~indent ((_, stmt_) : A.stmt) =
text
and string_of_expression ~env e =
string_of_param_default_value ~env:{ env with use_single_quote = true } e
string_of_param_default_value ~env e
and string_of_xml ~env (_, id) attributes children =
let env = { env with in_xhp = true } in
let p = Pos.none in
let name = SU.Xhp.mangle id in
let _, attributes =
List.fold_right (string_of_xhp_attr p) attributes (0, [])
in
let attributes = string_of_param_default_value ~env (p, A.Array attributes) in
let attributes = string_of_param_default_value ~env (p, A.Darray attributes) in
let children = string_of_param_default_value ~env
(p, A.Array (List.map (fun e -> A.AFvalue e) children))
(p, A.Varray children)
in
"new "
^ name
@@ -1051,10 +1052,10 @@ and string_of_xml ~env (_, id) attributes children =
^ ", __FILE__, __LINE__)"
and string_of_xhp_attr p attr (spread_id, attrs) = match attr with
| A.Xhp_simple (id, e) -> (spread_id, (A.AFkvalue ((p, A.String id), e))::attrs)
| A.Xhp_simple (id, e) -> (spread_id, ((p, A.String id), e)::attrs)
| A.Xhp_spread e ->
let id = (p, "...$" ^ (string_of_int spread_id)) in
(spread_id + 1, (A.AFkvalue ((p, A.String id), e))::attrs)
(spread_id + 1, ((p, A.String id), e)::attrs)
and string_of_param_default_value ~env expr =
let p = Pos.none in
@@ -1104,7 +1105,6 @@ and string_of_param_default_value ~env expr =
~env:env.codegen_env ~should_format:false ~is_class_constant:false s1 in
let e =
(fst expr, if is_array_get then A.Id (p, s1) else A.String (p, s1)) in
let env = { env with use_single_quote = false } in
Some (string_of_param_default_value ~env e)
| _ -> None
in
@@ -1132,9 +1132,7 @@ and string_of_param_default_value ~env expr =
| A.Float (_, litstr) -> SU.Float.with_scientific_notation litstr
| A.Int (_, litstr) -> SU.Integer.to_decimal litstr
| A.String (_, litstr) ->
if env.use_single_quote
then SU.single_quote_string_with_escape ~f:escape_fn litstr
else SU.quote_string_with_escape ~f:escape_fn litstr
SU.quote_string_with_escape ~f:escape_fn litstr
| A.Null -> "NULL"
| A.True -> "true"
| A.False -> "false"
@@ -1145,7 +1143,6 @@ and string_of_param_default_value ~env expr =
"array(" ^ string_of_afield_list ~env afl ^ ")"
| A.Collection ((_, name), afl) when
name = "vec" || name = "dict" || name = "keyset" ->
let env = { env with use_single_quote = false } in
name ^ "[" ^ string_of_afield_list ~env afl ^ "]"
| A.Collection ((_, name), afl) ->
let name = SU.Types.fix_casing @@ SU.strip_ns name in
@@ -1267,12 +1264,21 @@ and string_of_param_default_value ~env expr =
let h = string_of_hint ~ns:true h in
e ^ " is " ^ h
| A.Varray es ->
let index i = p, A.Int (p, string_of_int i) in
string_of_param_default_value ~env @@
(p, A.Array (List.mapi (fun i e -> A.AFkvalue (index i, e)) es))
if env.in_xhp
then begin
let es = List.map (string_of_param_default_value ~env) es in
"varray[" ^ (String.concat ", " es) ^ "]"
end
else begin
let index i = p, A.Int (p, string_of_int i) in
string_of_param_default_value ~env @@
(p, A.Array (List.mapi (fun i e -> A.AFkvalue (index i, e)) es))
end
| A.Darray es ->
string_of_param_default_value ~env @@
(p, A.Array (List.map (fun (e1, e2) -> A.AFkvalue (e1, e2)) es))
let es = List.map (fun (e1, e2) -> A.AFkvalue (e1, e2)) es in
if env.in_xhp
then "darray[" ^ (string_of_afield_list ~env es) ^ "]"
else string_of_param_default_value ~env @@ (p, A.Array es)
| A.Import (fl, e) ->
let fl = string_of_import_flavor fl in
let e = string_of_param_default_value ~env e in
@@ -1295,9 +1301,7 @@ and string_of_param_default_value ~env expr =
let string_of_param_default_value_option env = function
| None -> ""
| Some (label, expr) ->
let use_single_quote =
match snd expr with A.String _ -> false | _ -> true in
let env = { codegen_env = env; use_single_quote } in
let env = { codegen_env = env; in_xhp = false } in
" = "
^ (string_of_label label)
^ "(\"\"\""
@@ -172,19 +172,66 @@ let int_comparer = primitive_comparer string_of_int
let string_comparer = primitive_comparer (fun s -> s)
let string_no_whitespace_no_casing_comparer =
(* TODO(T25624348): Remove this shortcut for arrays *)
let contains_array_like_element s =
String_utils.is_substring "array" s ||
String_utils.is_substring "vec" s ||
String_utils.is_substring "dict" s in
primitive_comparer (fun s -> if contains_array_like_element s then "" else
Str.global_replace (Str.regexp {|[\n\t\r\v ]|}) "" @@
String.lowercase_ascii s)
let default_value_text_comparer =
let env = Full_fidelity_ast.make_env
~codegen:true
~keep_errors:false Relative_path.default
~fail_open:false
~php5_compat_mode:true in
(* remove explicit array keys when its value matches with implicit order *)
let remove_explicit_array_keys e =
let v = object(self)
inherit [_] Ast.endo as super
method! on_expr nenv expr =
let expr = super#on_expr nenv expr in
Ast_constant_folder.fold_expr nenv expr
method! on_Array nenv _ values =
let values = self#on_list self#on_afield nenv values in
let _, values = Hh_core.List.map_env (Some 0L) values (fun exp_i el ->
match el, exp_i with
| Ast.AFvalue _, Some exp_i -> Some (Int64.add exp_i 1L), el
| Ast.AFkvalue ((_, Ast.Int (_, k)), v), _ ->
let i_opt =
Typed_value.string_to_int_opt
~allow_following:false ~allow_inf:false k in
begin match i_opt, exp_i with
| Some i, Some exp when i = exp ->
Some (Int64.add i 1L), Ast.AFvalue v
| Some i, _ ->
Some (Int64.add i 1L), el
| None, _ ->
None, el
end
| _ -> None, el
) in
Ast.Array values
end in
v#on_expr Namespace_env.empty_with_default_popt e in
let expr_to_sexp s =
let s = Php_escaping.unescape_long_string s in
let text = "<?hh\n" ^ "(" ^ s ^ ");" in
let text = Full_fidelity_source_text.make Relative_path.default text in
let ast = Full_fidelity_ast.from_text env text in
match ast.Full_fidelity_ast.ast with
| Ast.([Stmt (_, Markup _); Stmt (_, Expr e)]) ->
Debug.dump_ast (Ast.AExpr (remove_explicit_array_keys e))
| a -> failwith @@
"Unexpected shape of default value:" ^ (Debug.dump_ast (Ast.AProgram a)) in
{
comparer = (fun n1 n2 ->
if n1 = n2 then (0,(1,[]))
else
let n1str = expr_to_sexp n1 in
let n2str = expr_to_sexp n2 in
if n1str = n2str then (0,(1,[]))
else (1,(1,substedit n1str n2str)));
size_of = (fun _n -> 1);
string_of = expr_to_sexp;
}
let bool_comparer = primitive_comparer string_of_bool
(* wrap takes a (function a->b), a (function a->string->string), a b-comparer
(* wrap takes a (function a->b), a (function a->string->string), a b-comparer
and returns an a-comparer
TODO: this is still not quite right, as we want to be able to customize the
wrapped edits, and that should be compatible with what we do in string_of.
@@ -446,7 +493,7 @@ let param_ti_name_reference_comparer =
param_name_reference_comparer
let param_default_value_expression_comparer =
wrap (function (_, (_, Ast.String (_, s))) -> s | _ -> "")
(fun _e s -> s) string_no_whitespace_no_casing_comparer
(fun _e s -> s) default_value_text_comparer
let param_default_value_comparer = wrap Hhas_param.default_value
(fun _p s -> s)
(option_comparer param_default_value_expression_comparer)
@@ -973,7 +973,7 @@ let scan_dollar_token lexer =
match ch1 with
| '$' ->
let ch2 = peek_char lexer 2 in
if ch2 = '$' || is_name_nondigit ch2 then
if ch2 = '$' || ch2 = '{' || is_name_nondigit ch2 then
(advance lexer 1, TokenKind.Dollar) (* $$x or $$$*)
else
(advance lexer 2, TokenKind.DollarDollar) (* $$ *)
@@ -78,6 +78,8 @@ type literal_kind =
| Literal_heredoc
| Literal_double_quote
| Literal_backtick
(* string containing C-style char escapes (mimics escapeChar in as.cpp) *)
| Literal_long_string
let unescape_literal literal_kind s =
let len = String.length s in
@@ -101,24 +103,33 @@ let unescape_literal literal_kind s =
if c <> '\\' || !idx = len then Buffer.add_char buf c else begin
let c = next () in
match c with
| 'a' when literal_kind = Literal_long_string ->
Buffer.add_char buf '\x07'
| 'b' when literal_kind = Literal_long_string ->
Buffer.add_char buf '\x08'
| '\'' -> Buffer.add_string buf "\\\'"
| 'n' -> Buffer.add_char buf '\n'
| 'r' -> Buffer.add_char buf '\r'
| 'n' ->
if literal_kind <> Literal_long_string then
Buffer.add_char buf '\n'
| 'r' ->
if literal_kind <> Literal_long_string then
Buffer.add_char buf '\r'
| 't' -> Buffer.add_char buf '\t'
| 'v' -> Buffer.add_char buf '\x0b'
| 'e' -> Buffer.add_char buf '\x1b'
| 'f' -> Buffer.add_char buf '\x0c'
| '\\' -> Buffer.add_char buf '\\'
| '$' -> Buffer.add_char buf '$'
| '`' ->
| '?' when literal_kind = Literal_long_string -> Buffer.add_char buf '\x3f'
| '$' when literal_kind <> Literal_long_string -> Buffer.add_char buf '$'
| '`' when literal_kind <> Literal_long_string ->
if literal_kind = Literal_backtick
then Buffer.add_char buf '`'
else Buffer.add_string buf "\\`"
| '\"' ->
if literal_kind = Literal_double_quote
if literal_kind = Literal_double_quote || literal_kind = Literal_long_string
then Buffer.add_char buf '\"'
else Buffer.add_string buf "\\\""
| 'u' when !idx < len && s.[!idx] = '{' ->
| 'u' when literal_kind <> Literal_long_string && !idx < len && s.[!idx] = '{' ->
let _ = next () in
let unicode_count = count_f (fun c -> c <> '}') ~max:6 0 in
let n = parse_int ("0x" ^ String.sub s (!idx) unicode_count) in
@@ -191,3 +202,6 @@ let unescape_single s =
let unescape_nowdoc s =
unescape_single_or_nowdoc ~is_nowdoc:true s
let unescape_long_string s =
unescape_literal Literal_long_string s

0 comments on commit e3fc98a

Please sign in to comment.