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

Change hint propagation for user-defined structs. #434

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

ztangent
Copy link
Member

Finishes the implementation of Construct and GetField started by @georgematheos in #367. Also adds tests, and some logic to handle cases where types have non-default constructors (although this is not exhaustive, because we can't prevent users from writing pathological constructors).

@ztangent
Copy link
Member Author

@marcoct Any thoughts on how / where we should document these changes? I can't think of a natural place it fits in the docs.

@marcoct
Copy link
Collaborator

marcoct commented Jul 14, 2021

@ztangent I think Extending Gen would make sense.

@marcoct
Copy link
Collaborator

marcoct commented Jul 14, 2021

@ztangent This looks great! My only comments are:

  • Adding documentation

  • Is there a simple description for what subset of struct and constructor combinations this feature works on? If the description is complex (maybe it is not?) I wonder if it might make sense to make the definition more strict and reject other cases?

I'll also create another issue for adding support for AD through these structs.

@ztangent
Copy link
Member Author

ztangent commented Jul 20, 2021

Is there a simple description for what subset of struct and constructor combinations this feature works on? If the description is complex (maybe it is not?) I wonder if it might make sense to make the definition more strict and reject other cases?

This feature works on any structs which can be "constructed-by-field", i.e. if the struct is defined as:

struct Foo
    field1
    field2
    field3
    ...
end

then in order to propagate per-field changes, Construct should be used with a constructor foo = Foo(arg1, arg2, arg3, ...) such that foo.field1 == arg1, foo.field2 == arg2, and so on. The default constructor of a user-defined type adheres to this property.

Construct also tries to detect if it is being used with a compatible constructor by checking the number and type signature of the arguments it is called with. In cases where a non-compatible constructor can be detected (e.g. the number of arguments is different from the number of fields, or the argument types cannot be converted to the corresponding field types), any changes to one of the inputs always results in an UnknownChange(), which is safe behavior. But there are pathological constructors that meet the type signature requirements, but are not field-constructable, e.g.:

struct EvilStruct
    x::Int
    y::Int
    EvilStruct(x, y) = new(y, x)
end

Unfortunately, it doesn't seem like there's any way to detect cases like this without looking up the IR of the constructor and doing some fancy analysis. So type-based solution I came up with is "complete but unsound", in that it will work without complaint for any constructor, but for some constructors changes will be propagated inaccurately. I would prefer a "sound but incomplete" approach, but as of now, I think the only way to do that is to always return "UnknownChange", because there's no way to distinguish constructor like EvilStruct(x, y) = new(y, x) from a default constructor without inspecting IR.

It'd be great if you have thoughts on the best approach given these constraints @marcoct !

@ztangent
Copy link
Member Author

Added documentation here! Here's how I've stated the scope of Construct for now. It technically works safely (though not usefully) on more constructors, but I suppose it's best to just tell users what constructors they should stick to:

Note that that Construct should be used with constructors T(args...) where the nth argument corresponds to the nth field of the type T. Default constructors meet this requirement. Other constructors are not guaranteed to propagate changes correctly.

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.

4 participants