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

Confusing printing of Expr #9474

Closed
yuyichao opened this issue Dec 27, 2014 · 12 comments
Closed

Confusing printing of Expr #9474

yuyichao opened this issue Dec 27, 2014 · 12 comments

Comments

@yuyichao
Copy link
Contributor

While exploring the AST, I've found a few cases where the printed Expr is really confusing

A few examples that I can remember now are

julia> :(1::Int64 == 1::Int64::Bool)
:(1::Int64 == 1::Int64::Bool)

julia> :((1::Int64 == 1::Int64)::Bool)
:(1::Int64 == 1::Int64::Bool)

julia> :(A <: B)
:(A <: B)

julia> Expr(:(<:), :A, :B)
:(A <: B)

I agree that the best way to see the content of a Expr is probably using dump or Meta.show_sexpr but it'll be nice if the default display in REPL is not misleading...

@StefanKarpinski
Copy link
Member

This is definitely an issue sometimes. We should probably just parenthesize more aggressively when showing typed ASTs since that's really mainly for debugging use anyway.

@toivoh
Copy link
Contributor

toivoh commented Dec 27, 2014

We do have parentheses based on operator precedence, but maybe :: hasn't
been included in that machinery yet?

The second issue comes from the fact that <: is represented differently in
the AST depending on if it's a comparison or a bound, and will be hard to
deal with unless we begin to distinguish different contexts that an AST can
be printed in (e.g. expression / type (parameter) name / function argument
(for :kw)).

@yuyichao
Copy link
Contributor Author

For the first one I don't really mind seeing more parenthesis than necessary (which only makes it closer to lisp) although only adding them when necessary would be better. I thought the operator precedence is also defined in julia itself. Is the printing getting that information from somewhere else instead?

For the second one, maybe :(A <: B) can be printed as :(<:(A, B))? It will still be a little bit confusing mainly because the parsing is context dependent but at least it would be possible to distinguish them.

@yuyichao
Copy link
Contributor Author

The second one confuses me when I try to construct a type declaration in a macro with Expr(:type, :(A <: B), body). The result looks perfectly fine but it doesn't execute. The solution I proposed above can at least help with this problem.

I guess a better error message when :(A <: B) is used in the wrong context might also help.

@yuyichao
Copy link
Contributor Author

Another example I've noticed while tracking down another bug report (not for Expr though) is the following

julia> AbstractArray{TypeVar(:T), 1}.name
AbstractArray

julia> Base.SetPartitions.parameters[1].ub
AbstractArray{T,1}

julia> AbstractArray{TypeVar(:T), 1}
AbstractArray{T,1}

julia> typeof(Base.SetPartitions.parameters[1].ub)
TypeConstructor

julia> typeof(AbstractArray{TypeVar(:T), 1})
DataType

@stevengj
Copy link
Member

@toivoh, yes, the syntactic-operators in julia-parser.scm are currently not included in the operator-precedence function. This should be easily fixable.

No wait, the :: operator is included in the precedence table, and the syntactic operators are parsed specially so that isn't the problem. Something else is happening here.

@stevengj
Copy link
Member

I'm confused by the second example. What is the problem with the :(A <: B) printing?

@stevengj
Copy link
Member

With the first example, it is a trivial fix: we have the operator precedence but weren't using it:

--- a/base/show.jl
+++ b/base/show.jl
@@ -424,7 +424,7 @@ function show_unquoted(io::IO, ex::Expr, indent::Int, prec::

     # infix (i.e. "x<:y" or "x = y")
     elseif (head in expr_infix && nargs==2) || (is(head,:(:)) && nargs==3)
-        show_list(io, args, head, indent, 0, true)
+        show_list(io, args, head, indent, operator_precedence(head), true)

     elseif head in expr_infix_wide && nargs == 2
         func_prec = operator_precedence(head)

This is presumably because the operator_precedence function is more recent than most of the expression-showing code.

Or rather, the expr_infix should probably be merged with the expr_infix_wide handling.

@stevengj
Copy link
Member

Oh, I see the problem with A <: B ... the difficulty is that the same expression parses two different ways depending on context: as a comparison expression (normally) or as a <: expression (in type declarations etc). I think this is a separate issue — I would prefer to parse A <: B as a single expression type independent of context. It's not a question of printing per se.

@stevengj
Copy link
Member

A related bug:

julia> :((1:2:3) + 4)
:(1:2:3 + 4)

@stevengj
Copy link
Member

The misparenthesized infix output should be fixed. I've opened a separate issue for the A <: B parsing inconsistency.

@yuyichao
Copy link
Contributor Author

Thanks for the fix.

As long as there's no concern about backward compatibility etc, I would prefer parsing A <: B consistently too. (I guess the discussion should continue in the new issue...)

stevengj added a commit that referenced this issue May 23, 2015
(cherry picked from commit 5219315)
closes #11346

Conflicts:
	base/show.jl
	test/show.jl
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

No branches or pull requests

4 participants