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 static_show() print valid identifiers #38049

Merged
merged 4 commits into from
Oct 26, 2020

Conversation

NHDaly
Copy link
Member

@NHDaly NHDaly commented Oct 15, 2020

This PR makes a few separate changes to static_show(), to bring it more in line with julia's show(), and to fix cases where it was printing non-valid code.

  • Use var"" syntax in static_show to print names containing invalid characters as valid identifiers.
  • Start printing function instances by instance name, instead of via a non-existent type constructor.

Commit messages follow:


Use var"" syntax in static_show to print valid identifiers

Previously, static_show was printing invalid identifiers for
TypeVars that contain a #, such as those produced by gensym().

For example:

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> 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> 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> 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)

Make functions print as valid julia expressions.

Previously, function instances would be printed in static_show() as
elements of a DataType, with no special printing for <:Functions.
This led to printing them via invalid syntax, since there is no empty
constructor for T<:Function.

Before:

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> 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> l = (x,y)->x+y

julia> println(static_shown(l))
getfield(Main, Symbol("#3#4"))()

julia> eval(Meta.parse(static_shown(l)))
ERROR: MethodError: no method matching var"#3#4"()

After:

julia> l = (x,y)->x+y

julia> println(static_shown(l))
Main.var"#3"

julia> eval(Meta.parse(static_shown(l)))
ERROR: UndefVarError: #3 not defined

Extend static_show() escaping to any illegal identifier via var"".

Previously, it was only escaping names with a # in them, but now it
will escape all names that are illegal identifiers.

Before:

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> 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)

@NHDaly
Copy link
Member Author

NHDaly commented Oct 15, 2020

This addresses Case 2 in this issue about emitting invalid precompile statements: #28808

@NHDaly NHDaly force-pushed the nhd-static_show branch 2 times, most recently from 220c1f0 to 9404ef7 Compare October 16, 2020 03:52
@NHDaly NHDaly marked this pull request as ready for review October 16, 2020 03:52
@NHDaly NHDaly force-pushed the nhd-static_show branch 2 times, most recently from 68758b7 to 02c8a96 Compare October 16, 2020 15:05
@Sacha0 Sacha0 self-requested a review October 16, 2020 20:41
@Sacha0 Sacha0 added the compiler:precompilation Precompilation of modules label Oct 16, 2020
Copy link
Member

@Sacha0 Sacha0 left a 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! :)

Copy link
Member

@Sacha0 Sacha0 left a 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 :).

src/julia.h Outdated Show resolved Hide resolved
Copy link
Member

@Sacha0 Sacha0 left a 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! :)

Copy link
Member

@Sacha0 Sacha0 left a 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! :)

@Sacha0
Copy link
Member

Sacha0 commented Oct 22, 2020

Absent objections or requests for time, I plan to merge this pull request on Saturday morning PT. Best! :)

src/rtutils.c Outdated Show resolved Hide resolved
src/rtutils.c Outdated

char *sn = jl_symbol_name(name);
int hidden = 0;
if (!jl_is_identifier(sn) || jl_is_operator(sn)) {
Copy link
Sponsor Member

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.:(+)).

Copy link
Member Author

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!

Copy link
Member Author

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. :)
@JeffBezanson JeffBezanson merged commit 031c877 into JuliaLang:master Oct 26, 2020
@NHDaly NHDaly deleted the nhd-static_show branch October 26, 2020 18:30
NHDaly added a commit to NHDaly/julia that referenced this pull request Oct 26, 2020
NHDaly added a commit to NHDaly/julia that referenced this pull request Nov 11, 2020
…(v)`

This fixes a segfault introduced in JuliaLang#38049.

Co-Authored-By: Sacha Verweij <[email protected]>
vtjnash pushed a commit that referenced this pull request Nov 19, 2020
vchuravy pushed a commit that referenced this pull request Jan 21, 2021
…(v)` (#38399)

This fixes a segfault introduced in #38049.

(cherry picked from commit b602577)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:precompilation Precompilation of modules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants