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

Require all tuples in eachindex to have the same length. #48125

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

Conversation

navdeeprana
Copy link

Potential fix for #47898

@brenhinkeller brenhinkeller added the collections Data structures holding multiple items, e.g. sets label Jan 9, 2023
base/tuple.jl Outdated Show resolved Hide resolved
Co-authored-by: Jerry Ling <[email protected]>
base/tuple.jl Outdated Show resolved Hide resolved
base/tuple.jl Outdated
@inline
max(length(t), _maxlength(t2, t3...))
lent = length(t)
all(==(lent), t2) || throw_eachindex_mismatch_indices((), t, t2...)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
all(==(lent), t2) || throw_eachindex_mismatch_indices((), t, t2...)
all(t -> length(t)==lent, t2) || throw_eachindex_mismatch_indices((), t, t2...)

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah sorry I was stupid

@vtjnash vtjnash added the forget me not PRs that one wants to make sure aren't forgotten label Feb 13, 2023
@navdeeprana
Copy link
Author

any updates on this pr?

@Moelf
Copy link
Contributor

Moelf commented Aug 2, 2023

basically my first proposed change was bad, so you need to revert to I believe

all(t -> length(t)==lent, t2)

@navdeeprana
Copy link
Author

basically my first proposed change was bad, so you need to revert to I believe

all(t -> length(t)==lent, t2)

I did not implement the changes you suggested, so I thought it was good to go.

@Moelf
Copy link
Contributor

Moelf commented Aug 10, 2023

@oscardssmith did: 6e3e170

so you need to fix that.

You can always see the full change this PR current has by heading towards: https://github.com/JuliaLang/julia/pull/48125/files

@navdeeprana
Copy link
Author

Okay done.

base/tuple.jl Outdated
function keys(t::Tuple, t2::Tuple...)
@inline
OneTo(_maxlength(t, t2...))
@noinline function throw_eachindex_mismatch_indices(::Tuple{}, inds...)
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the purpose of the first ::Tuple{} parameter?

Copy link
Member

Choose a reason for hiding this comment

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

I think it resolves dispatch:

julia> methods(Base.throw_eachindex_mismatch_indices)
# 2 methods for generic function "throw_eachindex_mismatch_indices" from Base:
 [1] throw_eachindex_mismatch_indices(::IndexCartesian, inds...)
     @ abstractarray.jl:324
 [2] throw_eachindex_mismatch_indices(::IndexLinear, inds...)
     @ abstractarray.jl:321

It's not obvious to me that one couldn't use the IndexLinear dispatch but this seems fine.

base/tuple.jl Outdated
function keys(t::Tuple, t2::Tuple...)
@inline
OneTo(_maxlength(t, t2...))
@noinline function throw_eachindex_mismatch_indices(::Tuple{}, inds...)
Copy link
Member

Choose a reason for hiding this comment

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

I think it resolves dispatch:

julia> methods(Base.throw_eachindex_mismatch_indices)
# 2 methods for generic function "throw_eachindex_mismatch_indices" from Base:
 [1] throw_eachindex_mismatch_indices(::IndexCartesian, inds...)
     @ abstractarray.jl:324
 [2] throw_eachindex_mismatch_indices(::IndexLinear, inds...)
     @ abstractarray.jl:321

It's not obvious to me that one couldn't use the IndexLinear dispatch but this seems fine.

@timholy
Copy link
Member

timholy commented Aug 16, 2023

Let's rerun CI here (by closing and reopening) and then it looks good to merge.

@timholy timholy closed this Aug 16, 2023
@timholy timholy reopened this Aug 16, 2023
base/tuple.jl Outdated Show resolved Hide resolved
@navdeeprana navdeeprana reopened this Aug 17, 2023
@navdeeprana navdeeprana reopened this Aug 17, 2023
@timholy
Copy link
Member

timholy commented Aug 18, 2023

This looks like it worked (I haven't checked very carefully but the failures seem unrelated). If you're re-purposing the LinearIndices variant I think you should delete the method you're not calling.

@navdeeprana
Copy link
Author

Done.

.. So it's OK to define methods that call other methods that haven't yet been defined, as long as we don't actually trigger that call until later.

So that's why it doesn't fail during build with IndexLinear().

@timholy
Copy link
Member

timholy commented Aug 19, 2023

I'm tempted to merge this but a bit concerned about the failures on apple. They seem identical to one another, and in a quick survey of recent PRs I don't see the same error. You aren't by any chance running on apple, are you @navdeeprana? If so, can you run the same test locally and check?

@navdeeprana
Copy link
Author

No, I am on linux. :(

@timholy
Copy link
Member

timholy commented Aug 20, 2023

Let's just close and reopen and see if it happens again.

@timholy timholy closed this Aug 20, 2023
@timholy timholy reopened this Aug 20, 2023
@timholy
Copy link
Member

timholy commented Aug 21, 2023

Apple failed again, perhaps suggesting it's a consequence of this PR. If so, we can't merge this until that's fixed. Can anyone with a mac investigate why?

@timholy timholy added the DO NOT MERGE Do not merge this PR! label Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
collections Data structures holding multiple items, e.g. sets DO NOT MERGE Do not merge this PR! forget me not PRs that one wants to make sure aren't forgotten
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants