fix issue 72

[jw3126]

No description provided.

[codecov-commenter]

Codecov Report

Base: 34.05% // Head: 33.57% // Decreases project coverage by -0.49% ⚠️

Coverage data is based on head (2405543) compared to base (4d661f8).
Patch coverage: 33.33% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #73      +/-   ##
==========================================
- Coverage   34.05%   33.57%   -0.49%     
==========================================
  Files           5        5              
  Lines         138      140       +2     
==========================================
  Hits           47       47              
- Misses         91       93       +2     

Impacted Files	Coverage Δ
src/ConstructionBase.jl	50.00% <33.33%> (-1.09%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[aplavin]

This check was added in #54, see #54 (comment) for motivation.
It's always nice to throw an error with explanation instead of silently doing the wrong thing. But, if there's no "official" way to check whether properties are overloaded, maybe it's better to just drop this check altogether?
Yes, setproperty calls would do a wrong thing sometimes, but presumably it's rare. Also, maybe some "official" mechanism to check for that could be added to julia?

[jw3126]

Closing in favor of #74

[aplavin]

Generated functions docs don't make it clear at all what could be used there. which() seem innocent enough, but still it broke with recent nightlies.

[jw3126]

I agree it is blurry what is allowed in a generated function and what is not.

The @generated macro should not be used on functions mutating the global scope or depending on mutable elements.

Language lawyers might argue that which depends on mutable elements, namely the method table, but I agree it is not very clear.

[aplavin]

Yeah, then generated cannot call any other function - they all depend on the method table.

[jw3126]

Technically yes. In practice, this is only a problem if a method used in @genrated gets refined later.

[aplavin]

That's exactly the discussion we had in #60 (comment) :)
And seems like which shouldn't have any issues on this basis. And it hadn't...

[rafaqz]

I think it was just luck that it had no issues before. It's not that accessing the method table won't ever work in @generated, it's that it may break at any point, like it just has, because there is no promise that it will work.

We should avoid adding anything like that in future and be very conservative with changes. This package is widely used basic infrastructure now.

[aplavin]

I still don't see any clear difference on how the method table is accessed: via which, or via just calling any function. Do you say "there is no promise" that either of these will work in @generated?

[rafaqz]

You're arguing from a position of ignorance though, right? That's likely not a good strategy for writing stable code.

I'm arguing that we need to be conservative because I don't fully understand everything the compiler does (and don't care to), and because I know that it changes and is allowed to in this situation because of that lack of guarantee on accessing mutable state - which does include the method table.

What I'm saying is please don't break half the ecosystem again, and I'm worried you are not becoming more cautious even after this has just happened.

[jw3126]

We should avoid adding anything like that in future and be very conservative with changes. This package is widely used basic infrastructure now.

Agreed.

What I'm saying is please don't break half the ecosystem again, and I'm worried you are not becoming more cautious even after this has just happened.

Yes in retrospect this usage of which was bad. However, this comment seems rather harsh to me.

• This was on a nightly julia version, these break very often and 99% of julia users won't notice.
• @aplavin implemented the PR with the problematic code, but everybody was free to review it and request changes.
  At least I thought it was a good idea and approved it.
• @aplavin was also the one to provide a timely fix.

[aplavin]

You're arguing from a position of ignorance though, right?

No. I don't argue, but ask you (or anyone else) to explain, please re-read my previous message with a question.

I'm arguing that we need to be conservative because I don't fully understand everything the compiler does (and don't care to)

Me neither. Does this mean @generated functions are off-limits for ConstructionBase? Probably not, but I'll be glad if you show a way to get rid of them completely.

I know that it changes and is allowed to in this situation because of that lack of guarantee on accessing mutable state - which does include the method table.

AIU, all generated functions are affected by mutable state: they call other functions, reading their methods from the method table.

[rafaqz]

Honestly, I review here less because I struggle with this attitude.

I don't have time or energy to argue why function are ok but the method table isn't - its a lot of work to understand and convey exactly why that is how things are, and who has time.

But we still shouldn't access the method table even if we don't 100% understand, because the docs say not to and we clearly don't know any better.

[aplavin]

I don't have time or energy to argue why function are ok but the method table isn't

But is it? The question of @generated functions is wider than this package, and I'd like to also get the kind of confidence you have on this question.

[jw3126]

@aplavin see here JuliaLang/julia#48611 (comment)

[rafaqz]

I usually only do much simpler things in @generated than calling which. For example, using fieldnames is fixed for the session so its fair game, but propertynames is not, so it's riskier because of potential world age problems. Mostly I use @generated to build various unrolled functions from fixed type parameters where things are pretty much @pure.

This is a fairly vague heuristic that wont hold up in debate unless being conservative under uncertainty is respected as a rationale.

I have a few dodgy old packages that do bad things with generated, but that is noted in the readme.

[aplavin]

OK I found some more details myself. From https://docs.julialang.org/en/v1/manual/metaprogramming/:

Generated functions are only permitted to call functions that were defined before the definition of the generated function.

This seems to hold in this particular example, but is relevant to another issue with the same code: 8eecb33.

Generated functions must not mutate or observe any non-constant global state (including, for example, IO, locks, non-local dictionaries, or using hasmethod).

And this is directly relevant - hasmethod and which are about the same thing.

IDK, maybe the worldage issue is due to this generated function loading before vs after propertynames definition?.. But it can also load before/after the definition of a type itself, and this somehow works fine...
And fieldnames can also potentially be overriden same as propertynames, it just happens very rarely if anywhere.

Hope it will be useful for others as well.

[rafaqz]

Exactly with hasmethod. vtjnash is allowed to move these things around wherever he likes because of these caveats in the docs. That's the reason we shouldn't use them, we don't need to understand much more than that.

And overwriting fieldnames would be seriously frowned on, and will break everything else too. Changing propertynames is, in contrast, encouraged.