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

Reconsider use of Setfield #215

Closed
davidanthoff opened this issue Dec 16, 2019 · 5 comments
Closed

Reconsider use of Setfield #215

davidanthoff opened this issue Dec 16, 2019 · 5 comments

Comments

@davidanthoff
Copy link
Member

@tfk, is Setfield.jl doing more stuff like jw3126/Setfield.jl#113? I see the value it brings to the table, but on the other hand I think we should not take dependencies here that use especially clever, but at the end brittle, tricks like that one...

@tkf
Copy link
Contributor

tkf commented Dec 16, 2019

I'd say @jw3126 is generally very careful about API compliance. The only API "violation" I can recall in Setfield.jl is these use of generic functions in the generator body of ConstructionBase.setproperties. But these generic functions (fieldnames, map, and issubset) are pretty low-level in Base so I'm not worrying too much. We are discussing this and tried other options JuliaObjects/ConstructionBase.jl#21.

@davidanthoff
Copy link
Member Author

Hm, ok... I am worried, I have to admit though, that it looks like Setfield is really trying to push what can be done with the compiler and seems to rely on a fair number of pretty complicated stuff (that I don't even understand from glancing at it briefly). I think it is quite normal that such things take time to bake and get stable, but for a pretty run of the mill package like VegaLite it seems not super ideal to have a dependency on something like that. Let's leave it in for now, but maybe we can keep an eye on it and I'd say that if we run into a similar issue like this crash via the show method we reconsider.

@tkf
Copy link
Contributor

tkf commented Dec 17, 2019

Setfield is a clever package but, compared to (say) crazy auto-diff packages, I'd say it uses pretty standard tech (the most advanced one being @generated).

BTW, as I mentioned in JuliaLang/julia#29428 (comment), it is pretty easy to invoke this without any overload of show(::IO, ::Type{<:MyType). I think it'd be ideal to make VS code extension side more robust, at the same time removing the dangerous code from Setfield.jl. I know at least Zygote does similar overload for show.

@davidanthoff
Copy link
Member Author

Yeah, I'm trying to come up with a way to make the extension more robust, but it is tricky when things segfault...

@jw3126
Copy link

jw3126 commented Dec 17, 2019

I am curious, why do you need to depend on Setfield? Does ConstructionBase not suffice?

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

No branches or pull requests

3 participants