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

only allow certain characters after interpolated vars #25234

Merged
merged 5 commits into from
Dec 25, 2017

Conversation

stevengj
Copy link
Member

@stevengj stevengj commented Dec 21, 2017

This closes #25231.

That is, an interpolated variable $x in a string literal must be followed by a character in a set S of codepoints that will never be allowed in an identifier. Currently, S = { spaces/control chars, ASCII/Latin1 non-connector punctuation, ` }.

Copy link
Member

@StefanKarpinski StefanKarpinski left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks for doing this!

@StefanKarpinski
Copy link
Member

Seems there's some trailing whitespace that snuck into src/flisp/julia_extensions.c.

@@ -258,3 +258,6 @@ end
# added ⟂ to operator precedence (#24404)
@test Meta.parse("a ⟂ b ⟂ c") == Expr(:comparison, :a, :⟂, :b, :⟂, :c)
@test Meta.parse("a ⟂ b ∥ c") == Expr(:comparison, :a, :⟂, :b, :∥, :c)

# only allow certain characters after interpolated vars (#25231)
@test Meta.parse("\"\$x෴ \"",raise=false) == Expr(:error, "interpolated variable \$x ends with invalid character \"෴\"; use \"\$(x)\" instead.")
Copy link
Member Author

Choose a reason for hiding this comment

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

(U+0df4) looks just like my signature 😉 .

@@ -2073,7 +2073,7 @@
(string.join (string-split s a) b))

(define (ends-interpolated-atom? c)
(or (opchar? c) (never-identifier-char? c)))
(or (eof-object? c) (opchar? c) (never-identifier-char? c)))
Copy link
Member

Choose a reason for hiding this comment

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

I don't really understand this – is there a test case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the replcompletions test exercises this; the issue was that you need Base.incomplete_tag(Meta.parse("\"\$foo", raise=false)) to return :string.

If you don't check eof-object? here, then parsing the incomplete string "$foo will raise a syntax error for because $foo is terminated by an eof, instead of raising an incomplete error.

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose we could add an explicit @test Base.incomplete_tag(Meta.parse("\"\$foo", raise=false)) == :string too.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, makes sense. Can't hurt to have the explicit test here.

@StefanKarpinski StefanKarpinski merged commit 4b74a74 into JuliaLang:master Dec 25, 2017
@stevengj stevengj deleted the interpblacklist branch December 25, 2017 22:26
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

Successfully merging this pull request may close these issues.

string interpolation identifier character blacklist
2 participants