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 #27352, disallow print/string of nothing #27829

Merged
merged 1 commit into from
Jun 28, 2018
Merged

Conversation

JeffBezanson
Copy link
Member

fixes #27352

@JeffBezanson JeffBezanson added breaking This change will break code display and printing Aesthetics and correctness of printed representations of objects. labels Jun 27, 2018
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

Intended to make this comment visible 2 days ago, but apparently it got stuck in Github's review queue :(

@@ -557,6 +557,7 @@ function show(io::IO, tn::Core.TypeName)
end

show(io::IO, ::Nothing) = print(io, "nothing")
print(io::IO, ::Nothing) = throw(MethodError(print, (io, nothing)))
Copy link
Member

Choose a reason for hiding this comment

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

I think almost any other error type would be better. As it is, this will make us print:
“No method matching print(nothing). Did you mean print(nothing) instead?”

ararslan added a commit that referenced this pull request Jul 8, 2018
Currently anything that's `dump`ed that contains `nothing` will error,
since there's no logic in place for handling `nothing` in `dump` after
PR #27829.
ararslan added a commit that referenced this pull request Jul 8, 2018
Currently anything that's `dump`ed that contains `nothing` will error,
since there's no logic in place for handling `nothing` in `dump` after
PR #27829.
@iamed2
Copy link
Contributor

iamed2 commented Jul 9, 2018

This is a breaking change to the beta; this should at very least be deprecated first. This changes makes every print of a Union{T, Nothing} possibly invalid. Given that print and show sometimes have different behaviour (and this behaviour can be introduced by the user for any user-defined type), there are going to be cases in user code where this happens.

This also contradicts the documented behaviour of print:

Write to io (or to the default output stream STDOUT if io is not given) a canonical (un-decorated) text representation of a value if there is one, otherwise call show.

By that definition, there doesn't seem to be any room for a case where print itself would error, it should always call show if it doesn't know how to handle something.

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Jul 9, 2018

What if the show fallback were to wrap its output in backticks or something to indicate that it was not just a plain string? That would mean that the printing of nothing would be sufficiently weird to avoid any chance of it accidentally being spliced into a command or something in a way that calls a command or opens a file called nothing.

ararslan added a commit that referenced this pull request Jul 9, 2018
Currently anything that's `dump`ed that contains `nothing` will error,
since there's no logic in place for handling `nothing` in `dump` after
PR #27829.
@JeffBezanson
Copy link
Member Author

I don't think that helps much, except in that it's much less likely for a filename with backticks to exist. Or we could handle this by only giving an error in backtick interpolation, instead of print in general.

@ararslan
Copy link
Member

ararslan commented Jul 9, 2018

Or we could handle this by only giving an error in backtick interpolation, instead of print in general.

I had assumed that was the original plan. 👍 to that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code display and printing Aesthetics and correctness of printed representations of objects.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interpolation of nothing should be an error
5 participants