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

disallow unrecognized string/char escapes #22800

Merged
merged 1 commit into from Jul 14, 2017

Conversation

Projects
None yet
6 participants
@stevengj
Member

stevengj commented Jul 13, 2017

As discussed in #21284, this PR disallows a backslash in string or character literals before unrecognized escape characters.

Besides the standard escapes that produce a different character (e.g. \n), the only allowed escapes where \* produces * are now for * ∈ {',",`,$,\}.

@stevengj

This comment has been minimized.

Show comment
Hide comment
@stevengj

stevengj Jul 13, 2017

Member

(Note that I found a few bugs this way.)

Member

stevengj commented Jul 13, 2017

(Note that I found a few bugs this way.)

@stevengj

This comment has been minimized.

Show comment
Hide comment
@stevengj

stevengj Jul 14, 2017

Member

I don't really understand the Travis failures. One appears to be a timeout, whereas the other is the mysterious:

	From worker 1:	compile: Test Failed
  Expression: Foo.g() === 97.0
   Evaluated: 2.0 === 97.0
Stacktrace:
 [1] (::Test62Main_compile.##1#14)() at /tmp/julia/share/julia/test/compile.jl:159

AppVeyor and FreeBSD and OSX Travis succeeded; is there some problem with Linux Travis now?

Member

stevengj commented Jul 14, 2017

I don't really understand the Travis failures. One appears to be a timeout, whereas the other is the mysterious:

	From worker 1:	compile: Test Failed
  Expression: Foo.g() === 97.0
   Evaluated: 2.0 === 97.0
Stacktrace:
 [1] (::Test62Main_compile.##1#14)() at /tmp/julia/share/julia/test/compile.jl:159

AppVeyor and FreeBSD and OSX Travis succeeded; is there some problem with Linux Travis now?

@vtjnash

This comment has been minimized.

Show comment
Hide comment
@vtjnash

vtjnash Jul 14, 2017

Member

We've seen that one before. It seems to suggest we're losing a backedge at some point (stochastically), but we haven't reproduced it yet locally.

Member

vtjnash commented Jul 14, 2017

We've seen that one before. It seems to suggest we're losing a backedge at some point (stochastically), but we haven't reproduced it yet locally.

@JeffBezanson

This comment has been minimized.

Show comment
Hide comment
@JeffBezanson

JeffBezanson Jul 14, 2017

Member

Travis is failing in all kinds of ways lately.

Member

JeffBezanson commented Jul 14, 2017

Travis is failing in all kinds of ways lately.

@JeffBezanson JeffBezanson merged commit 7cb02cd into JuliaLang:master Jul 14, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
julia freebsd ci Build done
Details

@stevengj stevengj deleted the stevengj:invalidescapes branch Jul 14, 2017

@StefanKarpinski

This comment has been minimized.

Show comment
Hide comment
@StefanKarpinski

StefanKarpinski Jul 17, 2017

Member

The fact that this revealed a bunch of bugs is a pretty good indicator that this was a good idea.

Member

StefanKarpinski commented Jul 17, 2017

The fact that this revealed a bunch of bugs is a pretty good indicator that this was a good idea.

@stevengj

This comment has been minimized.

Show comment
Hide comment
@stevengj

stevengj Jul 19, 2017

Member

There was a suggestion on discourse that this should be changed to a warning.

On the one hand, it seems like virtually all the errors that are popping up are bugs. On the other hand, we normally make changes like this a warning for a while...

Member

stevengj commented Jul 19, 2017

There was a suggestion on discourse that this should be changed to a warning.

On the one hand, it seems like virtually all the errors that are popping up are bugs. On the other hand, we normally make changes like this a warning for a while...

@JeffBezanson

This comment has been minimized.

Show comment
Hide comment
@JeffBezanson

JeffBezanson Jul 19, 2017

Member

These are very easy to fix, and the fixed code is both backwards and forwards compatible, so it doesn't seem so crucial to me to give a warning.

Member

JeffBezanson commented Jul 19, 2017

These are very easy to fix, and the fixed code is both backwards and forwards compatible, so it doesn't seem so crucial to me to give a warning.

@StefanKarpinski

This comment has been minimized.

Show comment
Hide comment
@StefanKarpinski

StefanKarpinski Jul 19, 2017

Member

Especially since when someone upgrades to 0.7, they should fix all warnings and then upgrade to 1.0, at which point this would fail anyway. There's even a case to be made that 0.7 should have depwarn=error on by default, but that's a different discussion.

Member

StefanKarpinski commented Jul 19, 2017

Especially since when someone upgrades to 0.7, they should fix all warnings and then upgrade to 1.0, at which point this would fail anyway. There's even a case to be made that 0.7 should have depwarn=error on by default, but that's a different discussion.

samoconnor added a commit to JuliaCloud/AWSSDK.jl that referenced this pull request Aug 20, 2017

samoconnor added a commit to JuliaCloud/AWSCore.jl that referenced this pull request Aug 20, 2017

@ChrisRackauckas

This comment has been minimized.

Show comment
Hide comment
@ChrisRackauckas

ChrisRackauckas Apr 6, 2018

Contributor

Did this cause math blocks to no longer work?

"""
    calculate_residuals!(out, ũ, u₀, u₁, α, ρ)

Save element-wise residuals
```math
\frac{ũ}{α+\max{|u₀|,|u₁|}*ρ}
```
in `out`.
"""
function calculate_residuals!(out, ũ, u₀, u₁, α, ρ, internalnorm)
   @. out = ũ / (α + max(internalnorm(u₀), internalnorm(u₁)) * ρ)
end
ERROR: syntax: invalid escape sequence
Contributor

ChrisRackauckas commented Apr 6, 2018

Did this cause math blocks to no longer work?

"""
    calculate_residuals!(out, ũ, u₀, u₁, α, ρ)

Save element-wise residuals
```math
\frac{ũ}{α+\max{|u₀|,|u₁|}*ρ}
```
in `out`.
"""
function calculate_residuals!(out, ũ, u₀, u₁, α, ρ, internalnorm)
   @. out = ũ / (α + max(internalnorm(u₀), internalnorm(u₁)) * ρ)
end
ERROR: syntax: invalid escape sequence
@KristofferC

This comment has been minimized.

Show comment
Hide comment
@KristofferC

KristofferC Apr 6, 2018

Contributor

Just escape the backslash?

Contributor

KristofferC commented Apr 6, 2018

Just escape the backslash?

@ChrisRackauckas

This comment has been minimized.

Show comment
Hide comment
@ChrisRackauckas

ChrisRackauckas Apr 6, 2018

Contributor

Having to go and escape the TeX code in every docstring is a pretty heavy handed workaround. For normal strings you'd just use LaTeXStrings.jl. Is there no simple way to write TeX in docstrings now? (Do string macros work on docstrings? I guess I never checked)

Contributor

ChrisRackauckas commented Apr 6, 2018

Having to go and escape the TeX code in every docstring is a pretty heavy handed workaround. For normal strings you'd just use LaTeXStrings.jl. Is there no simple way to write TeX in docstrings now? (Do string macros work on docstrings? I guess I never checked)

@stevengj

This comment has been minimized.

Show comment
Hide comment
@stevengj

stevengj Apr 6, 2018

Member

Note that \frac didn't work previously, either; it would have given frac since \f was equivalent to f. So, throwing an error instead caught a bug.

However, you can just use doc"..." to avoid the need for backslash escapes.

doc"""
    calculate_residuals!(out, ũ, u₀, u₁, α, ρ)

Save element-wise residuals
```math
\frac{ũ}{α+\max{|u₀|,|u₁|}*ρ}
```
in `out`.
"""
Member

stevengj commented Apr 6, 2018

Note that \frac didn't work previously, either; it would have given frac since \f was equivalent to f. So, throwing an error instead caught a bug.

However, you can just use doc"..." to avoid the need for backslash escapes.

doc"""
    calculate_residuals!(out, ũ, u₀, u₁, α, ρ)

Save element-wise residuals
```math
\frac{ũ}{α+\max{|u₀|,|u₁|}*ρ}
```
in `out`.
"""
@stevengj

This comment has been minimized.

Show comment
Hide comment
@stevengj

stevengj Apr 6, 2018

Member

The manual has a brief comment about this:

@doc_str should only be used when the docstring contains $ or \ characters that should not be parsed by Julia such as LaTeX syntax or Julia source code examples containing interpolation.

Member

stevengj commented Apr 6, 2018

The manual has a brief comment about this:

@doc_str should only be used when the docstring contains $ or \ characters that should not be parsed by Julia such as LaTeX syntax or Julia source code examples containing interpolation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment