From c0b9617db3863769f2defb580278f94f8a1044a8 Mon Sep 17 00:00:00 2001 From: Mitya Lyubarskiy Date: Thu, 7 May 2020 06:36:57 -0700 Subject: [PATCH] [nullsafe] More consistent version of anonymous classes in JavaClassName Summary: There is a case where the class name is "A$1$B$2". In this case, we would correctly say it is an anonymous class, and return A$1$B as user defined, which is bad: now we can get the outer class which will be A$1 that will be anonymous again. This was leading to tricky bugs. Now, get_user_defined_class name will return something that is indeeed contains only user defined names. There is one tricky pathological case left: when the outermost class is anonymous, which can in theory occur. This might lead to other tricky bugs, so the follow up will be to rewrite API of JavaClassName.t so that this case is made clear for the client. But lets unbreak nullsafe fore now first. Reviewed By: ngorogiannis Differential Revision: D21449686 fbshipit-source-id: b0ba4702e --- infer/src/IR/JavaClassName.ml | 53 +++++++++++++--------------- infer/src/unit/JavaClassNameTests.ml | 24 +++++++------ 2 files changed, 38 insertions(+), 39 deletions(-) diff --git a/infer/src/IR/JavaClassName.ml b/infer/src/IR/JavaClassName.ml index 1c4428f66..be7672891 100644 --- a/infer/src/IR/JavaClassName.ml +++ b/infer/src/IR/JavaClassName.ml @@ -67,31 +67,6 @@ let get_outer_class_name {package; classname} = String.rsplit2 classname ~on:'$' |> Option.map ~f:(fun (outer, _) -> {package; classname= outer}) -(* Strips $ suffixes from the class name, and return how many were stripped *) -let strip_anonymous_suffixes_if_present classname = - let rec strip_recursively classname nesting_level = - match String.rsplit2 classname ~on:'$' with - | Some (outer, suffix) when is_int suffix -> - (* Suffix is an integer - that was an anonymous class. - But it could be nested inside another anonymous class as well *) - strip_recursively outer (nesting_level + 1) - | _ -> - (* Suffix is not an integer or not present - not an anonymous class *) - (classname, nesting_level) - in - strip_recursively classname 0 - - -(* Strips everything after $Lambda$ (if it is a lambda-class), - and returns the result string together with if it was stripped *) -let strip_lambda_if_present classname = - match String.substr_index classname ~pattern:"$Lambda$" with - | Some index -> - (String.prefix classname index, true) - | None -> - (classname, false) - - (* Anonymous classes have two forms: - classic anonymous classes: suffixes in form of $. @@ -101,10 +76,30 @@ let strip_lambda_if_present classname = In general case anonymous class name looks something like Class$NestedClass$1$17$5$Lambda$_1_2, and we need to return Class$NestedClass *) let get_user_defined_class_if_anonymous_inner {package; classname} = - let without_lambda, was_lambda_stripped = strip_lambda_if_present classname in - let outer_class_name, nesting_level = strip_anonymous_suffixes_if_present without_lambda in - let was_stripped = was_lambda_stripped || nesting_level > 0 in - if was_stripped then Some {package; classname= outer_class_name} else None + let is_anonymous_name = function + | "Lambda" -> + true + | name when is_int name -> + true + | _ -> + false + in + let pieces = String.split classname ~on:'$' in + let first_anonymous_name = List.findi pieces ~f:(fun _index name -> is_anonymous_name name) in + Option.bind first_anonymous_name ~f:(fun (index, _name) -> + (* Everything before this index did not have anonymous prefixes. Deem it user defined. *) + match List.take pieces index with + | [] -> + (* This is a weird situation - our class _starts_ with an anonymous name. + This should not happen normally, but we can not rule this out completely since there is no physical limitations on + the bytecode names. + In this case, we return [None] because formally this is not an anonymous _inner_ class, but anonymous outermost class instead. + TODO: redesign this API so this case is modelled directly + *) + None + | list -> + (* Assemble back all pieces together *) + Some {package; classname= String.concat ~sep:"$" list} ) let is_anonymous_inner_class_name t = get_user_defined_class_if_anonymous_inner t |> is_some diff --git a/infer/src/unit/JavaClassNameTests.ml b/infer/src/unit/JavaClassNameTests.ml index f0252444c..d8aa215b4 100644 --- a/infer/src/unit/JavaClassNameTests.ml +++ b/infer/src/unit/JavaClassNameTests.ml @@ -80,23 +80,27 @@ let test_anonymous = |> assert_some |> assert_equal_to ~expected_package:(Some "some.package") ~expected_classname:"SomeClass$NestedClass$AgainNestedClass" ; - (* If it is a lambda class, we expect this to be detected *) + (* Some name inside of anonymous - this is still anonymous *) get_user_defined_class_if_anonymous_inner - (make ~package:(Some "some.package") ~classname:"SomeClass$Lambda$_4_1") + (make ~package:(Some "some.package") ~classname:"SomeClass$1$SomeName") |> assert_some |> assert_equal_to ~expected_package:(Some "some.package") ~expected_classname:"SomeClass" ; - (* Lambda might be inside anonymous (or several ones in general case) *) + (* Some names inside anonymous classes - should still return the innermost user defined *) get_user_defined_class_if_anonymous_inner - (make ~package:(Some "some.package") ~classname:"SomeClass$1$7$Lambda$_4_1") + (make ~package:(Some "some.package") ~classname:"A$B$1$C$2") |> assert_some - |> assert_equal_to ~expected_package:(Some "some.package") ~expected_classname:"SomeClass" ; - (* The most general case: nested class, lambda, and anonymous mixed *) + |> assert_equal_to ~expected_package:(Some "some.package") ~expected_classname:"A$B" ; + (* Pathological case - this is anonymous class on the outer level. Should return None because it is not inner. *) + get_user_defined_class_if_anonymous_inner (make ~package:(Some "some.package") ~classname:"23") + |> assert_none ; + (* Pathological case - this is anonymous class on the outer level. Should return None because it is not inner. *) + get_user_defined_class_if_anonymous_inner (make ~package:(Some "some.package") ~classname:"1$A") + |> assert_none ; + (* If it is a lambda class, we expect this to be detected *) get_user_defined_class_if_anonymous_inner - (make ~package:(Some "some.package") - ~classname:"SomeClass$NestedClass$7$1$Lambda$_4_1$2$Lambda$_7_18$19$16") + (make ~package:(Some "some.package") ~classname:"SomeClass$Lambda$_4_1") |> assert_some - |> assert_equal_to ~expected_package:(Some "some.package") - ~expected_classname:"SomeClass$NestedClass" ; + |> assert_equal_to ~expected_package:(Some "some.package") ~expected_classname:"SomeClass" ; (* If package was empty, everything should still work *) get_user_defined_class_if_anonymous_inner (make ~package:None ~classname:"SomeClass$NestedClass$AgainNestedClass$17$23$1")