[TG-1301] Parsing nested generics

[majakusber]

The first commit contains the implementation of this, the summary of changes is in the commit message. @thk123 and @mgudemann can you please review?

[smowton]

Could you (a) avoid restyling lines that are otherwise untouched, and (b) do the mass-renaming of parameter -> type variable in a separate commit from the actual substantial changes here? That will make it much easier to review as (a) will reduce the line count a lot and (b) will mean 90% of lines can be skimmed as they're just a find/replace, whilst the remaining 10% are the ones that really need reading carefully.

[majakusber]

@smowton Will do, thanks.

[thk123]

In future it would be helpful to ensure unrelated formatting changes are excluded or in a separate commit. It would also be useful for renamings to be done in isolated commits.

Other than that - code and tests look good. If you've not already done so - could you create a Test Gen pointer bump to verify this doesn't break any TG tests

[thk123]

Is this needed?

[majakusber]

Good point. The whole if block is actually not needed here, type is of type typet and we just need to make sure it is a reference_typet but we don't need to cast it to any of its subtypes (it will be cast back to reference when returned). Will make the change.

[thk123]

Removed as duplicate?

[smowton]

Changes look fine now. I'd carefully consider the right terminology before making a commit that changes it everywhere, though. In my understanding a function f(x, y, z) called with f(1, z, w + v) has formal parameters x, y, z instantiated by actual parameters 1, z, w + v. Thus in the context List<T, Integer> I would be tempted to say T is a formal type parameter or type variable, while Integer is an actual type parameter or simply a concrete type. However others may disagree -- I suggest quizzing wiser language analysis people such as @peterschrammel or @forejtv to check you're using a naming scheme that will agree with a new developer who has read "industry standard" terminology before.

[tautschnig]

I'd agree with what @smowton just said, though would add that elsewhere in the code base the terminology is "parameter" to refer to formal parameters, and "argument" to refer to actual parameters.

[mgudemann]

LGTM, modulo small comment and an eventual test-gen bump to see how this works there

[mgudemann]

might make sense to specify that it will also parse method signatures which are wrapped in (, ) brackets

[majakusber]

The beginning of the sentence says 'This is used for parsing the parameters of a function or...', would it be enough to change it to 'This is used for parsing the parameters of a function contained within parentheses or..' or did you mean something else?

[majakusber]

Thanks for the comments everyone. About the mixed commit, sorry about that, I understand it's not easy to locate the crucial parts. I tried to brake it into two commits, renaming and then functionality, after Chris's comment. However, it's quite tricky as renaming for consistency cannot really be done without changing the semantics of the java_generic_parametert class (see below for explanation).

For the choice of terminology, I was keeping two things in mind: minimal changes to existing terminology we use in the code base, and the terminology from Java spec:
https://docs.oracle.com/javase/tutorial/java/generics/types.html
In Java spec, type variables and type parameters refer to the same thing - uninstantiated types such as T, although type parameter is used more often than type variable. Once invoked/instantiated, they are called type arguments. In our code base, we used type variables to refer to anything appearing between the angle brackets, see generic_type_variablest in java_generic_typet. The type of these was a vector of java_generic_parametert (uninstantiated), which had a subclass java_generic_inst_parametert (instantiated). While this class hierarchy was quite ok for terminology, it prevented the instantiated types to be generic types (exactly what we need to parse nested generics). So the special class to hold instantiated type variables was removed, the type of the generic_type_variablest in java_generic_typet changed to reference_typet (allows concrete types such as Integer, generic types and type parameters). This allowed to keep the terminology in the code base almost the same and in accordance with Java spec: type variables for mixed types, parameters for uninstantiated type variables. Much of the renaming was only to make sure that anything that can be both uninstantiated and instantiated type is called type variable rather than parameter.

Note that no change was needed for classes, java_generic_class_typet, since in a declaration of a generic class only (uninstantiated) type parameters appear in the angle brackets.

Also, note that the terminology for generics is separate from terminology for functions - in functions, parameters refer to inputs.

I hope this makes it a bit more clear, I'm happy to make changes to the terminology if something else would make more sense.

[peterschrammel]

@svorenova, please rebase.

[majakusber]

As requested by Chris in our offline discussion, the terminology has been slightly adjusted as follows:

• type variables and type parameters are synonyms, referring to uninstantiated types, e.g., T in generic class declaration MyClass<T>,
• type arguments refer to instantiates types, that means to anything appearing within the angle brackets for objects of generic types, e.g., if the class MyClass<T> has a field of type HashMap<T,Integer>, then both T and Integer are arguments (T must already be instantiated for the class).
  This is now fully in accordance with Java spec.

@smowton @peterschrammel Can you please take a look again?