Skip to content
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

Replace SingletonRule with zero-parameter @rules #7479

Merged
merged 3 commits into from Apr 2, 2019

Conversation

Projects
None yet
4 participants
@stuhood
Copy link
Member

commented Apr 2, 2019

Problem

In #6170, it became possible for a @rule to take no Params, meaning that its identity did not include any of its context, and it could act as a singleton. Before that change, it had always been the case that a @rule would have a "subject" in its identity, necessitating SingletonRule in order to avoid creating a new copy of the rule in the graph for each distinct subject.

Solution

Remove SingletonRule in favor of definition of zero-parameter @rules, which reduces the total number of concepts involved in the engine. Because SingletonRule used to allow a declaration to lie about the exact type it was providing (and give an instance of a subclass instead), usage of SymbolTable first needed to be cleaned up to remove subclassing.

@stuhood

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2019

Commits are useful to review independently.

@Eric-Arellano
Copy link
Contributor

left a comment

Very cool! I don't understand this as well as Danny and Daniel, so going to hold off on approval but LGTM to me.

Show resolved Hide resolved src/python/pants/engine/legacy/graph.py
@@ -242,13 +235,13 @@ class GoalMappingError(Exception):
def _make_goal_map_from_rules(rules):
goal_map = {}
goal_to_rule = [(rule.goal, rule) for rule in rules if getattr(rule, 'goal', None) is not None]
for goal, rule in goal_to_rule:
for goal, r in goal_to_rule:

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Apr 2, 2019

Contributor

This rename doesn't seem like much of a win. I often like short names, but only when it's for highly generic code / functional programming.

This comment has been minimized.

Copy link
@stuhood

stuhood Apr 2, 2019

Author Member

After importing rule, the linter was complaining that the loop variable was shadowing the import.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Apr 2, 2019

Contributor

Ah that's fair. Maybe for parity make it be for g, r? Not sure if that's worse or better.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Apr 2, 2019

Contributor

That's annoying. I like goal, r, as the r being rule can be deduced from the variable goal_to_rule's name.

@cosmicexplorer
Copy link
Contributor

left a comment

This is a really fantastic improvement.

@@ -300,7 +300,12 @@ class CppToolchain(datatype([('cpp_compiler', CppCompiler), ('cpp_linker', Linke
class HostLibcDev(datatype(['crti_object', 'fingerprint'])): pass


@rule(Platform, [])
def platform_singleton():
return Platform.current

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Apr 2, 2019

Contributor

This is much, much better.

new rules yet. Unit tests, however, can mix in `SchedulerTestBase` from
`tests/python/pants_test/engine/scheduler_test_base.py` to generate and execute a scheduler from a
given set of rules.
new rules yet. Unit tests, however, can mix in `TestBase` from

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Apr 2, 2019

Contributor

Plugins actually can add new rules after #6892, I somehow completely forgot to update the docs here.

@@ -242,13 +235,13 @@ class GoalMappingError(Exception):
def _make_goal_map_from_rules(rules):
goal_map = {}
goal_to_rule = [(rule.goal, rule) for rule in rules if getattr(rule, 'goal', None) is not None]
for goal, rule in goal_to_rule:
for goal, r in goal_to_rule:

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Apr 2, 2019

Contributor

That's annoying. I like goal, r, as the r being rule can be deduced from the variable goal_to_rule's name.

@classmethod
def table(cls):
return {'struct': Struct, 'target': Target}
TARGET_TABLE = SymbolTable({'struct': Struct, 'target': Target})

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Apr 2, 2019

Contributor

This is also much nicer.

@classmethod
def table(cls):
return {'bob': Bob}
EMPTY_TABLE = parser.SymbolTable({})

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Apr 2, 2019

Contributor

This makes the symbol table much easier to reason about, imo.

@@ -335,14 +336,26 @@ def setup_legacy_graph_extended(
exclude_target_regexps=exclude_target_regexps,
subproject_roots=subproject_roots)

@rule(GlobMatchErrorBehavior, [])
def glob_match_error_behavior_singleton():
return glob_match_error_behavior or GlobMatchErrorBehavior.ignore

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Apr 2, 2019

Contributor

I was going to say it might be interesting to have a more concise wrapper for these expressions, then realized that's exactly what we're removing in this diff. I'm definitely 100% ok with using @rule here there and everywhere -- something special is happening (we are creating a rule!) and it makes perfect sense to require that it be specially annotated.

This comment has been minimized.

Copy link
@stuhood

stuhood Apr 3, 2019

Author Member

Yea. And I have a sneaking suspicion that singletons are generally an anti-pattern anyway.

Certainly many of the things that are currently singletons probably should not be in the long term: instead, they should be constructed based on options parsing... and the inputs to options parsing would be Params.

@stuhood stuhood force-pushed the twitter:stuhood/remove-singleton-rule branch from e58eac1 to f1668ac Apr 2, 2019

@stuhood stuhood merged commit 2e14cb2 into pantsbuild:master Apr 2, 2019

1 check passed

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

@stuhood stuhood deleted the twitter:stuhood/remove-singleton-rule branch Apr 2, 2019

@illicitonion

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

This is really nice! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.