-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 static_show()
print valid identifiers
#38049
Conversation
This addresses |
220c1f0
to
9404ef7
Compare
68758b7
to
02c8a96
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First commit looks great! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second commit looks great! --- modulo a couple things I don't entirely grok; let's chat offline :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Third commit looks great! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confusion resolved. Looks great! :)
Absent objections or requests for time, I plan to merge this pull request on Saturday morning PT. Best! :) |
02c8a96
to
4f1b398
Compare
src/rtutils.c
Outdated
|
||
char *sn = jl_symbol_name(name); | ||
int hidden = 0; | ||
if (!jl_is_identifier(sn) || jl_is_operator(sn)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the isoperator
check really needed? For example this gives typeof(Base.:(var"+"))
instead of typeof(Base.:(+))
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops! thanks Jeff. I really think i meant to make this if (!(jl_is_identifier(sn) || jl_is_operator(sn)))
. Because already jl_is_identifier("+")
returns false, so i think i added this specifically to allow operators to be okay, but i screwed up the parens. That's what i get for coding late at night 😅 thanks - this is a great catch!
julia> @ccall jl_is_identifier("+"::Cstring)::Bool
false
julia> @ccall(jl_is_identifier("+"::Cstring)::Bool) || @ccall(jl_is_operator("+"::Cstring)::Bool)
true
i've amended the commit to fix this, and also added an expected output test for this case. 👍 thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now fixed. Does this sound right to you? Thanks.
Previously, `static_show` was printing invalid identifiers for `TypeVar`s that contain a `#`, such as those produced by `gensym()`. For example: ```julia julia> println(static_shown(Vector{<:Bool})) Array{#s5, 1} where #s5<:Bool julia> eval(Meta.parse(static_shown(Vector{<:Bool}))) ERROR: syntax: incomplete: premature end of input ``` After this commit, this is printed as a valid identifier: ```julia julia> println(static_shown(Vector{<:Bool})) Array{var"#s5", 1} where var"#s5"<:Bool julia> eval(Meta.parse(static_shown(Vector{<:Bool}))) Vector{var"#s5"} where var"#s5"<:Bool (alias for Array{var"#s5", 1} where var"#s5"<:Bool) ``` As you can see, this mirrors the printing done from julia's `show()`. This commit also applies the same technique to simplify the printing for Type names that contain a `#`, simply for asthetic reasons and consistency with julia `show()`, by reusing the above functionality. Before: ```julia julia> struct var"#X#" x::Int end julia> println(static_shown(var"#X#")) getfield(Main, Symbol("#X#")) julia> println(static_shown(var"#X#"(0))) getfield(Main, Symbol("#X#"))(x=0) ``` After: ```julia julia> struct var"#X#" x::Int end julia> println(static_shown(var"#X#")) Main.var"#X#" julia> println(static_shown(var"#X#"(0))) Main.var"#X#"(x=0) ```
Previously, function instances would be printed in `static_show()` as elements of a `DataType`, with no special printing for `<:Function`s. This led to printing them via _invalid_ syntax, since there is no empty constructor for `T<:Function`. Before: ```julia julia> g() = 2 g (generic function with 1 method) julia> println(static_shown(g)) typeof(Main.g)() julia> eval(Meta.parse(static_shown(g))) ERROR: MethodError: no method matching typeof(g)() ``` After: ```julia julia> g() = 2 g (generic function with 1 method) julia> println(static_shown(g)) Main.g julia> eval(Meta.parse(static_shown(g))) g (generic function with 1 method) ``` I chose to keep the Module prefix on the functions, so that they will be valid julia expressions that you can enter at the REPL to reproduce the function instance, even though this is a break from the julia `show()` behavior. One caveat is that this is still not correct for anonymous functions, but I'm also not really sure what would be a better way to print anonymous functions... I think this is _probably_ better than it was before, but very open to suggestions for how to improve it. Before: ```julia julia> l = (x,y)->x+y julia> println(static_shown(l)) getfield(Main, Symbol("JuliaLang#3#4"))() julia> eval(Meta.parse(static_shown(l))) ERROR: MethodError: no method matching var"JuliaLang#3#4"() ``` After: ```julia julia> l = (x,y)->x+y julia> println(static_shown(l)) Main.var"JuliaLang#3" julia> eval(Meta.parse(static_shown(l))) ERROR: UndefVarError: JuliaLang#3 not defined ```
Previously, it was only escaping names with a `#` in them, but now it will escape all names that are illegal identifiers. Before: ```julia julia> struct var"a%b+c" x::Int end julia> println(static_shown(var"a%b+c")) Main.a%b+c julia> println(static_shown(var"a%b+c"(1))) Main.a%b+c(x=1) ``` After: ```julia julia> struct var"a%b+c" x::Int end julia> println(static_shown(var"a%b+c")) Main.var"a%b+c" julia> println(static_shown(var"a%b+c"(1))) Main.var"a%b+c"(x=1) ```
I was particularly confused when reading the code between the block for instances of DataTypes and instances of types that are DataTypes, so I hope these comments will help future readers be less confused. :)
4f1b398
to
720863d
Compare
…(v)` This fixes a segfault introduced in JuliaLang#38049. Co-Authored-By: Sacha Verweij <sacha.verweij@alumni.stanford.edu>
This PR makes a few separate changes to
static_show()
, to bring it more in line with julia'sshow()
, and to fix cases where it was printing non-valid code.var""
syntax in static_show to print names containing invalid characters as valid identifiers.Commit messages follow:
Use
var""
syntax in static_show to print valid identifiersPreviously,
static_show
was printing invalid identifiers forTypeVar
s that contain a#
, such as those produced bygensym()
.For example:
After this commit, this is printed as a valid identifier:
As you can see, this mirrors the printing done from julia's
show()
.This commit also applies the same technique to simplify the printing for
Type names that contain a
#
, simply for asthetic reasons andconsistency with julia
show()
, by reusing the above functionality.Before:
After:
Make functions print as valid julia expressions.
Previously, function instances would be printed in
static_show()
aselements of a
DataType
, with no special printing for<:Function
s.This led to printing them via invalid syntax, since there is no empty
constructor for
T<:Function
.Before:
After:
I chose to keep the Module prefix on the functions, so that they will
be valid julia expressions that you can enter at the REPL to reproduce
the function instance, even though this is a break from the julia
show()
behavior.One caveat is that this is still not correct for anonymous functions,
but I'm also not really sure what would be a better way to print
anonymous functions... I think this is probably better than it was
before, but very open to suggestions for how to improve it.
Before:
After:
Extend
static_show()
escaping to any illegal identifier via var"".Previously, it was only escaping names with a
#
in them, but now itwill escape all names that are illegal identifiers.
Before:
After: