bug#67061: [PATCH] Improve syntax highlighting for python-ts-mode

[Denis Zubarev]

> >  > One of the complaints is that "User" is not highlighted as a type when

> >  > used in other, non-built-in methods, which like a reasonable question to

> >  > me. Yes, Python is dynamic, but using CamelCase for types is a fairly

> >  > regular convention, so highlighting such identifiers as types can work.

> > It is good idea, to highlight some variables as types. But I think it

> > should be done on the 4th level. One could split the variable feature

> > into multiple features: variable-type, variable-argument, variable-use,

> > etc. So for variable-type feature we can use python--treesit-type-regex

> > and highlight matched identifiers with type face. For now I wanted to

> > properly highlight types in places where they expected to be.

 

> I wouldn't mind the level 4 (after all, python-mode is also conservative

>  here and doesn't add such highlighting), but I'd rather not add the

>  special handling for isinstance/issubclass thing for the reasons

>  previously outlined.

 

Just adding a rule for highlighting CamelCase identifiers as types would

lead to many false positives. For example, global variables or an object

instantiation. We could add its own features for these cases, so the type

feature couldn't override them, but it requires extra work and the patch

is already quite large. So I'd like to highlight identifiers as types

only in the context, where types are expected, for now. We could reconsider

this after dealing with the most common false positives.

 

 

 

 

 

21.12.2023, 02:34, "Dmitry Gutov" <dmitry@gutov.dev>:

On 19/12/2023 02:14, Denis Zubarev wrote:

  > If you recall my earlier complaint that these highlightings didn't work
  > (and the tests didn't pass), this happened due to an older Python
 grammar.
 Thank you for investigating this. It seems this commit introduced
 changes to type nodes hierarchy
 (https://github.com/tree-sitter/tree-sitter-python/commit/bcbf41589f4dc38a98bda4ca4c924eb5cae26f7b).

Could be this one, yes.
 

  > The queries didn't lead to errors either (that's a good thing), but maybe
  > we'll want to revisit these highlights later to add support for the
  > older grammar as well.
 It may lead to unnecessarily complex rules. I don't
 know is it worth it, since users can easily update grammars.

No problem.
 

  > I'm not sure highlighting types based on the caller method and position
  > is a good idea. I think that's backward, logically. If one puts a
  > non-type value in such argument, and we would highlight it as a type --
  > that seems like the wrong message.
 These two functions expect a type (or tuple of types) as the second
 argument. To address your concerns about highlighting as a type a
 non-type variable, I added regexp python--treesit-type-regex. This regex
 matches if text is either built-in type or text starts with capital
 letter. I extracted built-in types from the python--treesit-builtins
 into its own variable python--treesit-builtin-types.
 python--treesit-builtins is now constructing by appending
 python--treesit-builtin-types and other built-ins. I hope it is ok.

Thank you. I'm actually not sure if we _have to_ check the identifier
names in this context (any chance to have a false negative, miss some
valid types?), but it probably doesn't hurt either.
 

  > One of the complaints is that "User" is not highlighted as a type when
  > used in other, non-built-in methods, which like a reasonable question to
  > me. Yes, Python is dynamic, but using CamelCase for types is a fairly
  > regular convention, so highlighting such identifiers as types can work.
 It is good idea, to highlight some variables as types. But I think it
 should be done on the 4th level. One could split the variable feature
 into multiple features: variable-type, variable-argument, variable-use,
 etc. So for variable-type feature we can use python--treesit-type-regex
 and highlight matched identifiers with type face. For now I wanted to
 properly highlight types in places where they expected to be.

I wouldn't mind the level 4 (after all, python-mode is also conservative
here and doesn't add such highlighting), but I'd rather not add the
special handling for isinstance/issubclass thing for the reasons
previously outlined.

Perhaps Yuan will disagree. I'm just here to say that the rest of the
patch LGTM.