Skip to content
This repository has been archived by the owner on Apr 1, 2022. It is now read-only.

Use new shrink function to filter sourceunit dependencies #319

Merged
merged 7 commits into from
Aug 5, 2021
Merged

Conversation

cnr
Copy link
Contributor

@cnr cnr commented Aug 4, 2021

Overview

When converting our internal graph representation to a SourceUnit we, among other things, filter out unsupported dependency types

This is problematic, because some analyzers (looking at you, Gradle) produce graphs containing only SubprojectType dependencies as direct. When we filter out those dependencies, we're left with a graph without any direct dependencies -- only deep dependencies and edges between them.

shrink is a new operation like filter that removes vertices, but incoming and outgoing edges from removed vertices are rewritten to preserve the overall graph structure. This means that, e.g., direct deps that are filtered out will have their successors marked as direct after a shrink

Unrelatedly, this refactors Graphing into its isomorphic representation, AdjacencyMap (Node a), where data Node a = Root | Node a. This gains us much more code reuse in general, and reduces the surface area for incorrectly implementing Graphing operations. In particular, it was non-trivial to correctly implement shrink otherwise. I also refactor the operations of Graphing to more-closely mirror those of AdjacencyMap

Acceptance criteria

  • SourceUnits generated from Gradle projects contain direct dependencies as appropriate
  • shrink is semantically sound for multi-node removal, rewiring several incoming/outgoing edges, and rewriting "direct" relationships

Testing plan

  1. Manual testing
  • scan a Gradle project (with at least one dependency) prior to these changes; notice that the resulting SourceUnit.Build.Imports array is empty
  • scan a Gradle project (with at least one dependency) after these changes; notice that the resulting SourceUnit.Build.Imports array is non-empty
  1. Unit tests to validate the properties of shrink/shrinkSingle

Risks

shrink needs to be semantically sound, because we run it on every SourceUnit we generate in the CLI.

References

ZD3001 / ZD3006

  • I added tests for this PR's change (or confirmed tests are not viable).
  • If this PR introduced a user-visible change, I added documentation into docs/.
  • I updated Changelog.md if this change is externally facing. If this PR did not mark a release, I added my changes into an # Unreleased section at the top.
  • I linked this PR to any referenced GitHub issues, if they exist.

@cnr cnr requested review from a team and skilly-lily and removed request for a team August 4, 2021 22:01
Copy link
Contributor

@skilly-lily skilly-lily left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One issue with a comment, otherwise looks good. shrink makes sense to me, semantically, and the test confirm the behavior.

giphub

src/Graphing.hs Show resolved Hide resolved
@cnr cnr enabled auto-merge (squash) August 5, 2021 23:27
@cnr cnr merged commit 68e284e into master Aug 5, 2021
@cnr cnr deleted the shrink branch August 5, 2021 23:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants