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

[RFC] Redesign UserValue to avoid breaking code that inherits from it #355

Closed
whitequark opened this issue Apr 14, 2020 · 11 comments
Closed

[RFC] Redesign UserValue to avoid breaking code that inherits from it #355

whitequark opened this issue Apr 14, 2020 · 11 comments
Labels
Milestone

Comments

@whitequark
Copy link
Member

@whitequark whitequark commented Apr 14, 2020

UserValue fulfills an important role: it allows code downstream from nMigen to define objects that can be fluently used in nMigen code, i.e. on RHS and, possibly, LHS of nMigen assignments. It was originally created to reflect the non-primitive nature of Records, and also because there was some disagreement on the best way to handle Records. These disagreements have persisted, and I both acknowledge them, and see UserValue as an important tool to explore any solutions.

Unfortunately, as currently implemented, it has a fatal flaw. It pollutes the namespace of the classes that inherit from it. We add new methods to Value liberally (see #352 for the latest addition), and each of these methods has the potential of clashing with downstream code. As it is, any new method on Value is a breaking change, which is not acceptable.

Fortunately, there is a good solution. One can observe that UserValues (for example, any Records--at least post-#354) do not really need to implement any numeric operations because they are not numbers--indeed, if they were simple numbers, they would be normal Values. They don't necessarily implement bit vector operations either--for example, Record overrides __getitem__. They only need to be implicitly castable to Value as opposed to be a kind of Value.

I propose that we change UserValue to not inherit from Value at all, and make sure it injects a small and well-defined set of attributes into derived classes. Its primary purpose would then to be a marker class that Value.cast uses to recognize that it should first lower its argument.

Unresolved questions:

  • Should we rename lower to as_value? The former is a fairly generic term, and therefore potentially useful to derived classes. The latter, in context of nMigen, is quite unambiguous, and very unlikely to clash with user-defined record fields.
  • Should we provide eq? Some UserValues, such as Records, will be commonly used on LHS, and writing rec1.as_value().eq(rec2) is a lot less convenient than rec1.eq(rec2). However, one can implement a UserValue that lowers to a constant, and that would be never usable on LHS.
    One option is to leave it to the specific derived class to implement eq, since the implementation is as simple as return Assign(self, value, src_loc_at=1). Such an approach could also be useful because the derived class might be able to implement useful diagnostics that are not applicable to Values in general.
@anuejn
Copy link
Contributor

@anuejn anuejn commented Apr 15, 2020

I mostly agree on what you wrote here, however, I have the concern, that when there is no common base class for nMigen values, it would be harder to check which fields of an object are somehow nMigen related (e.g. for implementing iter_flat in a more generic way that doesn't assume Record is the only signal bundle)

Loading

@whitequark
Copy link
Member Author

@whitequark whitequark commented Apr 15, 2020

I have the concern, that when there is no common base class for nMigen values, it would be harder to check which fields of an object are somehow nMigen related

Well, in principle, instead of isinstance(x, Value) you would have isinstance(x, (Value, UserValue)), but...

e.g. for implementing iter_flat in a more generic way that doesn't assume Record is the only signal bundle

... my concern is that doing this is not a very good idea because there is no way to write an iter_flat that would apriori know what to do with any UserValue.

In other words, you can do it quite easily even after this change, but I think the minor speed bump you get is a good reminder to ask yourself, "is this actually a good idea?"

Loading

@anuejn
Copy link
Contributor

@anuejn anuejn commented Apr 15, 2020

okay

Loading

@awygle
Copy link
Collaborator

@awygle awygle commented Apr 24, 2020

I prefer as_value to lower, and do not think we should implement eq, except that we should do so for Record as it's provided with nmigen (and any other cases like that, if they exist). Otherwise I agree with this.

Loading

@whitequark
Copy link
Member Author

@whitequark whitequark commented Apr 24, 2020

@awygle We have a consensus then.

Loading

@whitequark
Copy link
Member Author

@whitequark whitequark commented Jul 2, 2020

Additional wishes:

  • Rename it to something other than UserValue, since it's going to be used internally, too; CustomValue?
  • Move all of the lazy lowering logic into Value.cast itself to avoid polluting the namespace of classes inheriting from this class as much as possible. The suggested way to go from CustomValue to Value is to call Value.cast on it.

Loading

@awygle
Copy link
Collaborator

@awygle awygle commented Jul 20, 2020

Per discussion in IRC meeting on July 7th 2020:

  • Rename UserValue to ValueCastable to better describe what it is
  • as_value is the name for the lowering method
  • as_value must be decorated with a memoizing decorator to avoid lowering multiple times to different representations
  • The memoization requirement will be enforced by ValueCastable overriding __new__

Loading

@whitequark
Copy link
Member Author

@whitequark whitequark commented Jul 20, 2020

@awygle Thanks a lot for summarizing this! Would you be up for implementing it, too?

Loading

@awygle
Copy link
Collaborator

@awygle awygle commented Jul 20, 2020

Sure. Post-0.3, I assume?

Loading

@whitequark
Copy link
Member Author

@whitequark whitequark commented Jul 20, 2020

I'd prefer to add ValueCastable in 0.3, deprecate UserValue in 0.3, and then finally pull the plug on UserValue in 0.4.

Loading

@whitequark
Copy link
Member Author

@whitequark whitequark commented Jul 20, 2020

Notably this doesn't actually prevent us from keeping Record beyond 0.4 because Record can be redesigned to keep compat with (broken) UserValue design by redefining __getattr__ to auto-cast if you are trying to call a Value method on it.

Loading

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

Successfully merging a pull request may close this issue.

None yet
3 participants