Skip to content

make eltype throw for some known noniterators #58122

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

Closed

Conversation

nsajko
Copy link
Contributor

@nsajko nsajko commented Apr 15, 2025

Neither Nothing or Missing are iterators, iterate(nothing) throws: thus eltype should throw for such arguments as well.

@nsajko nsajko added missing data Base.missing and related functionality iteration Involves iteration or the iteration protocol needs pkgeval Tests for all registered packages should be run with this change labels Apr 15, 2025
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe there is anything about not implementing iteration or getindex which implies that eltype needs to throw or return anything particularly special

@vtjnash
Copy link
Member

vtjnash commented Apr 15, 2025

The PR also breaks a handful of tests, so it is probably not worth considering

@nsajko
Copy link
Contributor Author

nsajko commented Apr 15, 2025

I don't believe there is anything about not implementing iteration or getindex which implies that eltype needs to throw or return anything particularly special

It's better to throw than to return an incorrect result silently. Any possible return value is incorrect, given that a non-iterator doesn't have an element type. Thus throwing seems to be left as the only option.

The PR also breaks a handful of tests, so it is probably not worth considering

I haven't yet looked through all or even most of these, but I'm not disheartened by what I've seen yet. Some of the test failures are benign things, such as @test_throws test that check for a hardcoded error message.

@ararslan
Copy link
Member

ararslan commented May 8, 2025

Any possible return value is incorrect, given that a non-iterator doesn't have an element type.

I'm not sure that's true in general: surely there are examples in the ecosystem of containers that don't implement iteration but have a meaningful notion of element(s) and thus element type.

There's also nothing special about nothing and missing in this regard. For example, iterate is not implemented for Symbol but eltype works.

It's better to throw than to return an incorrect result silently.

I'm somewhat ambivalent about how nothing is handled here but IMO eltype(missing) === Any makes sense. A missing value means there is a value, we just don't know what it is. That value could be a scalar or container with a meaningful element type, but we don't know for certain, so Any is a logical answer.

I haven't looked at the failing tests but this feels like it has the potential to be more breaking than it appears. FWIW, the generic fallback for eltype returning Any in the absence of a more specific method dates back to 2013, before the release of Julia v0.2. I have to imagine that it's become a pretty deep-seated assumption.

nsajko and others added 2 commits May 9, 2025 00:56
Neither `Nothing` or `Missing` are iterators, `iterate(nothing)`
throws: thus `eltype` should throw for such arguments as well.
@nsajko nsajko force-pushed the noniterator_has_no_element_type branch from a970a01 to ca285fe Compare May 8, 2025 22:57
@nsajko
Copy link
Contributor Author

nsajko commented May 8, 2025

eltype(missing) === Any makes sense

I agree, now that you pointed it out, given the special, missing data, semantics of missing. However I still think something like eltype(Missing) should throw, given that Missing is not an iterator type.

I'm not sure that's true in general: surely there are examples in the ecosystem of containers that don't implement iteration but have a meaningful notion of element(s) and thus element type.

In that case I guess such a "container" would better use some other function instead of eltype. Perhaps I'm not seeing the big picture or something, but as far as I understand eltype is a basic part of the iteration interface, as documented in Interfaces in the Manual. So any other use of eltype would be harmful punning.

@ararslan
Copy link
Member

Can a function not be part of two interfaces?

@martinholters
Copy link
Member

Any possible return value is incorrect, given that a non-iterator doesn't have an element type.

Eh, Union{} might be considered a correct return value, indicating that trying to obtain an element will error.

@nsajko nsajko closed this May 15, 2025
@nsajko nsajko deleted the noniterator_has_no_element_type branch May 15, 2025 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iteration Involves iteration or the iteration protocol missing data Base.missing and related functionality needs pkgeval Tests for all registered packages should be run with this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants