diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..dc3b3a5 --- /dev/null +++ b/.gitignore @@ -0,0 +1 @@ +testdata/pkg/* diff --git a/go.mod b/go.mod index ffe1d7c..b71e974 100644 --- a/go.mod +++ b/go.mod @@ -7,6 +7,7 @@ require ( github.com/google/go-cmp v0.3.1 github.com/pkg/errors v0.8.1 github.com/stretchr/testify v1.3.0 - golang.org/x/tools v0.0.0-20190826060629-95c3470cfb70 + golang.org/x/tools v0.0.0-20190918214516-5a1a30219888 + golang.org/x/tools/gopls v0.1.7 // indirect honnef.co/go/tools v0.0.1-2019.2.3 ) diff --git a/go.sum b/go.sum index a6c1a87..c1d8609 100644 --- a/go.sum +++ b/go.sum @@ -26,6 +26,7 @@ golang.org/x/mod v0.0.0-20190513183733-4bf6d317e70e/go.mod h1:mXi4GBBbnImb6dmsKG golang.org/x/net v0.0.0-20190311183353-d8887717615a/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg= golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg= golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= +golang.org/x/sync v0.0.0-20190423024810-112230192c58 h1:8gQV6CLnAEikrhgkHFbMAEhagSSnXWGV915qUMm9mrU= golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= @@ -34,6 +35,12 @@ golang.org/x/tools v0.0.0-20190425150028-36563e24a262/go.mod h1:RgjU9mgBXZiqYHBn golang.org/x/tools v0.0.0-20190621195816-6e04913cbbac/go.mod h1:/rFqwRUd4F7ZHNgwSSTFct+R/Kf4OFW1sUzUTQQTgfc= golang.org/x/tools v0.0.0-20190826060629-95c3470cfb70 h1:Cde4MUY6o3RgxpWu8/7xCjEW7SE22me73ix+NIXtV7s= golang.org/x/tools v0.0.0-20190826060629-95c3470cfb70/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= +golang.org/x/tools v0.0.0-20190918214516-5a1a30219888 h1:ER45Jz0UDQ3e6em1lwXVwuPf96lvyQogb7m+gEbsoPg= +golang.org/x/tools v0.0.0-20190918214516-5a1a30219888/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= +golang.org/x/tools v0.0.0-20191014205221-18e3458ac98b h1:EsQHTYgcM562dq02r6y2Yt9VpvvLNIyNECx96XQeolA= +golang.org/x/tools/gopls v0.1.7 h1:YwKf8t9h69++qCtVmc2q6fVuetFXmmu9LKoPMYLZid4= +golang.org/x/tools/gopls v0.1.7/go.mod h1:PE3vTwT0ejw3a2L2fFgSJkxlEbA8Slbk+Lsy9hTmbG8= +golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7 h1:9zdDQZ7Thm29KFXgAX/+yaf3eVbP7djjWp/dXAppNCc= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/errgo.v2 v2.1.0/go.mod h1:hNsd1EY+bozCKY1Ytp96fpM3vjJbqLJn88ws8XvfDNI= diff --git a/ider.go b/ider.go index 9fc90ff..1547e55 100644 --- a/ider.go +++ b/ider.go @@ -2,20 +2,21 @@ package splitfile import ( "fmt" - "go/token" "go/types" ) type Ider interface { Pkg() *types.Package Name() string - Pos() token.Pos + Type() types.Type } func Id(ider Ider) string { + id := fmt.Sprintf("%s %s", ider.Name(), ider.Type().String()) + if ider.Pkg() != nil { - return fmt.Sprintf("%s %s %d", ider.Pkg().String(), ider.Name(), ider.Pos()) + id = fmt.Sprintf("%s %s", ider.Pkg().String(), id) } - return fmt.Sprintf("%s %d", ider.Name(), ider.Pos()) + return id } diff --git a/ider_test.go b/ider_test.go index eeee959..61c51a9 100644 --- a/ider_test.go +++ b/ider_test.go @@ -1,20 +1,26 @@ package splitfile import ( - "go/token" "go/types" "testing" ) +type mockType struct { + s string +} + +func (mt mockType) Underlying() types.Type { return nil } +func (mt mockType) String() string { return mt.s } + type mockObject struct { pkgFn func() *types.Package nameFn func() string - posFn func() token.Pos + typeFn func() types.Type } func (mo mockObject) Pkg() *types.Package { return mo.pkgFn() } func (mo mockObject) Name() string { return mo.nameFn() } -func (mo mockObject) Pos() token.Pos { return mo.posFn() } +func (mo mockObject) Type() types.Type { return mo.typeFn() } func TestId(t *testing.T) { var tests = []struct { @@ -22,14 +28,24 @@ func TestId(t *testing.T) { want string pkgFn func() *types.Package nameFn func() string - posFn func() token.Pos + typeFn func() types.Type }{ { - name: "no package", - want: "name 1", + name: "no_package", + want: "name type", pkgFn: func() *types.Package { return nil }, nameFn: func() string { return "name" }, - posFn: func() token.Pos { return token.Pos(1) }, + typeFn: func() types.Type { return mockType{s: "type"} }, + }, + + { + name: "package_exists", + want: "package pkg (\"github.com/mccurdyc/path\") name type", + pkgFn: func() *types.Package { + return types.NewPackage("github.com/mccurdyc/path", "pkg") + }, + nameFn: func() string { return "name" }, + typeFn: func() types.Type { return mockType{s: "type"} }, }, } for _, tt := range tests { @@ -37,7 +53,7 @@ func TestId(t *testing.T) { o := mockObject{ pkgFn: tt.pkgFn, nameFn: tt.nameFn, - posFn: tt.posFn, + typeFn: tt.typeFn, } got := Id(o) diff --git a/internal/graph/graph.go b/internal/graph/graph.go index f5d947f..707bbf5 100644 --- a/internal/graph/graph.go +++ b/internal/graph/graph.go @@ -29,27 +29,30 @@ func (g Graph) ContainsNode(id string) bool { return ok } -// Partition returns a slice of nodes that should be split from a given source graph. -// -// TODO (Issue#8): actually add logic to properly partition -func (g Graph) Partition() []*Node { - res := make([]*Node, 0, len(g)) - - // TODO: for now, just return every node - for _, v := range g { - res = append(res, v) +// Roots finds the many possible roots in the graph. +// The order of the graph traversal matters because if a parent is added _after_ +// a child, then that child node won't have the identified parent node. +func (g Graph) Roots() []*Node { + roots := make([]*Node, 0, len(g)) + + for _, node := range g { + if len(node.Parents) == 0 { + roots = append(roots, node) + } } - return res + return roots } -func (g Graph) shouldPartition() bool { - if len(g) <= 1 { - return false - } +// Edges returns all of the edges in the graph. +func (g Graph) Edges() []WeightedEdge { + edges := make([]WeightedEdge, 0) - // TODO: thought; maybe eventually we store meta data about the graph - // (e.g., types of edges such as method and if there are zero methods, maybe we consider not partitioning) + for _, node := range g { + for _, edge := range node.Edges { + edges = append(edges, edge) + } + } - return true + return edges } diff --git a/internal/graph/graph_test.go b/internal/graph/graph_test.go index 38252d9..ac80c8c 100644 --- a/internal/graph/graph_test.go +++ b/internal/graph/graph_test.go @@ -31,15 +31,25 @@ func TestNew(t *testing.T) { } var ( + parentNode = Node{ID: "parent", Edges: make(map[string]WeightedEdge), Parents: make(map[string]WeightedEdge)} + + parentEdge = map[string]WeightedEdge{ + "parent": WeightedEdge{ + Weight: 1.0, + Dest: &parentNode, + }, + } + edgeB = map[string]WeightedEdge{ "b": WeightedEdge{ Weight: 1.0, - Dest: &Node{ID: "b", Edges: make(map[string]WeightedEdge)}, + Dest: &Node{ID: "b", Edges: make(map[string]WeightedEdge), Parents: make(map[string]WeightedEdge)}, }, } - nodeAWithSingleEdgeB = Node{ID: "a", Edges: edgeB} - nodeCWithSingleEdgeB = Node{ID: "c", Edges: edgeB} + nodeAWithSingleEdgeB = Node{ID: "a", Edges: edgeB, Parents: make(map[string]WeightedEdge)} + nodeCWithSingleEdgeB = Node{ID: "c", Edges: edgeB, Parents: make(map[string]WeightedEdge)} + nodeCWithEdgeBAndParent = Node{ID: "c", Edges: edgeB, Parents: parentEdge} ) func TestAddNode(t *testing.T) { @@ -153,3 +163,36 @@ func TestContainsNode(t *testing.T) { }) } } + +func TestRoots(t *testing.T) { + tests := []struct { + name string + graph Graph + want []*Node + }{ + { + name: "empty-graph", + graph: Graph(make(map[string]*Node)), + want: []*Node{}, + }, + + { + name: "one-root-node", + graph: Graph(map[string]*Node{ + "parent": &parentNode, + "c": &nodeCWithEdgeBAndParent, + }), + want: []*Node{&parentNode}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got := test.graph.Roots() + + if diff := cmp.Diff(test.want, got); diff != "" { + t.Errorf("(%+v) Roots() mismatch (-want +got): \n%s", test.graph, diff) + } + }) + } +} diff --git a/internal/graph/node.go b/internal/graph/node.go index 5b21202..07f577a 100644 --- a/internal/graph/node.go +++ b/internal/graph/node.go @@ -4,31 +4,46 @@ import ( "errors" ) +const defaultWeight = -1.0 + // WeightedEdge contains pointers to source and destination Nodes as well as // the assigned weight of the relationship. type WeightedEdge struct { - Weight float64 - Dest *Node + Weight float64 + Source, Dest *Node + + ConnectednessStrength float64 + + Partition bool + Distance float64 } // Node has an ID and a map of WeightedEdges or weighted relationships to other // Nodes. type Node struct { - ID string - Object interface{} - Edges map[string]WeightedEdge + ID string + Object interface{} + Edges map[string]WeightedEdge + Parents map[string]WeightedEdge // TODO: may be able to delete this now + + ShortestPath float64 + ShortestPaths []float64 } // NewNode creates a pointer to a new Node with ID, id, and initializes a map of Edges. func NewNode(id string, v interface{}) *Node { return &Node{ - ID: id, - Object: v, - Edges: make(map[string]WeightedEdge), + ID: id, + Object: v, + Edges: make(map[string]WeightedEdge), + Parents: make(map[string]WeightedEdge), + ShortestPath: defaultWeight, + ShortestPaths: make([]float64, 0), } } // AddEdges adds weighted edges to a source node that signify relationships with other nodes. +// Also adds parents to the destination node. func (n *Node) AddEdge(dest *Node, w float64) { // prevent edge between node and itself if n.ID == dest.ID { @@ -37,8 +52,15 @@ func (n *Node) AddEdge(dest *Node, w float64) { n.Edges[dest.ID] = WeightedEdge{ Weight: w, + Source: n, Dest: dest, } + + dest.Parents[n.ID] = WeightedEdge{ + Weight: w, + Source: dest, + Dest: n, + } } // ContainsEdge returns whether or not the graph contains an edge from source to dest. @@ -60,5 +82,9 @@ func (n *Node) Valid() (bool, error) { return false, errors.New("invalid node; edge map must be initialized") } + if n.Parents == nil { + return false, errors.New("invalid node; parent map must be initialized") + } + return true, nil } diff --git a/internal/graph/node_test.go b/internal/graph/node_test.go index 63f7c69..7ffb9a8 100644 --- a/internal/graph/node_test.go +++ b/internal/graph/node_test.go @@ -5,6 +5,26 @@ import ( "testing" ) +var ( + nodeA = Node{ + ID: "a", + Object: nil, + Edges: map[string]WeightedEdge{}, + Parents: map[string]WeightedEdge{}, + ShortestPath: 1.0, + ShortestPaths: []float64{1.0}, + } + + nodeB = Node{ + ID: "b", + Object: nil, + Edges: map[string]WeightedEdge{}, + Parents: map[string]WeightedEdge{}, + ShortestPath: 1.0, + ShortestPaths: []float64{1.0}, + } +) + func TestAddEdge(t *testing.T) { tests := []struct { name string @@ -15,21 +35,31 @@ func TestAddEdge(t *testing.T) { }{ { name: "add-edge-w1.0-to-empty-slice-should-return-edges-with-single-value", - node: &Node{"a", nil, map[string]WeightedEdge{}}, - dest: &Node{"b", nil, map[string]WeightedEdge{}}, + node: &nodeA, + dest: &nodeB, weight: 1.0, - want: Node{"a", nil, map[string]WeightedEdge{"b": WeightedEdge{ - Weight: 1.0, - Dest: &Node{ID: "b", Object: nil, Edges: map[string]WeightedEdge{}}, - }}}, + want: Node{ + ID: "a", + Object: nil, + Edges: map[string]WeightedEdge{ + "b": WeightedEdge{ + Weight: 1.0, + Source: &nodeA, + Dest: &nodeB, + }, + }, + Parents: map[string]WeightedEdge{}, + ShortestPath: 1.0, + ShortestPaths: []float64{1.0}, + }, }, { name: "add-edge-to-same-node-does-nothing", - node: &Node{"a", nil, map[string]WeightedEdge{}}, - dest: &Node{"a", nil, map[string]WeightedEdge{}}, + node: &nodeA, + dest: &nodeA, weight: 1.0, - want: Node{"a", nil, map[string]WeightedEdge{}}, + want: nodeA, }, } @@ -38,7 +68,7 @@ func TestAddEdge(t *testing.T) { tt.node.AddEdge(tt.dest, tt.weight) if ok := reflect.DeepEqual(*tt.node, tt.want); !ok { - t.Errorf("(%+v) AddEdge(%+v, %f) - mismatch \n%+v", tt.node, tt.dest, tt.weight, tt.want) + t.Errorf("AddEdge() - mismatch \n\twant: %+v\n\tgot:%+v", tt.want, *tt.node) } }) } diff --git a/internal/graph/partition.go b/internal/graph/partition.go new file mode 100644 index 0000000..412eb08 --- /dev/null +++ b/internal/graph/partition.go @@ -0,0 +1,134 @@ +package graph + +import ( + "log" +) + +func shouldPartition(g Graph) bool { + if len(g) <= 1 { + return false + } + + // TODO: thought; maybe eventually we store meta data about the graph + // (e.g., types of edges such as method and if there are zero methods, maybe we consider not partitioning) + + return true +} + +// Partition returns a slice of WeightedEdges that should be partitioned (i.e., "broken"). +// The pseudo-algorithm for Partition is as follows: +// 1. find root nodes +// 2. recursively traverse the graph +// a. keeping track of the current shortest path to a node +// b. keeping track of all observed shortest paths to a node +// 4. calculate edge distances +// 5. identify edges where the sum of cross-iteration distances > epsilon +func Partition(g Graph, epsilon float64) []WeightedEdge { + roots := make(map[string]*Node) + for _, r := range g.Roots() { + roots[r.ID] = r + } + + var curr, next map[string]*Node + curr = roots + + for len(next) > 0 { + next = bfs(curr) + + for id, n := range curr { + g[id] = n + } + + curr = next + } + + dists := calculateDistances(g, g.Edges()) + return identifyPartitions(dists, epsilon) +} + +// bfs does breadth-first search of the graph, keeping track of the current and past shortest paths. +func bfs(level map[string]*Node) map[string]*Node { + next := make(map[string]*Node) + + for _, node := range level { + for _, childEdge := range node.Edges { + p, changed := shortestPath(childEdge) + + childEdge.Dest.ShortestPaths = append(childEdge.Dest.ShortestPaths, p) + + if changed { + childEdge.Dest.ShortestPath = p + next[childEdge.Dest.ID] = childEdge.Dest + } + } + } + + return next +} + +func shortestPath(edge WeightedEdge) (res float64, changed bool) { + res = edge.Weight + + if edge.Source != nil && edge.Source.ShortestPath != defaultWeight { + res += edge.Source.ShortestPath + } + + if edge.Dest == nil || edge.Dest.ShortestPath == defaultWeight || res < edge.Dest.ShortestPath { + return res, true + } + + return edge.Dest.ShortestPath, false +} + +// calculateDistances calculates the strongest strong distance of an edge from +// every possible root in the graph. +// +// The strongest strong distance is defined as: +// * 1 / CONN(u, v) +// * where CONN(u, v) denotes the strength of connectedness of a pair of vertices. +// * CONN(u, v) is defined as the MAX{s(P)}, where s(P) denotes the MIN edge weight in a path. +// * CONN(u, v) can be summarized as the max, weakest edge between nodes of various paths. +// +// "Distances in Weighted Graphs" with authors Dhanyamol M V and Sunil Mathew. +// * http://www.researchmathsci.org/apamart/apam-v8n1-1.pdf +// +// In splitfile, we modify this equation slightly to get a more global understanding +// * 1 / SUM(CONN(u, v)) +// * where we are summing the CONN(u, v) from different roots +// +// If you are interested in my notes/highlights, check out this Tweet +// * https://twitter.com/McCurdyColton/status/1179024173664002049?s=20 +func calculateDistances(g Graph, edges []WeightedEdge) []WeightedEdge { + res := make([]WeightedEdge, 0, len(edges)) + + for _, edge := range edges { + var sum float64 + for _, sp := range edge.Dest.ShortestPaths { + sum += sp + } + + if sum == 0.0 { + continue + } + + edge.Distance = 1 / (sum) + res = append(res, edge) + } + + return res +} + +// identifyPartitions identifies the edges where the distance is greater than the +// configured epsilon value. These edges will be split/"broken". +func identifyPartitions(edges []WeightedEdge, epsilon float64) []WeightedEdge { + res := make([]WeightedEdge, 0, len(edges)) + + for _, e := range edges { + if e.Distance > epsilon { + log.Printf("identified partition with distance: %+v\n\tsrc: %+v \n\tdest: %+v\n", e.Distance, e.Source, e.Dest) + res = append(res, e) + } + } + + return res +} diff --git a/internal/graph/partition_test.go b/internal/graph/partition_test.go new file mode 100644 index 0000000..83c82b3 --- /dev/null +++ b/internal/graph/partition_test.go @@ -0,0 +1,185 @@ +package graph + +import ( + "testing" + + "github.com/google/go-cmp/cmp" +) + +func Test_bfs(t *testing.T) { + var tests = map[string]struct { + input map[string]*Node + want map[string]*Node + }{ + "node_two_changed_children_should_return_two_next-level_nodes": { + input: map[string]*Node{"a": &Node{ + ID: "a", + Edges: map[string]WeightedEdge{ + "b": { + Weight: 1.0, + Source: &Node{ID: "a", ShortestPath: 1.0}, + Dest: &Node{ID: "b", ShortestPath: 3.0, ShortestPaths: []float64{3.0}, Edges: map[string]WeightedEdge{}}, + }, + "c": { + Weight: 4.0, + Source: &Node{ID: "a", ShortestPath: 1.0}, + Dest: &Node{ID: "c", ShortestPath: 6.0, ShortestPaths: []float64{6.0}, Edges: map[string]WeightedEdge{}}, + }, + }, + ShortestPath: 1.0, + ShortestPaths: []float64{1.0}, + }}, + want: map[string]*Node{ + "b": &Node{ + ID: "b", + Edges: map[string]WeightedEdge{}, + ShortestPath: 2.0, + ShortestPaths: []float64{3.0, 2.0}, + }, + "c": &Node{ + ID: "c", + Edges: map[string]WeightedEdge{}, + ShortestPath: 5.0, + ShortestPaths: []float64{6.0, 5.0}, + }, + }, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + got := bfs(tt.input) + + if diff := cmp.Diff(got, tt.want); diff != "" { + t.Errorf("bfs() mismatch (-want +got):\n%s", diff) + } + }) + } +} + +func Test_shortestPath(t *testing.T) { + type input struct { + edge WeightedEdge + } + + type want struct { + shortest float64 + changed bool + } + + tests := map[string]struct { + input input + want want + }{ + "nil_source_node_with_edge_weight_less_than_current": { + input: input{ + edge: WeightedEdge{ + Weight: 2.0, + Source: nil, + Dest: &Node{ShortestPath: 4.0}, + }, + }, + want: want{ + shortest: 2.0, + changed: true, + }, + }, + + "nil_source_node_with_edge_weight_greater_than_current": { + input: input{ + edge: WeightedEdge{ + Weight: 2.0, + Source: nil, + Dest: &Node{ShortestPath: 1.0}, + }, + }, + want: want{ + shortest: 1.0, + changed: false, + }, + }, + + "source_node_with_default_weight": { + input: input{ + edge: WeightedEdge{ + Weight: 2.0, + Source: &Node{ShortestPath: defaultWeight}, + }, + }, + want: want{ + shortest: 2.0, + changed: true, + }, + }, + + "nil_dest_node_new_shortest": { + input: input{ + edge: WeightedEdge{ + Weight: 2.0, + Source: &Node{ShortestPath: 1.0}, + Dest: nil, + }, + }, + want: want{ + shortest: 3.0, + changed: true, + }, + }, + + "dest_node_with_default_weight_force_new_shortest": { + input: input{ + edge: WeightedEdge{ + Weight: 2.0, + Source: &Node{ShortestPath: 3.0}, + Dest: &Node{ShortestPath: defaultWeight}, + }, + }, + want: want{ + shortest: 5.0, + changed: true, + }, + }, + + "new_shortest_path": { + input: input{ + edge: WeightedEdge{ + Weight: 2.0, + Source: &Node{ShortestPath: 2.0}, + Dest: &Node{ShortestPath: 5.0}, + }, + }, + want: want{ + shortest: 4.0, + changed: true, + }, + }, + + "equal_shortest_path": { + input: input{ + edge: WeightedEdge{ + Weight: 2.0, + Source: &Node{ShortestPath: 2.0}, + Dest: &Node{ShortestPath: 4.0}, + }, + }, + want: want{ + shortest: 4.0, + changed: false, + }, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + gotShortest, gotChanged := shortestPath(tt.input.edge) + + if gotShortest != tt.want.shortest { + t.Errorf("shortestPath() = %+v - want: %+v\n\tinput: %+v\n", gotShortest, tt.want.shortest, tt.input) + } + + if gotChanged != tt.want.changed { + t.Errorf("shortestPath() = %+v - want: %+v\n\tinput: %+v\n", gotChanged, tt.want.changed, tt.input) + } + }) + } +} diff --git a/splitfile.go b/splitfile.go index 57f86ae..e40eee1 100644 --- a/splitfile.go +++ b/splitfile.go @@ -14,7 +14,7 @@ import ( var Analyzer = &analysis.Analyzer{ Name: "splitfile", - Doc: "checks for clean splits of files in packages based on objects and their relationships with other objects.", + Doc: "A static analysis that identifies partitions of declarations and their uses to improve the readability of Go packages.", Requires: []*analysis.Analyzer{inspect.Analyzer}, Run: run, } @@ -26,14 +26,17 @@ type Poser interface { func run(pass *analysis.Pass) (interface{}, error) { g := traverse(pass.TypesInfo.Defs) - nodes := g.Partition() // TODO (Issue#8): right now, this returns every single node in the graph - for _, n := range nodes { - p, ok := n.Object.(Poser) + edges := graph.Partition(g, 0.1) // TODO: make this epsilon value configurable (or at least find a reasonable default i.e., the "natural" value) + for _, e := range edges { + src, ok := e.Source.Object.(Poser) if !ok { continue } - pass.Reportf(p.Pos(), "parition found - %+v", n) + pass.Report(analysis.Diagnostic{ + Pos: src.Pos(), + Message: "potential partition identified", + }) } return nil, nil @@ -45,18 +48,19 @@ func traverse(defs map[*ast.Ident]types.Object) graph.Graph { g := graph.New() for _, def := range defs { - if skip := filter(def); skip { continue } - node := graph.NewNode(Id(def), def) - err := g.AddNode(node) - if err != nil { - continue + if !g.ContainsNode(Id(def)) { + node := graph.NewNode(Id(def), def) + err := g.AddNode(node) + if err != nil { + continue + } } - err = addRelated(g, node) + err := addRelated(g, g[Id(def)]) if err != nil { continue } @@ -71,7 +75,12 @@ func filter(def types.Object) bool { return true } - return false + switch def.(type) { + case *types.Func, *types.TypeName: + return false + } + + return true } type Typer interface { @@ -88,19 +97,31 @@ func addRelated(g graph.Graph, node *graph.Node) error { if !ok { return errors.New("node does not have a Type() method") } + m := checkMethods(types.NewMethodSet(t.Type())) - for _, r := range m { - if r.ID == node.ID { - continue + // it is important to add receivers first + for _, v := range m { + if !g.ContainsNode(v.method.ID) { + err := g.AddNode(v.method) + if err != nil { + continue + } } - err := g.AddNode(r) - if err != nil { - continue - } + // Lower weight is better. It indicates "closeness" + node.AddEdge(g[v.method.ID], 2.0) // TODO (Issue #15): read value from config or use default - node.AddEdge(r, 5.0) // TODO (Issue #15): read value from config or use default + for _, r := range v.other { + if !g.ContainsNode(r.ID) { + err := g.AddNode(r) + if err != nil { + continue + } + } + + g[v.method.ID].AddEdge(g[r.ID], 5.0) // params, results, etc should be on the method node, not the receiver node + } } // TODO: check other places for related (e.g., funcs, interfaces, etc.) @@ -108,54 +129,76 @@ func addRelated(g graph.Graph, node *graph.Node) error { return nil } +type methodSetResult map[string]methodResult +type methodResult struct { + method *graph.Node + receivers []*graph.Node + other []*graph.Node +} + // checkMethods checks methods' signatures for related types. -func checkMethods(mset *types.MethodSet) []*graph.Node { - rel := make([]*graph.Node, 0) +func checkMethods(mset *types.MethodSet) methodSetResult { + res := make(methodSetResult) for i := 0; i < mset.Len(); i++ { method := mset.At(i) - m := graph.NewNode(Id(method.Obj()), method.Obj()) - rel = append(rel, m) // methods themselves are always related - sig, ok := method.Type().(*types.Signature) if !ok { - continue + continue // skip evaluating method bodies + } + + m := graph.NewNode(Id(method.Obj()), method.Obj()) + v := methodResult{ + method: m, + receivers: make([]*graph.Node, 0), + other: make([]*graph.Node, 0), } - related := checkSignature(sig) - rel = append(rel, related...) + sigRes := checkSignature(sig) + v.receivers = append(v.receivers, sigRes.receivers...) + v.other = append(v.other, sigRes.other...) + + res[m.ID] = v } - return rel + return res } -// checkSignature checks a function signature, the receiver (if it is a method this -// will be a non-nil value), the parameters and the return types. -func checkSignature(sig *types.Signature) []*graph.Node { - rel := make([]*graph.Node, 0) +// checkSignature checks a function signature. Specifically, the parameters and the return types. +func checkSignature(sig *types.Signature) methodResult { + res := methodResult{ + receivers: make([]*graph.Node, 0), + other: make([]*graph.Node, 0), + } if v := checkVar(sig.Recv()); v != nil { - rel = append(rel, v) + res.receivers = append(res.receivers, v) } - rel = append(rel, checkTuple(sig.Params())...) - rel = append(rel, checkTuple(sig.Results())...) + res.other = append(res.other, checkTuple(sig.Params())...) + res.other = append(res.other, checkTuple(sig.Results())...) - return rel + return res } -// checkVar validates a variable and if it is valid, it is returned as a valid related. +// checkVar validates a variable and if it is valid, it is returned as a valid node. func checkVar(v *types.Var) *graph.Node { if v == nil || v.Type() == types.Type(nil) { return nil } - return graph.NewNode(Id(v), v) + typ, ok := v.Type().(*types.Named) + if !ok { + return nil + } + obj := typ.Obj() + + return graph.NewNode(Id(obj), obj) } -// checkTuple checks a tuple of variables for related nodes. +// checkTuple checks a tuple of variables for nodes. func checkTuple(vars *types.Tuple) []*graph.Node { - rel := make([]*graph.Node, 0) + res := make([]*graph.Node, 0) for i := 0; i < vars.Len(); i++ { v := checkVar(vars.At(i)) @@ -164,8 +207,8 @@ func checkTuple(vars *types.Tuple) []*graph.Node { continue } - rel = append(rel, v) + res = append(res, v) } - return rel + return res } diff --git a/splitfile_analysis_test.go b/splitfile_analysis_test.go index b8c4079..f541b8d 100644 --- a/splitfile_analysis_test.go +++ b/splitfile_analysis_test.go @@ -17,10 +17,10 @@ import ( // rather deep directory hierarchy, and obscure similarities among // related tests, especially when tests involve multiple packages, or // multiple variants of a single scenario. -// func TestFromFileSystem(t *testing.T) { -// testdata := analysistest.TestData() -// analysistest.Run(t, testdata, splitfile.Analyzer, "abc") -// } +func TestFromFileSystem(t *testing.T) { + testdata := analysistest.TestData() + analysistest.Run(t, testdata, splitfile.Analyzer, "abc") +} // TestFromStringLiterals demonstrates how to test an analysis using // a table of string literals for each test case. diff --git a/splitfile_test.go b/splitfile_test.go new file mode 100644 index 0000000..511c384 --- /dev/null +++ b/splitfile_test.go @@ -0,0 +1,123 @@ +package splitfile + +import ( + "github.com/mccurdyc/splitfile/internal/graph" +) + +var ( + emptyNode = graph.Node{ + ID: "node", + Object: nil, + Edges: make(map[string]graph.WeightedEdge), + Parents: make(map[string]graph.WeightedEdge), + ShortestPath: -1.0, + ShortestPaths: make([]float64, 0), + } +) + +// func Test_addRelated(t *testing.T) { +// var tests = []struct { +// name string +// graph graph.Graph +// pkgpath string +// files map[string]string +// node *graph.Node +// typeNameToCheck string +// want graph.Graph +// wantErr error +// }{ +// { +// name: "type with no methods", +// graph: make(graph.Graph), +// pkgpath: "a", +// files: map[string]string{"a/a.go": ` +// package a +// type a int +// `}, +// node: &emptyNode, +// typeNameToCheck: "a", +// want: make(graph.Graph), +// wantErr: nil, +// }, +// +// { +// name: "type with a single method", +// graph: make(graph.Graph), +// pkgpath: "a", +// files: map[string]string{"a/a.go": ` +// package a +// type a int +// func (a a) Val() int { +// return int(a) +// } +// `}, +// node: &graph.Node{ +// ID: "package a (\"a\")", +// }, +// typeNameToCheck: "a", +// want: graph.Graph{ +// "package a (\"a\")": &graph.Node{}, +// "package a (\"a\") Val func() int": &graph.Node{ +// ID: "package a (\"a\") Val func() int", +// Object: nil, +// Edges: map[string]graph.WeightedEdge{}, +// Parents: map[string]graph.WeightedEdge{}, +// ShortestPath: -1.0, +// ShortestPaths: []float64{}, +// }, +// "package a (\"a\") a a.a": &graph.Node{ +// ID: "package a (\"a\") a a.a", +// Object: nil, +// Edges: map[string]graph.WeightedEdge{}, +// Parents: map[string]graph.WeightedEdge{}, +// ShortestPath: -1.0, +// ShortestPaths: []float64{}, +// }, +// }, +// wantErr: nil, +// }, +// } +// +// for _, tt := range tests { +// tt := tt +// t.Run(tt.name, func(t *testing.T) { +// fset := token.NewFileSet() +// var files []*ast.File +// +// for fname, fileContents := range tt.files { +// f, err := parser.ParseFile(fset, fname, fileContents, 0) +// if err != nil { +// log.Fatal(err) +// } +// +// files = append(files, f) +// } +// +// conf := types.Config{Importer: importer.Default()} +// pkg, err := conf.Check(tt.pkgpath, fset, files, nil) +// if err != nil { +// log.Fatal(err) +// } +// +// tt.node.Object = pkg.Scope().Lookup(tt.typeNameToCheck) +// +// gotErr := addRelated(tt.graph, tt.node) +// +// if !reflect.DeepEqual(tt.want, tt.graph) { +// t.Errorf("addRelated() mismatch:\n\twant: %+v\n\tgot: %+v", tt.want, tt.graph) +// } +// +// // https://github.com/google/go-cmp/issues/24 +// errorCmp := func(x, y error) bool { +// if x == nil || y == nil { +// return x == nil && y == nil +// } +// return x.Error() == y.Error() +// } +// +// if ok := errorCmp(gotErr, tt.wantErr); !ok { +// t.Errorf("addRelated() = %v, wantErr %v", gotErr, tt.wantErr) +// } +// }) +// } +// } diff --git a/testdata/src/abc/abc.go b/testdata/src/abc/abc.go index a0ac4fb..9d470f5 100644 --- a/testdata/src/abc/abc.go +++ b/testdata/src/abc/abc.go @@ -1,40 +1,13 @@ package abc -type Ta struct { - Tb Tb -} +type ( + A int + B int + C int +) -func (Ta) M1() {} -func (Ta) M2() {} -func Fa() Ta { - var ta Ta - return ta -} +func fabc(a A, b B, c C) {} -type Tb int - -func (Tb) M1() {} -func FCommon() { - var ta Ta - var tb Tb - - if ta.Tb == 0 || tb == 0 { - return - } - return -} - -func FCommonParams(ta Ta, tb Tb) { - return -} - -type tNonExported int - -func (tNonExported) M1() {} -func (tNonExported) M2() {} -func (t *tNonExported) M3() {} -func (t *tNonExported) M4() {} -func F() *tNonExported { - t := tNonExported(1) - return &t -} +func (a A) ma(b B) {} +func (b B) mb() {} +func (c C) mc(b B) {}