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

#16071: restrict typed variable declaration to local x::T and global x::T #17125

Closed
wants to merge 1 commit into from

Conversation

jamesonquinn
Copy link
Contributor

This is my first Julia pull request, so tell me if I'm doing it wrong. This is a fix to #16071. My deprecation message is:

          (syntax-deprecation s
            (string "Type statement on left hand side of assignment.")
            (string "Use with `local` or `global` first.")))

Also, this doesn't catch the case where the LHS is in parentheses or otherwise complicated (such as tuple assignment where only the first tuple item is typed.)

@fcard
Copy link
Contributor

fcard commented Jun 26, 2016

When deprecating something, you should look for uses of it on the codebase and fix them. Also, tests!

@tkelman tkelman added the needs tests Unit tests are required for this change label Jun 29, 2016
@vtjnash
Copy link
Member

vtjnash commented Jul 18, 2016

It's easier and more accurate to do this during lowering (julia-syntax.scm) rather than parsing. That also allows macros to continue using the syntax, and keeps the parser simpler (context-unaware), but changes the interpretation of that syntax later. For example, here I think we would just need to catch the branch where this construct is handled during lowering:

diff --git a/src/julia-syntax.scm b/src/julia-syntax.scm
index 770aaae..c52b34e 100644
--- a/src/julia-syntax.scm
+++ b/src/julia-syntax.scm
@@ -1828,7 +1828,13 @@
           (let ((x (cadr lhs))
                 (T (caddr lhs))
                 (rhs (caddr e)))
-            (let ((e (remove-argument-side-effects x)))
+            (let* ((e (remove-argument-side-effects x))
+                   (str-x (deparse x))
+                   (str-T (deparse T))
+                   (str-lhs (string str-x "::" str-T " = ...")))
+              (if (symbol? x)
+                  (syntax-deprecation #f str-lhs (string "local " str-lhs " = ..."))
+                  (syntax-deprecation #f str-lhs (string "typeassert(" str-x ", " str-T "); " str-x " = ...")))
               (expand-forms
                `(block ,@(cdr e)
                        (decl ,(car e) ,T)

(fwiw, your PR looked fine otherwise, just starting off in the wrong direction on where to make the change)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs tests Unit tests are required for this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants