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

python-mode snippets are expanded with wrong indentation if there is more than one level of indentation in the snippet #485

Closed
lgp171188 opened this issue May 31, 2014 · 14 comments

Comments

@lgp171188
Copy link

I have installed yasnippet-20140514.1649 from melpa on Emacs 24.3.1 (packaged in Debian unstable). In python-mode, when I try expanding snippets like prop, props, propsg, the indentation is wrong. I checked the snippet file and the indentation is correct there, but the expansion has the wrong indentation like listed below

def foo():
    doc = """Doc string"""
    def fget(self):
        return self._foo

        def fset(self, value):
            self._foo = value

            def fdel(self):
                del self._foo
                return locals()
                foo = property(**foo())

instead of

def def foo():
    doc = """Doc string"""
    def fget(self):
        return self._foo
    def fset(self, value):
        self._foo = value
    def fdel(self):
        del self._foo
    return locals()
foo = property(**foo())

which is the correct indentation. This issue reproducible on all the listed commands even when running emacs with -Q option and manually loading just the yasnippet package. Since I am a developer writing code mostly in python, this makes yasnippet totally unusable for me. So would love to see this issue fixed.

@joaotavora
Copy link
Owner

At first sight this appears to be an emacs bug. If you cycle through the lines and type M-x indent-according-to-mode you get the same result, but if you tweak del self._foo to say something different, for example, the line following it will not indent deeply. I will try with 24.3 when I have the time.

@lgp171188
Copy link
Author

Yes, running M-x indent-according-to-mode on each line one by one produces the same result. I have no idea of emacs internals, but is it the right operation to call on each line of the expansion? Because when I press tab in python-mode and there are multiple indentation levels possible, pressing tab will move between the levels. So won't it be a good idea to get the indentation of the current line and add it to the indentation of all the lines in the snippet so that the indentation is maintained? I am asking this because when there are multiple indentation levels possible in python-mode, M-x indent-according-to-mode will just choose the first of those and further tabs will just make it cycle across the other levels. This makes sense because only the developer will know the level of indentation to make based on the logic of the code when there are multiple levels possible.

For example, just have a look at this source code that has to be indented.

if True:
doSomething()
elif <some condition>:
if <condition>:
doSomething1()
else:
doSomething2()

It can be indented as either

if True:
    doSomething()
elif <some condition>:
    if <condition>:
        doSomething1()
    else:
        doSomething2()

or

if True:
    doSomething()
elif <some condition>:
    if <condition>:
        doSomething1()
else:
    doSomething2()

The correct one has to be chosen by the developer who writes the code and Emacs doesn't have a clue on what is correct. It will just pick one of the indentation levels to start the cycling of tabs on pressing tab key each time.

Similarly for functions, which is the case here,

def fun1():
do something
def fun2():
do something else

can be indented either as

def fun1():
    do something
def fun2():
    do something else

where these are 2 different functions or as

def fun1():
    do something
    def fun2():
        do something else

which means fun2() is a function nested inside fun1() and is valid in Python.

It looks like for such cases, Emacs picks the 1-level of forward indentation as the first in the cycle of possible indents which can be changed by tab key. What is correct cannot be picked by Emacs or by yasnippet code, it has to be specified by the snippet file in case of yasnippet and in other cases by the developer.

Sorry for the long comment explaining what is in my mind. Hope it is relevant to solving this issue.

@lgp171188
Copy link
Author

This could be an issue with all the snippets with more than one level of indentation.

@joaotavora
Copy link
Owner

@lgp171188, indent-according-to-mode doesn't do the same thing as tab, i.e. it doesn't cycle, so I believe yes, it is the right operation. It would be good to get the behaviour of other versions of emacs in this respect. Otherwise there is the workaround to set the snippet's indendation to fixed.

@lgp171188
Copy link
Author

@capitaomorte Yes, indent-according-to-mode doesn't do the samething as tab i.e. it doesn't cycle. But I think it is picking the first of the possible indentation levels and sticking with it, which may or may not be wrong depending the context of the code being written. There is no automatic way to figure out the logic of the program and indent appropriately. Only the developer of that piece of code will know the correct level of indentation. So I am wondering if using indent-according-to-mode was correct in the first place.

@lgp171188 lgp171188 changed the title python-mode snippets are expanded with wrong indentation python-mode snippets are expanded with wrong indentation if there is more than one level of indentation in the snippet May 31, 2014
@npostavs
Copy link
Collaborator

I don't think the behaviour of indent-according-to-mode and tab is so different. The non-cycling vs cycling is a property of python-indent-line, not Emacs' general indentation machinery:

indent-according-to-mode is an interactive compiled Lisp function in `indent.el'.

(indent-according-to-mode)

Indent line in proper way for current major mode.
Normally, this is done by calling the function specified by the
variable `indent-line-function'. ...
indent-for-tab-command is an interactive compiled Lisp function in `indent.el'.

(indent-for-tab-command &optional ARG)

Indent the current line or region, or insert a tab, as appropriate.
This function either inserts a tab, or indents the current line,
or performs symbol completion, depending on `tab-always-indent'.
The function called to actually indent the line or insert a tab
is given by the variable `indent-line-function'.
...
python-indent-line is a compiled Lisp function in `python.el'.

(python-indent-line &optional FORCE-TOGGLE)

Internal implementation of `python-indent-line-function'. ...

When the variable `last-command' is equal to one of the symbols
inside `python-indent-trigger-commands' or FORCE-TOGGLE is
non-nil it cycles levels ...

Otherwise there is the workaround to set the snippet's indendation to fixed.

I think yas-indent-line should be set to fixed in python-mode; adding indentation to python code is like adding braces to C code.

@joaotavora
Copy link
Owner

@npostavs perhaps you're right, and maybe just

(add-hook 'python-mode-hook
   '(lambda () (set (make-local-variable 'yas-indent-line) 'fixed)))

is sufficient. @lgp171188 can you try this?

But I still think there is a difference between indent-according-to-mode and tab. In these examples, I see no reason for the former to indent some lines deeper when called the first time, and indent-region does not do that, for example. The idea is that auto is roughly the same as indent-region applied to the recently inserted snippet.

Admittedly I haven't written any python in some years, or followed python-mode :-) , but I find it suspicious that this problem arises only now, when python snippets have been a part of yasnippet since almost its beginning in 2008.

@npostavs
Copy link
Collaborator

npostavs commented Jun 1, 2014

but I find it suspicious that this problem arises only now,

I'm finding the same behaviour in 23.4.2 (which is the oldest emacs I have handy).

@joaotavora
Copy link
Owner

@lgp171188, I'm well aware of python indentation semantics. But it's the heuristic that emacs uses that is bringing trouble in this snippet, i.e. it always assume you want to continue statements in the deepest possible indent level. So it's actually the missing return in the bodies of fset and fdel that confuse Emacs, and then yasnippet.

So, I'm not sure I agree with you @npostavs that yasnippet should force or recommend yas-indent-line to be fixed in python-mode, since a value of auto appears to be perfectly useful in other snippets which don't have this ambiguity.

I do realise that omitting return in these cases is probably idiomatic and more correct stylistically.
So I would say that this and similar snippets must have # expand-env: ((yas-indent-line 'fixed)) added to them.

@lgp171188 perhaps you can test this change and then tell the maintainer of these snippets, which live at https://github.com/AndreaCrotti/yasnippet-snippets, to add it to his repo.

@npostavs
Copy link
Collaborator

npostavs commented Jun 1, 2014

So it's actually the missing return in the bodies of fset and fdel that confuse Emacs, and then yasnippet.

Note: it's only in 24.4 that Emacs became smart enough to look for a return statement.

@lgp171188
Copy link
Author

@capitaomorte Thanks for suggesting a solution. I added the # expand-env: ((yas-indent-line 'fixed)) line to the snippet and voila the indentation was correct after that. I will raise an issue on the snippets repository referring to this issue to get this fixed. Thanks for your help :-)

@shaleh
Copy link

shaleh commented Aug 18, 2014

@capitaomorte, as I demonstrated in my ticket #506 the return is not always syntactically correct. yasnippets fails to do the right thing with a block of if/else constructs.

I also have this snippet:

try:
    $1
except $2 as e:
    $3

The except is lined up under the expansion of $1. I need to set this expand-env in all of my Python snippets to get the correct behaviour.

@joaotavora
Copy link
Owner

@shaleh, OK, but lets continue this in #506. It's also important to test if Emacs itself does incorrect/unexpected things when asked, line by line, to indent the region by itself. Because this might signal that Emacs itself needs to be fixed.

@shaleh
Copy link

shaleh commented Aug 18, 2014

Shall do.

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

No branches or pull requests

4 participants