New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix edge removal in Graph that could make invalidation inaccurate #6123

Merged
merged 1 commit into from Jul 15, 2018

Conversation

Projects
None yet
3 participants
@stuhood
Copy link
Member

stuhood commented Jul 14, 2018

Problem

Removing edges with Graph (note, not StableGraph) causes their EdgeIndexes to change. So collecting all outbound edges and then removing them one by one could remove the wrong edges (or be a noop).

Solution

Lookup and remove edges one at a time.

@stuhood stuhood requested a review from illicitonion Jul 14, 2018

@@ -310,7 +310,6 @@ pub type StoreBytesExtern = extern "C" fn(*const ExternContext, *const u8, u64)

pub type StoreUtf8Extern = extern "C" fn(*const ExternContext, *const u8, u64) -> Handle;


This comment has been minimized.

@stuhood

stuhood Jul 14, 2018

Member

rustfmt decided this should change. Let's see what CI says.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 14, 2018

Contributor

Should we be using rustfmt to check formatting as well, to avoid this concern?

This comment has been minimized.

@stuhood

stuhood Jul 15, 2018

Member

We do.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 15, 2018

Contributor

Sorry -- then why might there be a discrepancy between this and CI?

This comment has been minimized.

@stuhood

stuhood Jul 15, 2018

Member

I don't know... I called it out because it was strange. While rustfmt is seemingly deterministic, it does seem to have multiple legal permutations of the same code. So, yea.

@@ -1075,12 +1075,13 @@ impl<N: Node> Graph<N> {
}

// Otherwise, clear the deps.
let dep_edges: Vec<_> = inner
// NB: Because `remove_edge` changes EdgeIndex values, we remove edges one at a time.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 14, 2018

Contributor

Would it be useful to look into StableGraph at some point? It seems to be specifically designed to address this concern (although it notes that feature parity is not quite there yet).

This comment has been minimized.

@stuhood

stuhood Jul 15, 2018

Member

We recently stopped using StableGraph for efficiency reasons.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 15, 2018

Contributor

Great! I will try to git grep for these questions in the future.

@stuhood stuhood force-pushed the twitter:stuhood/fix-graph-remove-edge branch from 320b347 to b3ebede Jul 15, 2018

@kwlzn

kwlzn approved these changes Jul 15, 2018

Copy link
Member

kwlzn left a comment

lgtm - would a test be feasible here to cement the bugfix tho?

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jul 15, 2018

lgtm - would a test be feasible here to cement the bugfix tho?

Possibly... but it would be pretty finicky. I'll see if I can turn this into an assert in a followup. For now I'd like to just get it into the release.

@stuhood stuhood merged commit 5c282f8 into pantsbuild:master Jul 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stuhood stuhood deleted the twitter:stuhood/fix-graph-remove-edge branch Jul 15, 2018

@stuhood stuhood added the bug label Jul 15, 2018

@stuhood stuhood added this to the 1.9.x milestone Jul 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment