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

Fix show output for time/date Period types #1

Closed
wants to merge 4 commits into from

Conversation

fchorney
Copy link
Owner

Fixing the show(), repr(), print(), string() functions for Period types (Hour, Day, Minute, etc)

if get(io, :compact, false)
print(io, p)
else
print(io, $(string(T)), '(', p.value, ')')

Choose a reason for hiding this comment

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

No need to call string here:

print(io, $T, '(', p.value, ')')

Copy link
Owner Author

Choose a reason for hiding this comment

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

I tried that, but the problem here is as follows (if I removed that string):

julia> import Dates

julia> show(Dates.Hour(1))
Dates.Hour(1)

julia> show(Hour(1))
ERROR: UndefVarError: Hour not defined
Stacktrace:
 [1] top-level scope at REPL[2]:1
julia> using Dates

julia> show(Dates.Hour(1))
Hour(1)

julia> show(Hour(1))
Hour(1)

but if I put the string there, then in both situations (import vs using) it will just show Hour instead of Dates.Hour.

Copy link
Owner Author

Choose a reason for hiding this comment

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

The reason I came across this is because in the tests it would be outputting "Dates.X" rather than just "X"

Choose a reason for hiding this comment

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

Try just T instead of $T

Copy link

Choose a reason for hiding this comment

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

The results you show above are correct, that's what should be happening

Copy link

Choose a reason for hiding this comment

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

But what Curtis said is better yeah

Copy link
Owner Author

Choose a reason for hiding this comment

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

hm well perhaps that DateTime string should also be changed in a similar manner

Copy link

@omus omus Jan 22, 2019

Choose a reason for hiding this comment

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

You may also want to consider making:

const BitPeriod = Union{Year, Month, Week, Day, Hour, Minute, Second, Millisecond, Microsecond, Nanosecond}

(mirrors: Base.BitInteger)

Copy link

Choose a reason for hiding this comment

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

I was just pointing out that with using, either way it would return "Hour" instead of "Dates.Hour"

Which is fine, because after using Dates you can access it as Hour instead of Dates.Hour :)

Copy link
Owner Author

Choose a reason for hiding this comment

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

yeah that makes sense.

end

function Base.print(io::IO, p::Period)
print(io, string(p))

Choose a reason for hiding this comment

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

string is never needed inside of print, as it's defined as (effectively) sprint(print, x). So this can be just print(io, p). Also, the separate method for plain text shouldn't be necessary.

Copy link
Owner Author

Choose a reason for hiding this comment

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

ah right. ok

Copy link
Owner Author

Choose a reason for hiding this comment

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

hmm if this is just print(io, p) then wouldn't it just call itself forever?

Choose a reason for hiding this comment

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

Oh you're right. Is the definition here necessary or does the fallback look as you'd expect?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Guess that depends on how we want it. Without that print command that calls string, then show(x), print(x), repr(x) all show "Dates.Hour(1)" with x = Dates.Hour(1), and then string(x) is "1 hour"

Copy link
Owner Author

Choose a reason for hiding this comment

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

with that print call that calls string, then string(x) and print(x) would show "1 hour"


function Base.show(io::IO, ::MIME"text/plain", p::Period)
print(io, p)
end
Copy link

Choose a reason for hiding this comment

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

Can be a one-liner:

Base.show(io::IO, ::MIME"text/plain", p::Period) = print(io, p)

- Given `x = Dates.Day(1)`
 - The REPL will print: `1 day`
 - show will print: `Dates.Day(1)`
 - repr will print: `Dates.Day(1)`
 - print will print: `1 day`
 - string will print: `1 day`
 - dataframes will print: `1 day`
@omus
Copy link

omus commented Jan 30, 2019

Can be closed now.

@fchorney fchorney closed this Feb 13, 2019
@fchorney
Copy link
Owner Author

JuliaLang#30817 merged into master

@fchorney fchorney deleted the fc/fix-time-repr branch February 13, 2019 22:12
fchorney pushed a commit that referenced this pull request Jan 15, 2020
…#32605)

The bug here is a bit subtle, but perhaps best illustrated with
the included test case:
```
function f32579(x::Int64, b::Bool)
    if b
        x = nothing
    end
    if isa(x, Int64)
        y = x
    else
        y = x
    end
    if isa(y, Nothing)
        z = y
    else
        z = y
    end
    return z === nothing
end
```
The code just after SSA conversion looks like:
```
2  1 ─       goto JuliaLang#3 if not _3
3  2 ─ %2  = Main.nothing::Core.Compiler.Const(nothing, false)
5  3 ┄ %3  = φ (JuliaLang#2 => %2, #1 => _2)::Union{Nothing, Int64}
   │   %4  = (%3 isa Main.Int64)::Bool
   └──       goto JuliaLang#5 if not %4
6  4 ─ %6  = π (%3, Int64)
   └──       goto JuliaLang#6
8  5 ─ %8  = π (%3, Nothing)
10 6 ┄ %9  = φ (JuliaLang#4 => %6, JuliaLang#5 => %8)::Union{Nothing, Int64}
   │   %10 = (%9 isa Main.Nothing)::Bool
   └──       goto JuliaLang#8 if not %10
11 7 ─ %12 = π (%9, Nothing)
   └──       goto JuliaLang#9
13 8 ─ %14 = π (%9, Int64)
15 9 ┄ %15 = φ (JuliaLang#7 => %12, JuliaLang#8 => %14)::Union{Nothing, Int64}
   │   %16 = (%15 === Main.nothing)::Bool
   └──       return %16
```
Now, we have special code in SROA (despite it not really being an
SROA transform) that looks at `===` and replaces
it by a nest of phis of booleans. The reasoning for this transform
is that it eliminates a use of a value where we only care about the
type and not the content, thus making it more likely that the value
will subsequently be eligible for SROA. In addition, while it goes
along resolving which values feed into any particular phi, it
accumulates and type conditions it encounters along the way.

Thus in the example above, something like the following happens:
- We look at %14, which πs to %9 with an Int64 constraint, so we only
  consider the JuliaLang#4 predecessor for %9 (due to the constraint), until
  we get to %3, where we again only consider the #1 predecessor,
  where we find the argument (of type Int64) and conclude the result
  is always false
- Now we pop the next item of the stack from our original phi, look
  at %12, which πs to %9 with a Nothing constraint.

At this point we used to terminate the search because we already looked
at %9. However, crucially, we looked at %9 only with an Int64 constraint,
so we missed the fact that `nothing` was in fact a possible value for this
phi. The result was a missing entry in the generated phi node:
```
1 ─       goto JuliaLang#3 if not b
2 ─ %2  = Main.nothing::Core.Compiler.Const(nothing, false)
3 ┄ %3  = φ (#1 => false)::Bool
│   %4  = φ (JuliaLang#2 => %2, #1 => _2)::Union{Nothing, Int64}
│   %5  = (%4 isa Main.Int64)::Bool
└──       goto JuliaLang#5 if not %5
4 ─ %7  = π (%4, Int64)
└──       goto JuliaLang#6
5 ─ %9  = π (%4, Nothing)
6 ┄ %10 = φ (JuliaLang#4 => %3, JuliaLang#5 => %3)::Bool
│   %11 = φ (JuliaLang#4 => %7, JuliaLang#5 => %9)::Union{Nothing, Int64}
│   %12 = (%11 isa Main.Nothing)::Bool
└──       goto JuliaLang#8 if not %12
7 ─       goto JuliaLang#9
8 ─       nothing::Nothing
9 ┄ %16 = φ (JuliaLang#7 => %10, JuliaLang#8 => %10)::Bool
└──       return %16
```
(note the missing JuliaLang#2 predecessor in phi node %3), which would result
in an undefined value at runtime, though in this case LLVM would
have taken advantage of that to just return 0:
```
define i8 @julia_f32579_16051(i64, i8) {
top:
;  @ REPL[1]:15 within `f32579'
  ret i8 0
}
```
Compare this now to the optimized IR with this patch:
```
1 ─       goto JuliaLang#3 if not b
2 ─ %2  = Main.nothing::Core.Compiler.Const(nothing, false)
3 ┄ %3  = φ (JuliaLang#2 => true, #1 => false)::Bool
│   %4  = φ (JuliaLang#2 => %2, #1 => _2)::Union{Nothing, Int64}
│   %5  = (%4 isa Main.Int64)::Bool
└──       goto JuliaLang#5 if not %5
4 ─ %7  = π (%4, Int64)
└──       goto JuliaLang#6
5 ─ %9  = π (%4, Nothing)
6 ┄ %10 = φ (JuliaLang#4 => %3, JuliaLang#5 => %3)::Bool
│   %11 = φ (JuliaLang#4 => %7, JuliaLang#5 => %9)::Union{Nothing, Int64}
│   %12 = (%11 isa Main.Nothing)::Bool
└──       goto JuliaLang#8 if not %12
7 ─       goto JuliaLang#9
8 ─       nothing::Nothing
9 ┄ %16 = φ (JuliaLang#7 => %10, JuliaLang#8 => %10)::Bool
└──       return %16
```
The %3 phi node has its missing entry and the generated LLVM code
correctly returns `b`:
```
define i8 @julia_f32579_16112(i64, i8) {
top:
  %2 = and i8 %1, 1
;  @ REPL[1]:15 within `f32579'
  ret i8 %2
}
```
fchorney pushed a commit that referenced this pull request Jun 10, 2020
This reuses the machinery that gives good stacktraces when a user
calls wait on a Task. Since bind internally uses _wait2, this
machinery is bypassed currently.

Currently, julia throws an uninformative stacktrace in this situation:

julia> c = Channel(_ -> error("foo"))
Channel{Any}(sz_max:0,sz_curr:0)

julia> for i in c end
ERROR: foo
Stacktrace:
 [1] iterate(::Channel{Any}) at ./channels.jl:459
 [2] top-level scope at ./REPL[2]:1

With this change, the stacktrace is much more informative:

julia> for i in c end
ERROR: TaskFailedException:
foo
Stacktrace:
 [1] error(::String) at ./error.jl:33
 [2] (::var"#1#2")(::Channel{Any}) at ./REPL[4]:1
 [3] (::Base.var"JuliaLang#652#653"{var"#1#2",Channel{Any}})() at ./channels.jl:129
Stacktrace:
 [1] check_channel_state at ./channels.jl:167 [inlined]
 [2] take_unbuffered(::Channel{Any}) at ./channels.jl:405
 [3] take! at ./channels.jl:383 [inlined]
 [4] iterate(::Channel{Any}, ::Nothing) at ./channels.jl:449
 [5] iterate(::Channel{Any}) at ./channels.jl:448
 [6] top-level scope at ./REPL[5]:1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants