julia-mode: Indentation fixes #15156

justbur · 2016-02-19T22:15:00Z

Note: This needs tests, which I can write, but it would be helpful to have someone check this to make sure they agree with the indentation rules I've put in here.

Introduce julia-indent-hanging to calculate indentation following any hanging operators as defined in julia-hanging-operator-regexp or julia-hanging-assignment-regexp on the previous line. If a line follows a hanging operator increase the indentation by one increment, unless it is preceded by two hanging
operators.

Fixes #15118

yuyichao · 2016-02-19T23:13:11Z

contrib/julia-mode.el

+                        ">" "<" ">=" "<=" "==" "===" "!=" "!==" "<:" "≠" "≤" "≥"
+                        ".>" ".<" ".>=" ".<=" ".==" ".!="
+                        "&&" "||" "?"))
+          (regexp-opt '(" #" " \n" "#" "\n"))))


Are these missing many operators from the list here

Oh yes, I see there's some that are missing. I used this table because I saw this comment and wasn't sure how to interpret it for the purposes of julia-mode.

By the way, do you think it's necessary to treat assignment operators differently? I was trying to preserve the original behavior with them, but I wasn't sure if that was important.

What exactly is the special behavior?

Sorry, I should have explained. The current behavior is to increase the indent level if the previous line ends in a "=". In this PR I kept that behavior but extended it to all assignment operators. For example,

a = b = 1

For other operators, I increase the indent after the first one, but keep the indentation the same after that. For example,

a = 1 + 2 + 3 + 4

My question is is the current behavior important? Is there something wrong with this for example?

a = b = c

It seems strange to worry too much about chained assignments, but then again I don't have much experience with writing julia yet.

Thanks for the explaination, I don't really think the current behavior is particularly important (since other language modes do not do this). You can probably find why it is introduced with git-blame

Ok thanks. I'll look into it more tomorrow
On Fri, Feb 19, 2016 at 10:45 PM Yichao Yu [email protected] wrote:

In contrib/julia-mode.el
#15156 (comment):

+(defconst julia-hanging-assignment-regexp

(concat "^[^#\n]+ "

(regexp-opt '("=" "+=" "-=" "*=" "/=" "//=" "\=" "^=" "÷=" "%=" "|=" "&=" "$=" "<<=" ">>="

".+=" ".-=" ".*=" "./=" ".//=" ".\=" ".^=" ".÷=" ".%="))

(regexp-opt '(" #" " \n" "#" "\n"))))

+(defconst julia-hanging-operator-regexp

(concat "^[^#\n]+ "

(regexp-opt '("^" ".^" "//" ".//" "_" "/" "%" "&" "\" "._" "./" ".%" ".\"

"<<" ">>" ">>>" ".<<" ".>>" ".>>>"

"+" "-" "|" "$" ".+" ".-"

":" ".." "|>"

">" "<" ">=" "<=" "==" "===" "!=" "!==" "<:" "≠" "≤" "≥"

".>" ".<" ".>=" ".<=" ".==" ".!="

"&&" "||" "?"))

(regexp-opt '(" #" " \n" "#" "\n"))))

Thanks for the explaination, I don't really think the current behavior is
particularly important (since other language modes do not do this). You can
probably find why it is introduced with git-blame
https://github.com/JuliaLang/julia/blame/master/contrib/julia-mode.el

—
Reply to this email directly or view it on GitHub
https://github.com/JuliaLang/julia/pull/15156/files#r53543829.

justbur · 2016-02-20T01:25:46Z

Added another commit to fix #11559

justbur · 2016-02-20T18:07:45Z

@yuyichao I added all the operators from julia-parser.scm. I hope that doesn't cause any performance issues.

I also added a couple of tests for these rules. I'm happy to squash commits if you would like me to.

yuyichao · 2016-02-21T13:44:05Z

Hmmm, is the operator list automatically generated? Local test shows that it is missing some of the operators like .+, .-, ++. I've also notice #15173 while testing this but we probably don't really need to worry about that one for now...

justbur · 2016-02-21T20:04:50Z

Local test shows that it is missing some of the operators like .+, .-, ++.

Good catch. I missed a couple of vertical bars that needed to be removed from the translation from julia-parser.scm

Since I'm digging into the indentation code, I decided to take a crack at addressing some of the other indentation related issues too. For example, I think I found a nice way to improve the performance of indentation within parens to address #9831. That's what the latest commit attempts to do. I see big improvements on my computer (~75%) after increasing the max lookback for parens to 10k for the old version and removing it in the current version.

I was thinking about also going after gains in the block indentation procedure, but maybe this pr is getting too ambitious?

justbur · 2016-02-22T03:23:32Z

Just to note where I'm seeing the performance bottlenecks after this PR. If I profile indenting the whole lapack.jl file, for example, I get these results for the cpu portion, suggesting that julia-in-comment and julia-in-string may be called too frequently or are perhaps more expensive than they need to be.

- indent-line-to                             125,402,370  91%
 - or                                        125,401,306  91%
  - save-excursion                           122,024,042  88%
   - let                                     122,024,042  88%
    - julia-last-open-block                  119,325,182  86%
     - let                                   119,325,182  86%
      - julia-last-open-block-pos            118,999,102  86%
       - save-excursion                      118,743,742  86%
        - let                                118,743,742  86%
         - while                             118,743,742  86%
          - setq                              94,981,900  69%
           - cond                             94,981,900  69%
            - and                             61,455,884  44%
             - not                            33,650,240  24%
              - julia-in-brackets             32,009,952  23%
               - let                          32,009,952  23%
                - save-excursion              31,470,504  22%
                 - while                      31,470,504  22%
                  - if                        31,470,504  22%
                   - or                       31,470,504  22%
                    + julia-in-comment        15,935,496  11%
                    + julia-in-string         15,535,008  11%
              + julia-in-comment               1,640,288   1%
             - equal                          27,805,644  20%
                current-word                   8,168,328   5%
            - julia-at-keyword                33,526,016  24%

yuyichao · 2016-02-22T13:58:36Z

LGTM. We can merge this after squashing to 1-2 commits. Further improvements can go in other pr's.

justbur · 2016-02-22T14:22:17Z

whoops, sorry. I added one more just now. Would you like me to move it out?

On Mon, Feb 22, 2016 at 8:59 AM Yichao Yu [email protected] wrote:

LGTM. We can merge this after squashing to 1-2 commits. Further
improvements can go in other pr's.

—
Reply to this email directly or view it on GitHub
#15156 (comment).

justbur · 2016-02-22T15:10:26Z

ok, removed the last commit and squashed everything else to 2 commits. I'll start a new pr for the strings and blocks.

yuyichao · 2016-02-22T16:10:33Z

Either removing the new commit or not should be fine. Will merge after getting back to my computer and tested it locally

justbur · 2016-02-22T16:12:03Z

I removed it and opened a new pr, so nothing you recently tested should be
affected. Thanks

On Mon, Feb 22, 2016 at 11:10 AM Yichao Yu [email protected] wrote:

Either removing the new commit or not should be fine. Will merge after
getting back to my computer and tested it locally

—
Reply to this email directly or view it on GitHub
#15156 (comment).

yuyichao · 2016-02-22T18:09:12Z

I actually didn't run the test last time and only checked the indent of operators.

Looks like this patch breaks tests? (in particular, indent of parenthesis)

yuyichao% emacs -batch -L . -l ert -l julia-mode-tests.el -f  ert-run-tests-batch-and-exit
Running 15 tests (2016-02-22 13:06:03-0500)
Indenting region...
Indenting region...done
   passed   1/15  julia--test-indent-begin
Indenting region...
Indenting region...done
   passed   2/15  julia--test-indent-comment-equal
Indenting region...
Indenting region...done
   passed   3/15  julia--test-indent-else
Indenting region...
Indenting region...done
   passed   4/15  julia--test-indent-equals
Indenting region...
Indenting region...done
   passed   5/15  julia--test-indent-function
Indenting region...
Indenting region...done
   passed   6/15  julia--test-indent-if
Indenting region...
Indenting region...done
   passed   7/15  julia--test-indent-ignores-blank-lines
Indenting region...
Indenting region...done
   passed   8/15  julia--test-indent-leading-paren
Indenting region...
Indenting region...done
Indenting region...
Indenting region...done
   passed   9/15  julia--test-indent-module-keyword
Indenting region...
Indenting region...done
   passed  10/15  julia--test-indent-nested-if
Indenting region...
Indenting region...done
   passed  11/15  julia--test-indent-operator
Indenting region...
Test julia--test-indent-paren backtrace:
  julia-indent-line()
  indent-according-to-mode()
  indent-region(1 18)
  (let ((julia-indent-offset 4)) (julia-mode) (insert "\nfoobar(bar,\n
  (progn (let ((julia-indent-offset 4)) (julia-mode) (insert "\nfoobar
  (unwind-protect (progn (let ((julia-indent-offset 4)) (julia-mode) (
  (save-current-buffer (set-buffer temp-buffer) (unwind-protect (progn
  (let ((temp-buffer (generate-new-buffer " *temp*"))) (save-current-b
  (lambda nil (let ((temp-buffer (generate-new-buffer " *temp*"))) (sa
  #[0 "\306\307!r\211q\210\310\311\312\313\314\315!\316\"\317\320%DC
  funcall(#[0 "\306\307!r\211q\210\310\311\312\313\314\315!\316\"\31
  ert--run-test-internal([cl-struct-ert--test-execution-info [cl-struc
  #[0 "r\304 q\210\305 )\306\307\310\311\312\313!\314\"\315\316%DC\2
  funcall(#[0 "r\304 q\210\305 )\306\307\310\311\312\313!\314\"\315\
  ert-run-test([cl-struct-ert-test julia--test-indent-paren "We should
  ert-run-or-rerun-test([cl-struct-ert--stats t [[cl-struct-ert-test j
  ert-run-tests(t #[385 "\306\307\"\203D\211\211G\310U\203\211@\20
  ert-run-tests-batch(nil)
  ert-run-tests-batch-and-exit()
  command-line-1(("-L" "." "-l" "ert" "-l" "julia-mode-tests.el" "-f" 
  command-line()
  normal-top-level()
Test julia--test-indent-paren condition:
    (wrong-number-of-arguments
     (0 . 1)
     3)
   FAILED  12/15  julia--test-indent-paren
Indenting region...
Test julia--test-indent-paren-space backtrace:
  julia-indent-line()
  indent-according-to-mode()
  indent-region(1 20)
  (let ((julia-indent-offset 4)) (julia-mode) (insert "\nfoobar( bar,\
  (progn (let ((julia-indent-offset 4)) (julia-mode) (insert "\nfoobar
  (unwind-protect (progn (let ((julia-indent-offset 4)) (julia-mode) (
  (save-current-buffer (set-buffer temp-buffer) (unwind-protect (progn
  (let ((temp-buffer (generate-new-buffer " *temp*"))) (save-current-b
  (lambda nil (let ((temp-buffer (generate-new-buffer " *temp*"))) (sa
  #[0 "\306\307!r\211q\210\310\311\312\313\314\315!\316\"\317\320%DC
  funcall(#[0 "\306\307!r\211q\210\310\311\312\313\314\315!\316\"\31
  ert--run-test-internal([cl-struct-ert--test-execution-info [cl-struc
  #[0 "r\304 q\210\305 )\306\307\310\311\312\313!\314\"\315\316%DC\2
  funcall(#[0 "r\304 q\210\305 )\306\307\310\311\312\313!\314\"\315\
  ert-run-test([cl-struct-ert-test julia--test-indent-paren-space "We 
  ert-run-or-rerun-test([cl-struct-ert--stats t [[cl-struct-ert-test j
  ert-run-tests(t #[385 "\306\307\"\203D\211\211G\310U\203\211@\20
  ert-run-tests-batch(nil)
  ert-run-tests-batch-and-exit()
  command-line-1(("-L" "." "-l" "ert" "-l" "julia-mode-tests.el" "-f" 
  command-line()
  normal-top-level()
Test julia--test-indent-paren-space condition:
    (wrong-number-of-arguments
     (0 . 1)
     3)
   FAILED  13/15  julia--test-indent-paren-space
Indenting region...
Indenting region...done
   passed  14/15  julia--test-indent-toplevel
Indenting region...
Test julia--test-top-level-following-paren-indent backtrace:
  julia-indent-line()
  indent-according-to-mode()
  indent-region(1 30)
  (let ((julia-indent-offset 4)) (julia-mode) (insert "y1 = f(x,\n    
  (progn (let ((julia-indent-offset 4)) (julia-mode) (insert "y1 = f(x
  (unwind-protect (progn (let ((julia-indent-offset 4)) (julia-mode) (
  (save-current-buffer (set-buffer temp-buffer) (unwind-protect (progn
  (let ((temp-buffer (generate-new-buffer " *temp*"))) (save-current-b
  (lambda nil (let ((temp-buffer (generate-new-buffer " *temp*"))) (sa
  #[0 "\306\307!r\211q\210\310\311\312\313\314\315!\316\"\317\320%DC
  funcall(#[0 "\306\307!r\211q\210\310\311\312\313\314\315!\316\"\31
  ert--run-test-internal([cl-struct-ert--test-execution-info [cl-struc
  #[0 "r\304 q\210\305 )\306\307\310\311\312\313!\314\"\315\316%DC\2
  funcall(#[0 "r\304 q\210\305 )\306\307\310\311\312\313!\314\"\315\
  ert-run-test([cl-struct-ert-test julia--test-top-level-following-par
  ert-run-or-rerun-test([cl-struct-ert--stats t [[cl-struct-ert-test j
  ert-run-tests(t #[385 "\306\307\"\203D\211\211G\310U\203\211@\20
  ert-run-tests-batch(nil)
  ert-run-tests-batch-and-exit()
  command-line-1(("-L" "." "-l" "ert" "-l" "julia-mode-tests.el" "-f" 
  command-line()
  normal-top-level()
Test julia--test-top-level-following-paren-indent condition:
    (wrong-number-of-arguments
     (0 . 1)
     3)
   FAILED  15/15  julia--test-top-level-following-paren-indent

Ran 15 tests, 12 results as expected, 3 unexpected (2016-02-22 13:06:03-0500)

3 unexpected results:
   FAILED  julia--test-indent-paren
   FAILED  julia--test-indent-paren-space
   FAILED  julia--test-top-level-following-paren-indent

justbur · 2016-02-22T18:38:00Z

They pass for me in the buffer and from the command line. By the way that command from the command line needs to be run in the contrib folder of the repo.

It looks like you're getting errors, which suggests to me that you're potentially loading the wrong version of julia-mode.el, but I can't really tell from the output.

yuyichao · 2016-02-22T18:46:43Z

By the way that command from the command line needs to be run in the contrib folder of the repo.

I did, I've also tried replacing the file name and run it in a different directory to make sure I'm loading the system version and compare the result and these errors only appears when the version in this PR is used. I also see the regression when editing files.

I compile it to elc and I also have '(julia-indent-offset 4) in .emacs from testing a previous PR if these make a difference.

It looks like you're getting errors, which suggests to me that you're potentially loading the wrong version of julia-mode.el, but I can't really tell from the output.

I'm pretty sure I'm using the right version since I can see a difference when I'm using te original version.

justbur · 2016-02-22T19:08:25Z

I figured it out. backward-up-list got more options in emacs 25 (hence the wrong number of arguments error) and I didn't know that. You probably have emacs 24.

Give me some more time and I'll make it compatible with emacs 24. Sorry I didn't check that.

yuyichao · 2016-02-22T19:12:59Z

You probably have emacs 24.

24.5 from archlinux repo

Give me some more time and I'll make it compatible with emacs 24. Sorry I didn't check that.

NP. thanks for the improvements =)

Introduce julia-indent-hanging to calculate indentation following any hanging operators as defined in julia-hanging-operator-regexp on the previous line. If a line follows a hanging operator increase the indentation by one increment, unless it is preceded by two hanging operators. julia-hanging-operator-regexp is taken from julia-parser.scm. Added two tests Fixes JuliaLang#15118 Fix indentation after module kw Register "module" as a keyword that starts a block, and add to new var julia-block-start-keywords-no-indent for block keywords that should not increase the indentation level. Fixes JuliaLang#11559

Replace julia-paren-indent with a version that uses the higher level function backward-up-list to find any previous opening parentheses. This function automatically skips balanced expressions like strings, as well as comments. On some test files the time to indent falls by more than 75% with this change, even after increasing the threshold for paren lookback to 10k. Note the threshold is removed in this version.

justbur · 2016-02-22T20:17:50Z

@yuyichao I fixed the issue and tested in emacs 24.5. I think it should work for you.

yuyichao · 2016-02-22T20:34:34Z

Tested locally and it looks good. I've also verified that indenting the whole buffer is much faster and it can indent a list with a few hundred lines without giving up.

Merging before the CI finishes since the CI is not checking anything about these files, (not even the white space checker since some tests needs end-of-line space).

julia-mode: Indentation fixes

yuyichao · 2016-02-28T23:49:05Z

This is actually still a little confusing since it produces

if a &&
    b
    c
end

b and c are indented to the same level although they belongs to different part of the expression.

tkelman · 2016-02-29T02:11:17Z

I don't use julia-mode, but I'd prefer if we settled on indenting line continuations on initial if/for/etc block opener lines one level further than the rest of the block contents.

justbur · 2016-02-29T13:13:23Z

I don't use julia-mode, but I'd prefer if we settled on indenting line continuations on initial if/for/etc block opener lines one level further than the rest of the block contents.

I like that suggestion, and it's easy to do

yuyichao reviewed Feb 19, 2016
View reviewed changes

justbur force-pushed the julia-mode-indent branch from 4e7927a to 9e3074f Compare February 20, 2016 17:55

justbur force-pushed the julia-mode-indent branch 2 times, most recently from 63eed4e to 0100288 Compare February 21, 2016 20:03

justbur force-pushed the julia-mode-indent branch from 0100288 to 203a3da Compare February 21, 2016 20:06

justbur force-pushed the julia-mode-indent branch from 3b039cd to 7cf74ab Compare February 22, 2016 13:40

yuyichao added the julia-mode label Feb 22, 2016

justbur changed the title ~~julia-mode: Indent after "hanging" operators~~ julia-mode: Indentation fixes Feb 22, 2016

justbur force-pushed the julia-mode-indent branch from 0e7d3c9 to 9f7277e Compare February 22, 2016 15:09

justbur added 2 commits February 22, 2016 15:16

justbur force-pushed the julia-mode-indent branch from 9f7277e to 0d4de5a Compare February 22, 2016 20:16

yuyichao added a commit that referenced this pull request Feb 22, 2016

Merge pull request #15156 from justbur/julia-mode-indent

20b33da

julia-mode: Indentation fixes

yuyichao merged commit 20b33da into JuliaLang:master Feb 22, 2016

yuyichao mentioned this pull request Feb 28, 2016

julia-mode.el Indentation for long lists of keyword function arguments #9831

Closed

yuyichao mentioned this pull request Apr 1, 2016

Indentation after importing operators JuliaEditorSupport/julia-emacs#12

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

julia-mode: Indentation fixes #15156

julia-mode: Indentation fixes #15156

justbur commented Feb 19, 2016

yuyichao Feb 19, 2016

justbur Feb 20, 2016

yuyichao Feb 20, 2016

justbur Feb 20, 2016

yuyichao Feb 20, 2016

justbur Feb 20, 2016

justbur commented Feb 20, 2016

justbur commented Feb 20, 2016

yuyichao commented Feb 21, 2016

justbur commented Feb 21, 2016

justbur commented Feb 22, 2016

yuyichao commented Feb 22, 2016

justbur commented Feb 22, 2016

justbur commented Feb 22, 2016

yuyichao commented Feb 22, 2016

justbur commented Feb 22, 2016

yuyichao commented Feb 22, 2016

justbur commented Feb 22, 2016

yuyichao commented Feb 22, 2016

justbur commented Feb 22, 2016

yuyichao commented Feb 22, 2016

justbur commented Feb 22, 2016

yuyichao commented Feb 22, 2016

yuyichao commented Feb 28, 2016

tkelman commented Feb 29, 2016

justbur commented Feb 29, 2016

julia-mode: Indentation fixes #15156

julia-mode: Indentation fixes #15156

Conversation

justbur commented Feb 19, 2016

yuyichao Feb 19, 2016

Choose a reason for hiding this comment

justbur Feb 20, 2016

Choose a reason for hiding this comment

yuyichao Feb 20, 2016

Choose a reason for hiding this comment

justbur Feb 20, 2016

Choose a reason for hiding this comment

yuyichao Feb 20, 2016

Choose a reason for hiding this comment

justbur Feb 20, 2016

Choose a reason for hiding this comment

justbur commented Feb 20, 2016

justbur commented Feb 20, 2016

yuyichao commented Feb 21, 2016

justbur commented Feb 21, 2016

justbur commented Feb 22, 2016

yuyichao commented Feb 22, 2016

justbur commented Feb 22, 2016

justbur commented Feb 22, 2016

yuyichao commented Feb 22, 2016

justbur commented Feb 22, 2016

yuyichao commented Feb 22, 2016

justbur commented Feb 22, 2016

yuyichao commented Feb 22, 2016

justbur commented Feb 22, 2016

yuyichao commented Feb 22, 2016

justbur commented Feb 22, 2016

yuyichao commented Feb 22, 2016

yuyichao commented Feb 28, 2016

tkelman commented Feb 29, 2016

justbur commented Feb 29, 2016