Extract generics by owen-mc · Pull Request #686 · github/codeql-go

owen-mc · 2022-02-09T12:06:22Z

Current status (updated 2022-04-12):

I think this is complete except for performance testing.

Note that for the moment we are ignoring instantiated types, and treating them as the uninstantiated type.

smowton

Looking good so far! Worth considering for the future: do we want to extract the same thing for functionName[param, param...] and for typeName[param, param, ...]? Which one will be harder: distinguishing the cases in the extractor, or in QL?

smowton · 2022-02-10T17:59:19Z

@@ -0,0 +1,53 @@
+// // First we need to wrap some database types
+// class Location extends @location {


Remove, uncomment, or note why this is left here commented

Oh yes. These were from the scripts that I copied, but I don't think it's needed. I meant to test out doing an upgrade and a downgrade, and if they work I will delete it.

owen-mc · 2022-04-01T16:06:15Z

There seem to be some non-trivial test failures relating to data flow which I'll look into on Monday.

smowton

Reviewed up to overrideTypesOfTypeSetLiterals

smowton · 2022-04-01T16:32:44Z

+		populateTypeParamParents(tw, meth.Type().(*types.Signature).TypeParams(), methlbl)
+		populateTypeParamParents(tw, meth.Type().(*types.Signature).RecvTypeParams(), methlbl)


Add a comment (in both places) clarifying what's going on with this vs. the very similar calls in extractObjects

smowton · 2022-04-01T16:34:36Z


-// IndexExpr is the type of index expression AST nodes
+// IndexExpr is the type of AST nodes for index expressions and generic type
+// instantiation expressions with one type argument


Suggested change

// instantiation expressions with one type argument

// instantiation expressions with one type argument

// Note that syntactically unambiguous generic instantiations will be extracted

// as one of the Generic...InstantiationExpr expression kinds below.

smowton · 2022-04-01T16:35:11Z


+// GenericFunctionInstantiationExpr is the type of AST nodes that represent a instantiation
+// of a generic type. These correspond to some index expression AST nodes and all index
+// list expression AST nodes.


Suggested change

// list expression AST nodes.

// list expression AST nodes.

// Note some syntactically ambiguous instantations are extracted as an `IndexExpr` to be disambiguated in QL later.

Actually function instantiations can be distinguished in the extractor.

smowton · 2022-04-01T16:35:20Z

+
+// GenericTypeInstantiationExpr is the type of AST nodes that represent an instantiation
+// of a generic type. These correspond to some index expression AST nodes and all index
+// list expression AST nodes.


Suggested change

// list expression AST nodes.

// list expression AST nodes.

// Note some syntactically ambiguous instantations are extracted as an `IndexExpr` to be disambiguated in QL later.

smowton · 2022-04-01T16:42:54Z

+			n := flattenBinaryExprTree(tw, expr.X, lbl, 0)
+			flattenBinaryExprTree(tw, expr.Y, lbl, n)


Is this the same as flattenBinaryExprTree(expr)?

smowton · 2022-04-01T16:44:08Z

+// Go through a `FieldList` and update the types of all type set literals which
+// are not already union types to be union types. We do this by changing the
+// types stored in `tw.Package.TypesInfo.Types`.
+func overrideTypesOfTypeSetLiterals(tw *trap.Writer, fields *ast.FieldList) {


Suggested change

func overrideTypesOfTypeSetLiterals(tw *trap.Writer, fields *ast.FieldList) {

func makeTypeSetLiteralsUnionTyped(tw *trap.Writer, fields *ast.FieldList) {

Probably add a comment at the use sites explaining the reasoning why we're confident we don't extract a type-set literal prior to this override being applied.

smowton

Suggested tests to add: some basic dataflow tests to make sure we can see flow into and out of an instantiation of a generic function / method and/or via the fields of a generic type.

We should also add some basic control-flow tests to make sure our new expr kinds aren't being picked up and incorporated into the CFG leading to potential dead-ends.

smowton · 2022-04-06T16:41:48Z

+      u instanceof InterfaceType and
+      (
+        not u instanceof BasicInterfaceType and
+        if exists(u.(InterfaceType).getAnEmbeddedTypeSetLiteral())
+        then
+          forall(Type intersectionType |
+            intersectionType = u.(InterfaceType).getAnEmbeddedTypeSetLiteral().getATerm().getType() and
+            forall(TypeSetLiteralType tslit |
+              tslit = u.(InterfaceType).getAnEmbeddedTypeSetLiteral()
+            |
+              intersectionType = tslit.getATerm().getType()
+            )
+          |
+            intersectionType.implementsComparable()
+          )
+        else u.(InterfaceType).isOrEmbedsComparable()
+      )
+    )


Neaten this with exists(InterfaceType uif | uif = u | ... to eliminate all the u.(InterfaceType)

smowton · 2022-04-06T16:44:55Z

+            forall(TypeSetLiteralType tslit |
+              tslit = u.(InterfaceType).getAnEmbeddedTypeSetLiteral()
+            |
+              intersectionType = tslit.getATerm().getType()
+            )


Add a comment explaining what this is doing? I don't follow what the nested forall seeks to achieve.

All types in the intersection of all the embedded type set literals must implement comparable.

By "add a comment" (here and elsewhere) I mean add a comment in the code for future readers' benefit, not just in the PR

I did add it in the code. I just copied it here so you could see the wording easily without having to hunt it down in the updated PR.

Doh right sorry, I clicked through and surprisingly Github took me to the code before your change.

Looking again, based on the General Interfaces bit of https://go.dev/ref/spec#Interface_types I'm not sure this definition of the intersection is complete? Based on that spec, are you sure we don't need to consider method specifications here for example, to restrict to only those types with the required method defined on them?

smowton · 2022-04-06T16:49:41Z

  override string toString() { result = "pointer type" }
 }

+private newtype TTerm =


As a publicly exposed name this is a bit non-specific (let the record show I avoided calling it 'generic'). How about TypeSetTerm?

smowton · 2022-04-06T16:51:02Z

+  Type getType() { result = tp }
+
+  /** Holds if `t` is in the type set of this term. */
+  predicate hasInTypeSet(Type t) { if tilde = false then t = tp else t.getUnderlyingType() = tp }


matchesType or includesType perhaps? I'm thinking hasInTypeSet would apply to a type-set not an individual term

smowton · 2022-04-06T16:57:26Z

+  /**
+   * Holds if `tp` is an explicitly embedded type with index `index`.
+   *
+   * `tp` is either a type set literal type or its underlyign type is an


Suggested change

* `tp` is either a type set literal type or its underlyign type is an

* `tp` is either a type set literal type or its underlying type is an

Also confirm, are you saying tp can't directly be an interface type? If not (i.e. this is short for 'is type-set-literal OR is interface OR underlying is interface') then perhaps abbreviate this to

if tp (or its underlying type) is a type-set literal or a non-basic interface -- guessing from the context that having an underlying which is a type-set literal is impossible, but that's ok

It turns out it can be an interface type, if you do something like type t interface{ interface{ Read(p []byte) (n int, err error } }. There is also some confusion caused by the tension between the fact that it is standard to refer to types like Reader as interfaces, when they are in fact named types whose underlying type is an interface. So your suggested wording works.

smowton · 2022-04-06T17:07:02Z

+   * Note that the methods of the embedded interface are already considered
+   * as part of the method set of this interface.


Not immediately clear what this has to do with this predicate?

I want to make it clear that even though we are giving the user a way to access an embedded interface, they do not need to do that to access methods on this interface which come from an embedded interface, because we have already processed them so that they are considered methods on this interface.

smowton · 2022-04-06T17:07:39Z

+   * Note that the methods of the embedded interface are already considered
+   * as part of the method set of this interface.
+   */
+  Type getAnExplicitlyEmbeddedInterface() {


Sounds like this might be better called directly rather than explicitly embedded?

smowton · 2022-04-06T17:08:42Z

+   * Gets the type set literal with index `index` from the definition of this
+   * interface type.
+   *
+   * Note that the indexing includes embedded interfaces but not methods.


Are you saying the indices might be non-contiguous? If so just say that.

smowton · 2022-04-06T17:11:16Z

 #   20|         0: [CallExpr] call to Println
 #   20|             Type = (int, error)
 #   20|           0: [FunctionName, SelectorExpr] selection of Println
-#   20|               Type = func([]interface {  }) int, error


Can we contrive to avoid this change to eliminate lots of churn?

Would you be okay with the churn if it was in a separate PR? I would prefer to get rid of the double space, though I can see that it is not necessary to do it at the same time as all these other changes.

Sure that's fine

I've done it in a separate PR and rebased this branch on top of it. When I next force-push the churn will be reduced.

smowton · 2022-04-07T15:50:48Z

No further comments from me -- I don't thoroughly understand the ins and outs of the nightmare type system the Golang authors have created, but this seems like we could incrementally fix any mistakes in the event we've both misunderstood.

I'm a bit worried about the performance of the doubly-nested forall describing "for all types that are present in the type-sets of all elements of this interface", but I guess we'll only find out about that when we get some nontrivial generic projects in the wild to test this out against.

smowton · 2022-04-07T15:51:15Z

Testing steps:

[ ] DCA
[ ] LGTM dist build

owen-mc · 2022-04-08T14:27:01Z

@smowton If it makes you feel better about the doubly-nested forall, there isn't really any reason to ever have more than one type set literal in an interface, so it will almost always boil down to checking all the terms in term union.

lgtm-com · 2022-04-27T15:41:44Z

This pull request introduces 1 alert when merging 17fc38a into b8165d4 - view on LGTM.com

new alerts:

1 for Unreachable statement

lgtm-com · 2022-04-29T22:15:55Z

This pull request introduces 1 alert when merging c84ef3d into ee94eb5 - view on LGTM.com

new alerts:

1 for Unreachable statement

lgtm-com · 2022-05-03T12:22:00Z

This pull request introduces 1 alert when merging 2959803 into ee94eb5 - view on LGTM.com

new alerts:

1 for Unreachable statement

lgtm-com · 2022-05-10T13:48:35Z

This pull request introduces 1 alert when merging dfce34b into ef7363c - view on LGTM.com

new alerts:

1 for Unreachable statement

The tests which involve a flow through a receiver with a non-trivial access path currently don't give the right result. This should be fixed in a follow-up issue.

"type declarations inside generic functions are not currently supported"

lgtm-com · 2022-05-10T13:55:42Z

This pull request introduces 1 alert when merging c0fbd03 into ef7363c - view on LGTM.com

new alerts:

1 for Unreachable statement

smowton · 2022-05-11T09:14:26Z

DCA and distribution test both suggest this at least harmless. One of the tests has some MISSING: expectations which all relate to taking pointers to generic methods: to check whether this is particular to generics or a general shortcoming of Go's modelling of function pointers. However given this appears at least harmless I will merge this and follow up investigating the function pointer problems.

As of github/codeql-go#686 landing we support extracting generics, dataflow analysis in programs that use generics, etc. Note this hasn't gone out in a release yet but I would expect it to be in 2.9.2.

owen-mc force-pushed the extract-generics branch from 0db4ec8 to 7b099ef Compare February 10, 2022 16:19

smowton reviewed Feb 10, 2022

View reviewed changes

owen-mc force-pushed the extract-generics branch from 0bac20b to 2c2b64d Compare February 23, 2022 12:14

owen-mc force-pushed the extract-generics branch from 7a959b4 to 360f244 Compare March 2, 2022 16:54

owen-mc force-pushed the extract-generics branch 2 times, most recently from 164d49f to e0f8c68 Compare March 31, 2022 15:59

owen-mc marked this pull request as ready for review March 31, 2022 16:00

owen-mc requested a review from a team as a code owner March 31, 2022 16:00

owen-mc force-pushed the extract-generics branch from e0f8c68 to c0e6bb6 Compare April 1, 2022 13:27

smowton reviewed Apr 1, 2022

View reviewed changes

owen-mc force-pushed the extract-generics branch 2 times, most recently from bde455b to 4181beb Compare April 4, 2022 23:22

smowton reviewed Apr 6, 2022

View reviewed changes

owen-mc force-pushed the extract-generics branch 3 times, most recently from 8e3c70e to c23a496 Compare April 11, 2022 20:32

owen-mc force-pushed the extract-generics branch from 3287623 to 95c5077 Compare April 21, 2022 21:34

owen-mc mentioned this pull request May 24, 2022

Flow with non-trivial access path through receiver of method assigned to a variable is missed github/codeql#9296

Closed

owen-mc force-pushed the extract-generics branch from 1fc8554 to 17fc38a Compare April 27, 2022 15:38

owen-mc force-pushed the extract-generics branch from 17fc38a to c84ef3d Compare April 29, 2022 22:12

owen-mc force-pushed the extract-generics branch from c84ef3d to 2959803 Compare May 3, 2022 12:18

Use Go 1.18

7f0a379

owen-mc and others added 22 commits May 10, 2022 14:52

Update comment above first extraction of packages

4e71ab5

Add data flow tests for generic structs, methods and functions

7a7ca61

CallExpr.getCalleeExpr should get uninstantiated function

ce9c9cf

generic function instantions aren't type exprs

26d4acd

Add control flow test

5257c4a

Add missing this. to member predicate calls

20562cb

Improve comment

a8a351f

Update CodeQL tests to go 1.18.1

0dee215

Control flow for generic function instantiations

3790c4e

Find callee through function instantiation

4a9aeac

Test calls through variables

ba147e8

The tests which involve a flow through a receiver with a non-trivial access path currently don't give the right result. This should be fixed in a follow-up issue.

Fix extractor crash when missing type information

2e8b9a9

Fix panic when type is unknown

06d1398

Fix another place where type could be nil

aa62fab

Remove invalid code in test

7f1f428

"type declarations inside generic functions are not currently supported"

Test calling generic functions from other files

8477053

Make TypeParamParent map global

ac081dc

Reintroduce noinlined predicate to fix performance

d098bdc

Fix type aliases for instantiated generic types

3c4b520

Make objects-map-changed warning non-fatal

d330033

Remove commented-out debug code

3129c3d

Add qldoc for getTermInIntersection

c0fbd03

smowton force-pushed the extract-generics branch from dfce34b to c0fbd03 Compare May 10, 2022 13:52

smowton approved these changes May 11, 2022

View reviewed changes

smowton merged commit 440b311 into github:main May 11, 2022

smowton mentioned this pull request May 11, 2022

Claim Go 1.18 support github/codeql#9113

Merged

owen-mc deleted the extract-generics branch May 12, 2022 06:10

		@@ -0,0 +1,53 @@
		// // First we need to wrap some database types
		// class Location extends @location {

		populateTypeParamParents(tw, meth.Type().(*types.Signature).TypeParams(), methlbl)
		populateTypeParamParents(tw, meth.Type().(*types.Signature).RecvTypeParams(), methlbl)

	// list expression AST nodes.
	// list expression AST nodes.
	// Note some syntactically ambiguous instantations are extracted as an `IndexExpr` to be disambiguated in QL later.

		n := flattenBinaryExprTree(tw, expr.X, lbl, 0)
		flattenBinaryExprTree(tw, expr.Y, lbl, n)

	func overrideTypesOfTypeSetLiterals(tw trap.Writer, fields ast.FieldList) {
	func makeTypeSetLiteralsUnionTyped(tw trap.Writer, fields ast.FieldList) {

	* `tp` is either a type set literal type or its underlyign type is an
	* `tp` is either a type set literal type or its underlying type is an

		* Note that the methods of the embedded interface are already considered
		* as part of the method set of this interface.

Conversation

owen-mc commented Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smowton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

owen-mc commented Apr 1, 2022

Uh oh!

smowton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smowton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smowton commented Apr 7, 2022

Uh oh!

smowton commented Apr 7, 2022

Uh oh!

owen-mc commented Apr 8, 2022

Uh oh!

owen-mc commented Feb 9, 2022 •

edited

Loading

smowton commented May 11, 2022 •

edited

Loading