Feature/TG-494 Specialize Java Generic Methods #1460

chrisr-diffblue · 2017-10-10T13:19:09Z

No description provided.

thk123

This looks really good. The main blocking change is the dependency on TG-865."Nits" are suggestions (or formatting) so feel free to disregard if you think you have a good reason not to.

thk123 · 2017-10-11T09:54:15Z

src/java_bytecode/generate_java_generic_method.cpp

+  // as the specialized method we are about to create. By decorating the
+  // existing symbol, we ensure that no legal Java program could have already
+  // defined the symbol.
+  const std::string& signature_decoration=instantiation_decoration


Nit: break after the equals (or after the bracket)

thk123 · 2017-10-11T09:57:29Z

src/java_bytecode/generate_java_generic_method.cpp

+    (concrete_types);
+
+  // Check the method has not already been specialized
+  symbol_tablet::symbolst::const_iterator already_specialized=


Nit: As of this week(!) the symbol table's lookup method returns an optionalt (you might need to rebase to get this) which probably makes this code a little simpler:

symbol_tablet::opt_const_symbol_reft already_specialised=symbol_table.lookup(...) if(already_specialised) return already_specialised.value();

Actually, it looks like symbol_table.lookup has been further refactored even since then :-) so it now returns a symbolt*, which returns nullptr if the symbol isn't found. Point still stands though, and I'll update accordingly.

thk123 · 2017-10-11T10:01:02Z

src/java_bytecode/generate_java_generic_method.cpp

+    return already_specialized->second;
+
+  // Convert concrete types into a map for faster lookups
+  type_variable_instantiation_mapt concrete_type_map(concrete_types);


Two questions (that may become clear as I keep reading):

why not just accept as input a map?

faster lookup seems unlikely to have a big impact when I've never seen a class with more than 3 but I also suppose this will be more readable anway...

The problem with just accepting a Map as the input is the lack of ordering. At the moment we decorate the method identifier with a list of the concrete types in the order they are provided in the initlializer_list - this means that we get a consistent ordering of types in the identifier decoration. e.g. if we have

class Foo <A, B> { void someMethod(A a, B a) {...} }

then we pass in an intializer_list that looks something like {{A,Integer},{B,Double}} - which then gets used to decorate the method identifier with '<Integer, Double>' which matches the ordering of type parameters in the class definition. However, if we just passed in a Map<typevar,concretetype> - we don't know whether we'd end up with '<Integer,Double>' or '<Double,Integer>', depending on the implementation of Map...

Of course, perhaps the correct answer is to use a Map, but instead of constructing the decoration string from the Map, go look up the parent class (or type parameters for generic methods in non-generic classes) to find the ordering of the type parameters.

thk123 · 2017-10-11T10:03:52Z

src/java_bytecode/generate_java_generic_method.cpp

+
+  // Handle a generic return type
+  typet &method_return_type=specialized_code.return_type();
+  if(is_java_generic_parameter(method_return_type))


This is the case that a function returns T right?

Yep, thats right.

thk123 · 2017-10-11T10:04:13Z

src/java_bytecode/generate_java_generic_method.cpp

+      to_java_generic_parameter(method_return_type),
+      concrete_type_map);
+  }
+  else if(is_java_generic_type(method_return_type))


And this is the case of List<T> for example. Consider adding a comment to make this explicit

Good point - I'll add some comments to make this clearer.

thk123 · 2017-10-11T10:46:56Z

unit/java_bytecode/java_bytecode_specialize_generics/generics.java

+      return be.elem;
+  }
+
+  class double_element<A,B> {


Nit: could you auto-format the code as it is a bit hard to figure out what is going on

thk123 · 2017-10-11T10:48:16Z

unit/java_bytecode/java_bytecode_specialize_generics/generics.java

+  class double_element<A,B> {
+  	A first;
+  	B second;
+    java.util.Map<A,B> map;


I am concerned using the library classes like Map and List as I suppose it is just stubbing them which may not be exactly what we want.

Ah, good point - I'll update the tests to avoid standard container classes.

thk123 · 2017-10-11T10:52:48Z

unit/java_bytecode/java_bytecode_specialize_generics/java_bytecode_specialize_generics.cpp

+
+      THEN("Specialize a method with a generic parameter types")
+      {
+        REQUIRE(


I wonder if these "precondition" checks that are really checking the parsing works as you expect it to, should go in there own THEN block to make it clear they aren't part of the testing specialisation stuff

Yes, that probably makes sense - they act to document what the specialisation stuff is expecting as input, but that is, as you say, just testing the parsing is working.

thk123 · 2017-10-11T10:57:04Z

unit/java_bytecode/java_bytecode_specialize_generics/java_bytecode_specialize_generics.cpp

+            "java::generics$double_element.insert:"
+              "(Ljava/lang/Object;Ljava/lang/Object;)V");
+
+        const typet &symbol_type=generic_method_symbol.type;


Nit: a pattern I think might be good for us to use in our unit tests is something like this: https://github.com/diffblue/cbmc/blob/develop/unit/testing-utils/require_expr.cpp So for here you could do something like:

code_typet &require_code_type(const typet &type, optionalt<size_t> num_params, optional_t<typet &> return_type) { REQUIRE(type.id()==ID_code); const code_typet &code_type=to_code_type(type); if(num_params) REQUIRE(code_type.parameters().size()==num_params.value()); // maybe check return type? return code_type; }

If you think it is a good idea you might like to use it here (and put it in testing-utils/require_type)

That does look a neat way to cleanup the tests a bit - I'll have a play with that.

thk123 · 2017-10-11T10:59:28Z

unit/java_bytecode/java_bytecode_specialize_generics/java_bytecode_specialize_generics.cpp

+
+        // FIXME: Eventually want to replace instantiations of
+        // FIXME: generic types (e.g. List<Integer>) with references
+        // FIXME: to concrete instantiations of those once PR1406


If you could add a comment to the ticket to this effect as otherwise we might forget!

thk123

Looks good - blocking on the function signature/descriptor task and integrating Fotis's stuff into this.

A few formatting comments, they apply in more than one place.

thk123 · 2017-10-12T16:54:38Z

src/java_bytecode/generate_java_generic_method.cpp

+/// \param concrete_type_map The mapping from type variable to concrete types.
+void generate_java_generic_methodt::instantiate_generic_types(
+  typet &generic_type,
+  const type_variable_instantiation_mapt


Nit: but the variable name on same line:

const type_variable_instantiation_mapt &concrete_type_map)

thk123 · 2017-10-12T16:57:50Z

src/java_bytecode/generate_java_generic_method.cpp

+  const type_variable_instantiation_mapt
+    &concrete_types)
+{
+  INVARIANT(generic_parameter.id()==ID_pointer,


Formatting: break after (

thk123 · 2017-10-12T16:58:32Z

src/java_bytecode/generate_java_generic_method.cpp

+  INVARIANT(generic_parameter.subtype().id()==ID_symbol,
+            "All generic parameters should point to symbols");
+
+  const symbol_typet &generic_type_variable=generic_parameter


Formatting: break after the .

thk123 · 2017-10-12T16:58:46Z

src/java_bytecode/generate_java_generic_method.cpp

+
+  const symbol_typet &generic_type_variable=generic_parameter
+    .type_variable();
+  const auto &instantiation_type=concrete_types.find


Formatting: break after the ( or the =

thk123 · 2017-10-12T17:01:25Z

unit/java_bytecode/java_bytecode_specialize_generics/generics.java

+      second=b;
+    }
+
+    void setMap(java.util.Map<A,B> m) {


Outstanding: don't use java.util data structures

thk123 · 2017-10-12T17:05:01Z

unit/java_bytecode/java_bytecode_specialize_generics/java_bytecode_specialize_generics.cpp

+
+  GIVEN("Some class files with Generics")
+  {
+    WHEN("Methods with generic signatures")


Nit: I'd prefer if this was more specific WHEN("Generic method returning the generic type")

thk123 · 2017-10-12T17:06:38Z

unit/java_bytecode/java_bytecode_specialize_generics/java_bytecode_specialize_generics.cpp

+            new_code.return_type(), {"java::java.lang.Integer"});
+        }
+      }
+


I think it would be good to use a WHEN("Parsing a method that has a generic parameter")

thk123 · 2017-10-12T17:07:16Z

unit/java_bytecode/java_bytecode_specialize_generics/java_bytecode_specialize_generics.cpp

+        }
+      }
+
+      {


Ditto WHEN("Parsing a method with two generic parameters")

thk123 · 2017-10-12T17:14:19Z

unit/testing-utils/require_type.cpp

+    REQUIRE(generic_type_vars.size()==type_variables.value().size());
+
+    size_t i=0;
+    for(irep_idt type_variable_identifier : type_variables.value())


I think you can use something like:

REQUIRE( std::equal( type_variables->begin(), type_variables->end(), generic_type_vars.begin(), [](const irep_idt &param_name, const generic_type_parametert &param) { REQUIRE(!is_java_generic_inst_parameter(param)); return param.type_variable().get_identifier()==param_name; }));

to make it more clear(?)

thk123 · 2017-10-12T17:20:14Z

src/java_bytecode/generate_java_generic_method.cpp

+{
+  INVARIANT(generic_parameter.id()==ID_pointer,
+            "All generic parameters should be pointers in java");
+  INVARIANT(generic_parameter.subtype().id()==ID_symbol,


Let's discuss this tomorrow - I worry updating this as suggested to ns.follow(generic_type.subtype()).id()==ID_struct will break your unit tests

…n instantiated

This reverts commit be94ecdd2caf2bf5193c2cd2ced1b1ae19ce22d9.

thk123 · 2017-10-17T16:15:35Z

It appears this work will not be needed for the immediate generic support under development so I am closing this PR for now. Sorry again and thanks for the effort Chris.

chrisr-diffblue force-pushed the feature/TG494-generic-method-specialization branch from 2d731e7 to 9de05fe Compare October 10, 2017 14:16

thk123 suggested changes Oct 11, 2017

View reviewed changes

chrisr-diffblue force-pushed the feature/TG494-generic-method-specialization branch from 9de05fe to 3eca6d9 Compare October 11, 2017 16:31

thk123 suggested changes Oct 12, 2017

View reviewed changes

thk123 requested a review from mgudemann October 12, 2017 17:23

chrisr-diffblue force-pushed the feature/TG494-generic-method-specialization branch 4 times, most recently from cd1fb9c to ad82f9f Compare October 16, 2017 09:44

chrisr-diffblue added 3 commits October 16, 2017 11:05

TG-949: feature/specialize generic methods with concrete types

cb2261f

When instantiating a generic type, first check if it's previously bee…

008563b

…n instantiated

[DO NOT MERGE] Revert "Reverting method descriptor loading"

e16286a

This reverts commit be94ecdd2caf2bf5193c2cd2ced1b1ae19ce22d9.

chrisr-diffblue force-pushed the feature/TG494-generic-method-specialization branch from ad82f9f to e16286a Compare October 16, 2017 10:05

Ensure 'this' parameter has a specialized type

092d161

thk123 added the do not merge label Oct 16, 2017

thk123 closed this Oct 17, 2017

chrisr-diffblue mentioned this pull request Nov 1, 2017

Add some unit test helper functions, useful for Java generics unit tests #1550

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/TG-494 Specialize Java Generic Methods #1460

Feature/TG-494 Specialize Java Generic Methods #1460

chrisr-diffblue commented Oct 10, 2017

thk123 left a comment

thk123 Oct 11, 2017

thk123 Oct 11, 2017

chrisr-diffblue Oct 11, 2017 •

edited

Loading

thk123 Oct 11, 2017

chrisr-diffblue Oct 11, 2017

thk123 Oct 11, 2017

chrisr-diffblue Oct 11, 2017

thk123 Oct 11, 2017

chrisr-diffblue Oct 11, 2017

thk123 Oct 11, 2017

thk123 Oct 11, 2017

chrisr-diffblue Oct 11, 2017

thk123 Oct 11, 2017

chrisr-diffblue Oct 11, 2017

thk123 Oct 11, 2017

chrisr-diffblue Oct 11, 2017

thk123 Oct 11, 2017

thk123 left a comment

thk123 Oct 12, 2017

thk123 Oct 12, 2017

thk123 Oct 12, 2017

thk123 Oct 12, 2017

thk123 Oct 12, 2017

thk123 Oct 12, 2017

thk123 Oct 12, 2017

thk123 Oct 12, 2017

thk123 Oct 12, 2017

thk123 Oct 12, 2017

thk123 commented Oct 17, 2017

+                      }
+                    }
+                    {

Feature/TG-494 Specialize Java Generic Methods #1460

Feature/TG-494 Specialize Java Generic Methods #1460

Conversation

chrisr-diffblue commented Oct 10, 2017

thk123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisr-diffblue Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thk123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thk123 commented Oct 17, 2017

chrisr-diffblue Oct 11, 2017 •

edited

Loading