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

Make it easier to avoid StackOverflowErrors with promotion and convert #19918

Merged
merged 1 commit into from
Jan 9, 2017

Conversation

timholy
Copy link
Member

@timholy timholy commented Jan 7, 2017

This guards against errors that can lead to stack overflows. It resolves a mutually-recursive interaction between T(x) and convert(T, x) for T==Union{}, and more significantly introduces a new function, promote_noncircular(x, y, z...), that throws an error if promotion didn't change at least one of the types. That prevents stack overflows for definitions like

foo{T}(x::T, y::T) = # "real" definition of foo
foo(x, y) = foo(promote_noncircular(x, y)...)

promote_noncircular is somewhat similar to (but more general than) promote_to_supertype. It is not quite as @pure, so the existing mechanism is also retained. At some point we may want to think about exporting this (or a variant), but for now this is a purely internal method.

Fixes #12007, fixes #10326, fixes #15736

@timholy
Copy link
Member Author

timholy commented Jan 7, 2017

Also worth noting that we still have good codegen:

julia> @code_llvm flipsign(1,Int8(1))

define i64 @julia_flipsign_62360(i64, i8) #0 !dbg !5 {
top:
  %2 = sext i8 %1 to i64
  %3 = ashr i64 %2, 63
  %4 = add i64 %3, %0
  %5 = xor i64 %4, %3
  ret i64 %5
}

Still, we should @nanosoldier runbenchmarks(ALL, vs = ":master").

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

@timholy
Copy link
Member Author

timholy commented Jan 7, 2017

I can't reproduce any of those regressions, so I think this is good to go. Merge? Perhaps the name promote_noncircular could use some bikeshedding, particularly since I suspect packages will want to call it (exported or not). Certainly some of mine will 😄.

@pabloferz
Copy link
Contributor

safe_promote?

@timholy
Copy link
Member Author

timholy commented Jan 7, 2017

That suggestion has merit, and I even thought of it ahead of time. I went with promote_noncircular as being a bit more specific about how it was being safe. But I could easily be persuaded to change, if most others prefer it.

@KristofferC
Copy link
Member

I think safe is a bit too strong and doesn't really say what is unsafe with the normal convert so I like noncircular a bit more.

@kshyatt kshyatt added error handling Handling of exceptions by Julia or the user types and dispatch Types, subtyping and method dispatch labels Jan 8, 2017
…sion

promoted_noncircular is somewhat similar to (but more general than) promote_to_supertype. It is not quite as atsign-pure, so the existing mechanism is also retained.

Fixes #12007, fixes #10326, fixes #15736
@timholy timholy merged commit 8ec47d1 into master Jan 9, 2017
@timholy timholy deleted the teh/safe_promote branch January 9, 2017 14:45
@JeffBezanson
Copy link
Member

On my branch, the convert error doesn't work, since calling convert(Union{}, x) is ambiguous.

@timholy
Copy link
Member Author

timholy commented Jan 9, 2017

Sorry to steal a couple of issue-closings from you, but presumably you'll close so many when that merges you won't notice 😉. (Obviously feel free to modify those tests as needed.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error handling Handling of exceptions by Julia or the user types and dispatch Types, subtyping and method dispatch
Projects
None yet
6 participants