- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
deprecate f(instance::MyType)
or warn on additional method, for f(x) = f(typeof(x))
#34985
Comments
I agree with the idea, but I think the impact is too large. |
Preventing incorrect trait definitions should not have any impact in bug-free code, and it would find bugs like the one referenced. Though I don't think there's any mechanism for preventing it at the moment. It might be the job for a linter. |
What are the criteria for correct or incorrect? |
See
https://github.com/JuliaCollections/IterTools.jl/pull/70/files#diff-bd068feabf42c1d394ba76bc98a4d738L934
for
an example.
…On Wed, Nov 25, 2020, 8:11 PM kimikage ***@***.***> wrote:
What are the criteria for correct or incorrect?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#34985 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEN3M6EQPJR6FHJXEI7YTDSRWTNFANCNFSM4LATLVVQ>
.
|
A related reply on discourse from @StefanKarpinski :
|
I suggest either removing any definition other than
f(::Type{...})
, or warning if someone adds a new method that isn'tf(::Type{...})
.After reading
julia/base/generator.jl
Lines 83 to 87 in ddf904c
IteratorSize(::MyType)=SizeUnknown()
, which worked in some situations, and then broke when some library calledIteratorSize(MyType)
and got the default (HasLength()
). At first I suspected something was wrong with [how I understand] Generators, so I went to go check another example of aSizeUnknown()
iterator used in a Generator, and funny enough I found the same bug inIterTools
: https://github.com/JuliaCollections/IterTools.jl/pull/70/files#diff-bd068feabf42c1d394ba76bc98a4d738L934If there weren't these parallel definitions, or if there were a warning if someone adds a new method IteratorSize(::not_Type{....}) (so to speak), then this bug wouldn't be possible. The bugs are pretty small, but so is the convenience of not writing out
typeof
. It seems like removing the parallel definitions would be consistent with what's been done for e.g.fieldnames
(see related issues below).Here is a [possibly not exhaustive] list of such parallel definitions in Base:
I'd say the case for removing these methods for the trait interfaces (
IteratorSize
,IteratorEltype
, ...) is pretty strong. And that the case for the convenience ofeltype(my_vector)
is pretty strong too, so I'm not sure.Related:
#34788 (comment)
#22350
The text was updated successfully, but these errors were encountered: