Skip to content

Fix broadcast error when eltype is inconsistent with getindex #39185

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

Merged
merged 3 commits into from
Jan 29, 2021

Conversation

nalimilan
Copy link
Member

In that case we can infer the return type as Union{}, which triggers a type assertion error. This used to work in Julia 1.5. (I'm not sure this kind of thing is legitimate but this is probably not the best place to throw an error about it anyway.)

Fixes #38422.

In that case we can infer the return type as `Union{}`, which triggers
a type assertion error. This used to work in Julia 1.5.
@nalimilan nalimilan added bugfix This change fixes an existing bug broadcast Applying a function over a collection backport 1.6 Change should be backported to release-1.6 labels Jan 10, 2021
@nalimilan nalimilan requested a review from mbauman January 11, 2021 08:30
@mbauman
Copy link
Member

mbauman commented Jan 11, 2021

Wait, so the compiler infers Union{} and doesn't completely crap out when it actually gets a value?

@mbauman
Copy link
Member

mbauman commented Jan 11, 2021

Oooh, I see. It's because we use (and trust) eltype when guessing the return type, but don't actually do that when evaluating the expression.

Further, we don't call getindex(x::Cyclotomic, i) from within Broadcast; it's just getindex(x::Cyclotomic).

@mbauman
Copy link
Member

mbauman commented Jan 11, 2021

Maybe we should just ask inference for the getindex too since we're already in the business of pestering inference?

Co-authored-by: Simeon Schaub <simeondavidschaub99@gmail.com>
@nalimilan
Copy link
Member Author

Maybe we should just ask inference for the getindex too since we're already in the business of pestering inference?

Indeed. In general, assuming that getindex is inferrable, inference should give the same result as the current system which is more complex. See #39295.

Though I think it would be safer not to make that change in 1.6, so better backport this one, and only merge #39295 to master?

Co-authored-by: Matt Bauman <mbauman@juliacomputing.com>
@KristofferC KristofferC mentioned this pull request Jan 19, 2021
60 tasks
@nalimilan
Copy link
Member Author

@mbauman @KristofferC Should we merge and backport this? #39295 seems to be harder to implement, and too disruptive for 1.6 anyway.

@mbauman
Copy link
Member

mbauman commented Jan 29, 2021

Yes, this seems reasonable to me — especially as a patch for 1.6's behavior.

@mbauman mbauman merged commit 750f42a into master Jan 29, 2021
@mbauman mbauman deleted the nl/broadcast branch January 29, 2021 13:19
KristofferC pushed a commit that referenced this pull request Feb 1, 2021
* Fix broadcast error when eltype is inconsistent with getindex

In that case we can infer the return type as `Union{}`, which triggers
a type assertion error. This used to work in Julia 1.5.

* whitespace

Co-authored-by: Simeon Schaub <simeondavidschaub99@gmail.com>

* Update test/broadcast.jl

Co-authored-by: Matt Bauman <mbauman@juliacomputing.com>

Co-authored-by: Matt Bauman <mbauman@juliacomputing.com>
Co-authored-by: Simeon Schaub <simeondavidschaub99@gmail.com>
(cherry picked from commit 750f42a)
KristofferC pushed a commit that referenced this pull request Feb 1, 2021
* Fix broadcast error when eltype is inconsistent with getindex

In that case we can infer the return type as `Union{}`, which triggers
a type assertion error. This used to work in Julia 1.5.

* whitespace

Co-authored-by: Simeon Schaub <simeondavidschaub99@gmail.com>

* Update test/broadcast.jl

Co-authored-by: Matt Bauman <mbauman@juliacomputing.com>

Co-authored-by: Matt Bauman <mbauman@juliacomputing.com>
Co-authored-by: Simeon Schaub <simeondavidschaub99@gmail.com>
(cherry picked from commit 750f42a)
@KristofferC KristofferC mentioned this pull request Feb 1, 2021
10 tasks
@KristofferC KristofferC removed the backport 1.6 Change should be backported to release-1.6 label Feb 2, 2021
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
…ang#39185)

* Fix broadcast error when eltype is inconsistent with getindex

In that case we can infer the return type as `Union{}`, which triggers
a type assertion error. This used to work in Julia 1.5.

* whitespace

Co-authored-by: Simeon Schaub <simeondavidschaub99@gmail.com>

* Update test/broadcast.jl

Co-authored-by: Matt Bauman <mbauman@juliacomputing.com>

Co-authored-by: Matt Bauman <mbauman@juliacomputing.com>
Co-authored-by: Simeon Schaub <simeondavidschaub99@gmail.com>
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
…ang#39185)

* Fix broadcast error when eltype is inconsistent with getindex

In that case we can infer the return type as `Union{}`, which triggers
a type assertion error. This used to work in Julia 1.5.

* whitespace

Co-authored-by: Simeon Schaub <simeondavidschaub99@gmail.com>

* Update test/broadcast.jl

Co-authored-by: Matt Bauman <mbauman@juliacomputing.com>

Co-authored-by: Matt Bauman <mbauman@juliacomputing.com>
Co-authored-by: Simeon Schaub <simeondavidschaub99@gmail.com>
staticfloat pushed a commit that referenced this pull request Dec 23, 2022
* Fix broadcast error when eltype is inconsistent with getindex

In that case we can infer the return type as `Union{}`, which triggers
a type assertion error. This used to work in Julia 1.5.

* whitespace

Co-authored-by: Simeon Schaub <simeondavidschaub99@gmail.com>

* Update test/broadcast.jl

Co-authored-by: Matt Bauman <mbauman@juliacomputing.com>

Co-authored-by: Matt Bauman <mbauman@juliacomputing.com>
Co-authored-by: Simeon Schaub <simeondavidschaub99@gmail.com>
(cherry picked from commit 750f42a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
broadcast Applying a function over a collection bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type assert error in broadcasting on nightly
5 participants