Skip to content

Commit

Permalink
Fix errors for unreachable @rules. (#7660)
Browse files Browse the repository at this point in the history 
### Problem

While iterating in another branch, I noticed that a shadowed/unreachable `@rule` was not triggering an error. Most likely this issue was introduced when we began monomorphizing `@rules`.

### Solution

Fix the error for unreachable `@rules`. Additionally, backport a simplification to the `@rule` selection algorithm from #7600 which makes it significantly easier to describe.

### Result

Better error messages when `@rules` are shadowed or otherwise unreachable.
  • Loading branch information
@stuhood
stuhood authored May 7, 2019
1 parent b7c4cf3 commit 59bedd3
Show file tree
Hide file tree
Showing 2 changed files with 99 additions and 68 deletions.
130 changes: 64 additions & 66 deletions src/rust/engine/src/rule_graph.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ impl UnreachableError {
task_rule: task_rule,
diagnostic: Diagnostic {
params: ParamTypes::default(),
reason: "Unreachable".to_string(),
reason: "Was not usable by any other @rule.".to_string(),
details: vec![],
},
}
Expand All @@ -45,16 +45,6 @@ impl EntryWithDeps {
}
}

fn task_rule(&self) -> Option<&Task> {
match self {
&EntryWithDeps::Inner(InnerEntry {
rule: Rule::Task(ref task_rule),
..
}) => Some(task_rule),
_ => None,
}
}

///
/// Returns the set of SelectKeys representing the dependencies of this EntryWithDeps.
///
Expand Down Expand Up @@ -263,7 +253,7 @@ impl<'t> GraphMaker<'t> {
);
}

let unreachable_rules = self.unreachable_rules(&dependency_edges, &unfulfillable_rules);
let unreachable_rules = self.unreachable_rules(&dependency_edges);

RuleGraph {
root_param_types: self.root_param_types.clone(),
Expand All @@ -273,13 +263,38 @@ impl<'t> GraphMaker<'t> {
}
}

///
/// Compute input TaskRules that are unreachable from root entries.
///
fn unreachable_rules(
&self,
full_dependency_edges: &RuleDependencyEdges,
full_unfulfillable_rules: &UnfulfillableRuleMap,
) -> Vec<UnreachableError> {
let rules_in_graph: HashSet<_> = full_dependency_edges
// Walk the graph, starting from root entries.
let mut entry_stack: Vec<_> = full_dependency_edges
.keys()
.filter(|entry| match entry {
&EntryWithDeps::Root(_) => true,
_ => false,
})
.collect();
let mut visited = HashSet::new();
while let Some(entry) = entry_stack.pop() {
if visited.contains(&entry) {
continue;
}
visited.insert(entry);

if let Some(edges) = full_dependency_edges.get(entry) {
entry_stack.extend(edges.all_dependencies().filter_map(|e| match e {
&Entry::WithDeps(ref e) => Some(e),
_ => None,
}));
}
}

let reachable_rules: HashSet<_> = visited
.into_iter()
.filter_map(|entry| match entry {
&EntryWithDeps::Inner(InnerEntry {
rule: Rule::Task(ref task_rule),
Expand All @@ -288,17 +303,12 @@ impl<'t> GraphMaker<'t> {
_ => None,
})
.collect();
let unfulfillable_discovered_during_construction: HashSet<_> = full_unfulfillable_rules
.keys()
.filter_map(EntryWithDeps::task_rule)
.cloned()
.collect();

self
.tasks
.all_tasks()
.iter()
.filter(|r| !rules_in_graph.contains(r))
.filter(|r| !unfulfillable_discovered_during_construction.contains(r))
.filter(|r| !reachable_rules.contains(r))
.map(|&r| UnreachableError::new(r.clone()))
.collect()
}
Expand Down Expand Up @@ -674,38 +684,30 @@ impl<'t> GraphMaker<'t> {
if satisfiable_entries.is_empty() {
// No source of this dependency was satisfiable with these Params.
return vec![];
} else if satisfiable_entries.len() == 1 {
return satisfiable_entries;
}

// Prefer a Param, then the non-ambiguous rule with the smallest set of input Params.
// TODO: We should likely prefer Rules to Params.
if satisfiable_entries.len() == 1 {
satisfiable_entries
} else if let Some(param) = satisfiable_entries.iter().find(|e| match e {
&Entry::Param(_) => true,
_ => false,
}) {
vec![*param]
} else {
// We prefer the non-ambiguous rule with the smallest set of Params, as that minimizes Node
// identities in the graph and biases toward receiving values from dependencies (which do not
// affect our identity) rather than dependents.
let mut minimum_param_set_size = ::std::usize::MAX;
let mut rules = Vec::new();
for satisfiable_entry in satisfiable_entries {
if let &Entry::WithDeps(ref wd) = satisfiable_entry {
let param_set_size = wd.params().len();
if param_set_size < minimum_param_set_size {
rules.clear();
rules.push(satisfiable_entry);
minimum_param_set_size = param_set_size;
} else if param_set_size == minimum_param_set_size {
rules.push(satisfiable_entry);
}
}
// We prefer the non-ambiguous entry with the smallest set of Params, as that minimizes Node
// identities in the graph and biases toward receiving values from dependencies (which do not
// affect our identity) rather than dependents.
let mut minimum_param_set_size = ::std::usize::MAX;
let mut rules = Vec::new();
for satisfiable_entry in satisfiable_entries {
let param_set_size = match satisfiable_entry {
&Entry::WithDeps(ref wd) => wd.params().len(),
&Entry::Param(_) => 1,
};
if param_set_size < minimum_param_set_size {
rules.clear();
rules.push(satisfiable_entry);
minimum_param_set_size = param_set_size;
} else if param_set_size == minimum_param_set_size {
rules.push(satisfiable_entry);
}

rules
}

rules
}

fn powerset<'a, T: Clone>(slice: &'a [T]) -> impl Iterator<Item = Vec<T>> + 'a {
Expand Down Expand Up @@ -945,29 +947,25 @@ impl RuleGraph {
})
.collect();

let rule_diagnostics = self
.unfulfillable_rules
// Collect and dedupe rule diagnostics, preferring to render an unfulfillable error for a rule
// over an unreachable error.
let mut rule_diagnostics: HashMap<_, _> = self
.unreachable_rules
.iter()
.filter_map(|(e, diagnostics)| match e {
.map(|u| (&u.task_rule, vec![u.diagnostic.clone()]))
.collect();
for (e, diagnostics) in &self.unfulfillable_rules {
match e {
&EntryWithDeps::Inner(InnerEntry {
rule: Rule::Task(ref task_rule),
..
}) => Some((task_rule, diagnostics.clone())),
_ => {
// We're only checking rule usage not entry usage generally, so we ignore intrinsics.
None
}) if !used_rules.contains(&task_rule) && !diagnostics.is_empty() => {
rule_diagnostics.insert(task_rule, diagnostics.clone());
}
})
.chain(
self
.unreachable_rules
.iter()
.map(|u| (&u.task_rule, vec![u.diagnostic.clone()])),
);
for (task_rule, diagnostics) in rule_diagnostics {
if used_rules.contains(&task_rule) {
continue;
_ => {}
}
}
for (task_rule, diagnostics) in rule_diagnostics {
for d in diagnostics {
collated_errors
.entry(task_rule.clone())
Expand Down
37 changes: 35 additions & 2 deletions tests/python/pants_test/engine/test_rules.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,11 @@ def d_from_a(a):
create_scheduler(rules)

self.assert_equal_with_printing(dedent("""
Rules with errors: 1
Rules with errors: 3
(A, [B, C], a_from_b_and_c()):
Was not usable by any other @rule.
(A, [C, B], a_from_c_and_b()):
Was not usable by any other @rule.
(D, [A], d_from_a()):
Ambiguous rules to compute A with parameter types (B+C):
(A, [B, C], a_from_b_and_c()) for (B+C)
Expand Down Expand Up @@ -246,14 +250,43 @@ def a_from_c(c):
create_scheduler(rules)

self.assert_equal_with_printing(dedent("""
Rules with errors: 2
Rules with errors: 3
(A, [C], a_from_c()):
Was not usable by any other @rule.
(B, [D], b_from_d()):
No rule was available to compute D with parameter type SubA
(D, [A, SubA], [Get(A, C)], d_from_a_and_suba()):
No rule was available to compute A with parameter type SubA
""").strip(),
str(cm.exception))

def test_unreachable_rule(self):
"""Test that when one rule "shadows" another, we get an error."""
@rule(D, [])
def d_singleton():
yield D()

@rule(D, [B])
def d_for_b(b):
yield D()

rules = [
d_singleton,
d_for_b,
RootRule(B),
]

with self.assertRaises(Exception) as cm:
create_scheduler(rules)

self.assert_equal_with_printing(dedent("""
Rules with errors: 1
(D, [B], d_for_b()):
Was not usable by any other @rule.
""").strip(),
str(cm.exception)
)

def test_smallest_full_test(self):
@rule(A, [SubA])
def a_from_suba(suba):
Expand Down

0 comments on commit 59bedd3

Please sign in to comment.