[engine] Use enum for RuleEdges keys, add factory for Selects w/o variants #4461

Merged
merged 3 commits into from Apr 20, 2017

Conversation

Projects
None yet
3 participants
@baroquebobcat
Contributor

baroquebobcat commented Apr 12, 2017

Problem

selector paths between entries in the rule graph are not as well defined as they could be. Because of that, the nodes.rs usages of RuleEdges end up needing to do a lot of subject type filtering after use.

Solution

This introduces a SelectorPath enum that better clarifies the entry -> selector -> entries relationship. It represents the projections to other subjects as well as the nested selectors, which means we don't have to do subject filtering on the use side except in a couple cases.

[engine] Use struct for RuleEdges keys, add factory method for Select…
…s w/o variants

This introduces a SelectorPath enum that better clarifies the entry -> selector -> entries relationship. It represents the projections to other subjects as well as the nested selectors, which means we don't have to do subject filtering on the use side except in a couple cases.

@baroquebobcat baroquebobcat requested a review from stuhood Apr 12, 2017

@baroquebobcat baroquebobcat assigned kwlzn and unassigned kwlzn Apr 12, 2017

@baroquebobcat baroquebobcat requested a review from kwlzn Apr 12, 2017

@stuhood

Looks good! Thanks Nick.

src/rust/engine/src/rule_graph.rs
+ NestedSelect(Selector, Select),
+ // The projected select of a multi-select operator when there can only be one projected type.
+ ProjectedNestedSelect(Selector, TypeId, Select),
+ // The projected select of a multi-select operator when there can multiple projected types.

This comment has been minimized.

@stuhood

stuhood Apr 12, 2017

Member

when there can be

@stuhood

stuhood Apr 12, 2017

Member

when there can be

src/rust/engine/src/rule_graph.rs
@@ -143,6 +143,20 @@ impl Entry {
}
}
+#[derive(Eq, Hash, PartialEq, Clone, Debug)]
+pub enum SelectorPath {

This comment has been minimized.

@stuhood

stuhood Apr 12, 2017

Member

Can you add a larger docstring to this one? I think I understand what's going on, but.

Is the idea that this uniquely keys a set of tasks that are possible in some context? And the 'path' bit is that it represents information that used to be represented by multiple Nodes in the graph?

@stuhood

stuhood Apr 12, 2017

Member

Can you add a larger docstring to this one? I think I understand what's going on, but.

Is the idea that this uniquely keys a set of tasks that are possible in some context? And the 'path' bit is that it represents information that used to be represented by multiple Nodes in the graph?

src/rust/engine/src/nodes.rs
@@ -590,7 +571,10 @@ impl SelectTransitive {
selector: selectors::Select { product: self.selector.product, variant_key: None },
subject: subject_key,
variants: self.variants.clone(),
- entries: self.product_entries.clone()
+ // NB: Filtering by the matching subject type, since we now have a concrete one.

This comment has been minimized.

@stuhood

stuhood Apr 12, 2017

Member

Can you add a paired comment somewhere else to show the contrast between "now" and "then"? It's not clear from this comment where it is that we don't know the subject type.

@stuhood

stuhood Apr 12, 2017

Member

Can you add a paired comment somewhere else to show the contrast between "now" and "then"? It's not clear from this comment where it is that we don't know the subject type.

This comment has been minimized.

@baroquebobcat

baroquebobcat Apr 12, 2017

Contributor

I'll make it clearer.

@baroquebobcat

baroquebobcat Apr 12, 2017

Contributor

I'll make it clearer.

src/rust/engine/src/rule_graph.rs
@@ -334,7 +348,7 @@ impl <'t> GraphMaker<'t> {
&entry,
t.clone(),
format!("no matches for {} when resolving {}",
- selector_str(&Selector::Select(Select { product: *product, variant_key: None})),
+ selector_str(&Selector::select(*product)),

This comment has been minimized.

@stuhood

stuhood Apr 12, 2017

Member

Is Selector::select redundant with Select::without_variant?

@stuhood

stuhood Apr 12, 2017

Member

Is Selector::select redundant with Select::without_variant?

This comment has been minimized.

@baroquebobcat

baroquebobcat Apr 12, 2017

Contributor

Yep.

@baroquebobcat

baroquebobcat Apr 12, 2017

Contributor

Yep.

src/rust/engine/src/rule_graph.rs
- fn add_edges_via(&mut self, selector_path: Vec<Selector>, new_dependencies: &Entries) {
- if selector_path.is_empty() && !new_dependencies.is_empty() {
+ pub fn entries_for(&self, selector_path: &SelectorPath) -> Entries {
+ self.selector_to_dependencies.get(selector_path).map(|x|x.clone()).unwrap_or_else(|| Vec::new())

This comment has been minimized.

@stuhood

stuhood Apr 12, 2017

Member

iter.map(|x|x.clone()) should be equivalent to iter.cloned() ('cloned' with a 'd')

@stuhood

stuhood Apr 12, 2017

Member

iter.map(|x|x.clone()) should be equivalent to iter.cloned() ('cloned' with a 'd')

This comment has been minimized.

@baroquebobcat

baroquebobcat Apr 12, 2017

Contributor

Ah. Neat.

@baroquebobcat

baroquebobcat Apr 12, 2017

Contributor

Ah. Neat.

src/rust/engine/src/rule_graph.rs
+///
+/// A key for the Selects used from a rule. Rules are only picked up by Select selectors. These keys uniquely identify the
+/// selects used by a particular entry in the rule graph so that they can be mapped to the dependencies they correspond
+// to.

This comment has been minimized.

@stuhood

stuhood Apr 20, 2017

Member

double/triple mixed here

@stuhood

stuhood Apr 20, 2017

Member

double/triple mixed here

This comment has been minimized.

@baroquebobcat

baroquebobcat Apr 20, 2017

Contributor

🤦‍♂️ I wonder if there's a lint rule for that.

@baroquebobcat

baroquebobcat Apr 20, 2017

Contributor

🤦‍♂️ I wonder if there's a lint rule for that.

@baroquebobcat baroquebobcat merged commit a94b727 into pantsbuild:master Apr 20, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

lenucksi added a commit to lenucksi/pants that referenced this pull request Apr 25, 2017

[engine] Use enum for RuleEdges keys, add factory for Selects w/o var…
…iants (#4461)

### Problem
selector paths between entries in the rule graph are not as well defined as they could be. Because of that, the `nodes.rs` usages of `RuleEdges` end up needing to do a lot of subject type filtering after use.

### Solution

This introduces a SelectKey enum that better clarifies the entry -> selector -> entries relationship. It represents the projections to other subjects as well as the nested selectors, which means we don't have to do subject filtering on the use side except in a couple cases.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment