[Patch] Python syntax: avoid highlighting attributes as builtins

[Carlos Pita]

Hi all,

the syntax rules for python incorrectly consider, say, both `filter` and `obj.filter` as instances of the builtin `filter`. I've added a rule to explicitly set the group of attributes (defined as an identifier following a dot) to none. Maybe it's better to create a new group for attributes, I'm not sure. I will update the patch with any improving suggestion.

Cheers
--
Carlos

[Zvezdan Petkovic]

> On Jul 31, 2015, at 9:43 AM, Carlos Pita <carlosj...@gmail.com> wrote:
>
> Hi all,
>
> the syntax rules for python incorrectly consider, say, both `filter` and `obj.filter` as instances of the builtin `filter`.

Yes, I’m aware of this. I don’t think it’s incorrect though.

> I've added a rule to explicitly set the group of attributes (defined as an identifier following a dot) to none. Maybe it's better to create a new group for attributes, I'm not sure. I will update the patch with any improving suggestion.

Maybe, this should be a warning sign that you are using the name that’s also a builtin which is at least a bad practice? :-)

For example, take this program:

class A:
filter = None

def bogus_filter():
pass

filter = bogus_filter
# lots of code in between
filter(...) # oops, just called bogus_filter instead of the real one

a = A()
a.filter = 2

- your change will strip the highlighting only from the last line in “a.filter”.
- your change will not strip highlighting from the line in class A: “filter = None”

Questions for you:

- Is there any value in making an exception for only a.filter line?
- How about filter = None line?
- Do you consider useful a highlighting hint that you are using a builtin name as the name of the binding that’s assigned bogus_filter? Wouldn’t this prevent a mistake later?

I’m not convinced that such change adds enough value for a small number of cases, while it does take away the value of a warning that one is making a potential mistake. Isn’t that the main reason we use syntax highlighting?

What does the rest of Vim community think?

Regards,

Zvezdan

[Carlos Pita]

> - Is there any value in making an exception for only a.filter line?

Yes, you can say for (99%) sure that a.filter is not a builtin, at least no more than a.x. Instead it's dubious practice to name a global or local variable as a builtin, that's looking for trouble. But attributes and variables are different beasts, differently scoped.

There's little to say against value.set(3) or point.set(x=3, y=2) but a lot against def set(obj, attr, val): .... or set = [1, 2, 3].

Cheers
--
Carlos

[Zvezdan Petkovic]

Hi Carlos,

OK. But there still remains the question of discrepancy between:

class A:

    filter = None

and:

a = A()

a.filter = 2

On one line it would still be highlighted, while on another it wouldn’t, for the same attribute.

That’s inconsistent.

Zvezdan

[Carlos Pita]

> OK. But there still remains the question of discrepancy between:
>
> class A:
>     filter = None

Here filter is a variable locally scoped to the block defined by A, in that sense this is not different from:

def a():
    filter = None

Both clashes with the "auto-global" builtin.

Fortunately the most common case will be:

class A:

  def filter(...): ...

where filter would be highlighted because of the def anyway.

> and:
>
> a = A()
> a.filter = 2

Here a (and even A) is a potential offender but not filter, which is resolved inside a.

The rule is simple: an unqualified variable named like a builtin is highlighted to signal potential mistakes / bad smell.

There already is a variable to control highlighting of builtins. Currently it's a toggle but it could have 3 levels instead:

0: never.
1: only unqualified.
2: always.

Heuristically I think 1 does a better job, but you can set the default to 2 if you prefer.

Cheers
--
Carlos

[Carlos Pita]

> >
> > class A:
> >     filter = None
>

You could also avoid highlighting left hand sides as they're easy to match. A variable that is always assigned to would be weird but never confused with a builtin, as it's clear it's being bound at that point. Now, any potential *use* of a builtin (i.e. not a lhs) will be highlighted as usual.

I don't care a lot about this case though.

Cheers
--
Carlos

[Zvezdan Petkovic]

Hi Carlos,

On Aug 1, 2015, at 3:41 AM, Carlos Pita <carlosj...@gmail.com> wrote:

> OK. But there still remains the question of discrepancy between:
>
> class A:
>     filter = None

Here filter is a variable locally scoped to the block defined by A, in that sense this is not different from:

No, it’s not a locally scoped variable (binding in Python).

It’s a class attribute.

def a():
    filter = None

This is a locally scoped binding.

Both clashes with the "auto-global" builtin.

Fortunately the most common case will be:

class A:

  def filter(...): ...

where filter would be highlighted because of the def anyway.

I would still prefer to see this named member_filter, or any reasonable qualifier beside member, if I reviewed that code.

> and:
>
> a = A()
> a.filter = 2

Here a (and even A) is a potential offender but not filter, which is resolved inside a.

This is the same class attribute from above being assigned a value here.

Why would you have it highlighted above and not here?

The rule is simple: an unqualified variable named like a builtin is highlighted to signal potential mistakes / bad smell.

There already is a variable to control highlighting of builtins. Currently it's a toggle but it could have 3 levels instead:

0: never.
1: only unqualified.
2: always.

Heuristically I think 1 does a better job, but you can set the default to 2 if you prefer.

I understand fully what you ask for. It’s still inconsistent for the case of class attributes.

I just wonder if there is really a value to complicate the highlighting code for a small use case like this compared to the value this “warning” gives. You do realize that this highlighting always existed and never bothered anybody until now. What if this gets changed and then users start complaining about the fact it’s not highlighted any more.

Let’s have others chime in and take a look at this next week.

I’m taking a break this weekend.

Zvezdan

[Carlos Pita]

> Here filter is a variable locally scoped to the block defined by A, in that
> sense this is not different from:
>
> No, it’s not a locally scoped variable (binding in Python).
> It’s a class attribute.

How can it be an attribute of a class that doesn't exist yet? You must
differentiate a class being created from a class already created:

class A:

x = 1 # x is a local, A doesn't even exist at this point.
print(locals())

print(getattr(A, 'x')) # x is an attribute.

==>

{'__module__': '__main__', '__qualname__': 'A', 'x': 1}

1

Despite semantic subtleties, the point is that unqualified names are susceptible
of name clashes with builtins, while qualified named aren't.

> I understand fully what you ask for. It’s still inconsistent for the case of
> class attributes.

Again: differentiate between the class local scope at the point of
definition and proper class attributes. Then the rule is consistent.

> I just wonder if there is really a value to complicate the highlighting code

Well, it's a oneliner, but then I wouldn't mind putting it inside my
.vimrc, laissez-faire, laissez-passer ;).

> for a small use case like this compared to the value this “warning” gives.

Warnings are good when you can selectively disable false positives.
Otherwise they become distractions and you tend to overlook the real
ones. What I'm suggesting disables just a subset of false positives
(another subset is left hand sides). I don't see how it could hurt in
this sense.

> You do realize that this highlighting always existed and never bothered
> anybody until now. What if this gets changed and then users start
> complaining about the fact it’s not highlighted any more.

I suggested to keep the current behavior as default and adding one or
two lines of code to offer a new mode, without additional config
variables and retaining backward compatibility (just set
0=never,1=always and 2=non-qualified). Again, I wouldn't mind putting
that in my personal configuration. I just posted a patch that may be
useful for someone else, feel free to include it in the distributed
module (I can update it to add the 2=non-qualified mode, though).

> I’m taking a break this weekend.

Enjoy it.

Cheers
--
Carlos

[Marius Gedminas]

On Sat, Aug 01, 2015 at 07:39:47AM -0700, Zvezdan Petkovic wrote:
> Let’s have others chime in and take a look at this next week.

I've been mildly annoyed by 'filter' getting mistakenly highlighted as a
builtin in expressions like

foo.filter(...)

There are packages out there (e.g. SQLAlchemy) that have this kind of
API and I cannot change that.


I don't really care about

class Foo:
filter = ...

one way or another.

Marius Gedminas
--
Get a life? Well, once I nearly found one, but the link was broken.

[Carlos Pita]

> I've been mildly annoyed by 'filter' getting mistakenly highlighted as a
> builtin in expressions like
>
> foo.filter(...)

For those doing multiprocessing and/or multithreading there is
Event.set also, in the "standard library".

And, in general, it's pretty common having some_entity.id.

Cheers
--
Carlos

[Zvezdan Petkovic]

Hi Marius and Carlos,

I just sent the pull request on GitHub that should fix this issue without affecting other syntax highlighting.
https://github.com/vim/vim/pull/651

Please try it and see how it works for you.

Thanks!

Zvezdan

> --
> --
> You received this message from the "vim_dev" maillist.
> Do not top-post! Type your reply below the text you are replying to.
> For more information, visit http://www.vim.org/maillist.php
>
> ---
> You received this message because you are subscribed to the Google Groups "vim_dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to vim_dev+u...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.