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

Always include types in the engine's definition of equality. #10377

Merged
merged 1 commit into from Jul 16, 2020

Conversation

stuhood
Copy link
Sponsor Member

@stuhood stuhood commented Jul 16, 2020

Problem

I spent a few hours debugging why a boolean param was turning into an integer in some cases in the simple test in #10230, and determined that it was due to the behavior that 1 == True and 0 == False in Python. This impacts the engine in strange ways. For example: before the fix, the test in this change that attempts to use both a bool and an int fails with:

Exception: Values used as `Params` must have distinct types, but the following values had the same type (`int`):
  1
  1

Solution

Always include types in the engine's definition of equality. Although this affects more than just memoization/interning, my expectation is that in any possible position where the engine will use equals the default behavior would be undesirable.

@Eric-Arellano
Copy link
Contributor

How do you think this will impact Address and BuildFileAddress, which are both equal? We rely on that property with AddressFamily:

@property
def addressables_as_address_keyed(self) -> Dict[Address, TargetAdaptor]:
"""Identical to `addresses`, but with a `cast` to allow for type safe lookup of
`Address`es."""
return cast(Dict[Address, TargetAdaptor], self.addressables)

I think it's fine because that's a method and not an actual param.

@stuhood
Copy link
Sponsor Member Author

stuhood commented Jul 16, 2020

How do you think this will impact Address and BuildFileAddress, which are both equal? We rely on that property with AddressFamily

It shouldn't: the engine will only be directly comparing them when they are used as a Key/Param (both __eq__ and __hash__) or a Value (just __eq__, for a @rule to compare its new/old return values).

AFAIK, Address and BuildFileAddress are both used as Params in various places, but we would never want one to "turn into" the other by conversion to a Key/Param in the context of @rule arguments, for example. It's entirely possible that we've experienced bugs due to that though.

@stuhood stuhood merged commit 70a6004 into pantsbuild:master Jul 16, 2020
@stuhood stuhood deleted the stuhood/stricter-equals branch July 16, 2020 04:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants