Tryton Language Server

Feature Ideas
1

I spent a few days during the past year playing around with Pygls, and used some free time during holiday season to make a Language Server for Tryton

You can find it here.

It requires python 3.11 and a little patch on trytond to be able to get a useable Pool without a database, but it works and provides some features that I think are already useful:

  • depends / extras_depend -aware analysis
  • Detection of unknown attributes in python / view files when guessing the model is possible
  • Completion of fields / methods

Let me know what you think about this :slight_smile:

3 Likes
2

It looks great to me!

Although I use neovim, configuration of lspconfig did not work in my laptop (it seems I should upgrade nodejs…).

But we’ll try to take a look on how to configure it with VS Code which is what most of my collegues use.

3

This sounds interesting but I’m a bit afraid of the quantity of code needed.
I think some part of the analysis could be done by Python type checking like mypy if we extend it with plugins.
For example get_dynamic_class_hook could be used to provide the proper type to pool.get and get_customize_class_mro_hook to return the “inherited” classes to the partial class of a module. I think it will prevent all the complexity of analyzing the AST.

Now I guess the language server may be helpful for “non-code” analysis like the depends or missing required attributes. But this part I guess could be simpler.

For the patched trytond, I have the feeling that it should be possible to construct the result class without the need to call Pool.init but just by looping on the registered class and construct the requested one only for a specific sets of modules.

4

I agree, that’s a lot of code :frowning: . Unfortunately with this approach I do not see how it could be significantly reduced.

I did not spend a lot of time investigating this approach. It seemed to me that doing so would also need a lot of work to :

  • Reimplement the dependency resolution
  • Work-around the __getattr__ overrides
  • All the custom things of Tryton (like you said, depends, but also xml, completion, etc…).

We would still have to call the __setup__ / __post_setup__ (which may call Pool()) methods to have the final version of the models, so reusing init seemed simpler in the end.

5

You can use the trytond.modules tools, no need to re-implement everything.

I do not think it is needed because fields are accessors so indeed Model.__getattr__ could be replaced by something like Model.load. Anyway the type checker should not care as long as it can guess the type of each field.

I personally do not care much about these parts because we (should) have tests for that.

I do not think we have such calls in standard modules. But I think it may be good to find a way to forbid it because there is no way to guarantee that an model will be setup before another.

6

I started an experimental mypy plugin at Draft: Add plugin for type checking with mypy (!2352) · Merge requests · Tryton / Tryton · GitLab
For now it inject the “parent” classes for classes with metaclass as PoolMeta (for now only for the hard coded party.party in party_siret module). But it works as it can detect “undefined in superclass” and “Missing positional argument” for inherited method.

The next points are:

  • find classes to inject for a tryton name and module
  • find a fix for the Model.__getattr__ (probably remove it from the definition)
  • get a way to type using model that are not in the module
  • hook Pool.get to return the proper type
7

The difficulty with this is that we can not really use Tryton code to compute this for us because the Tryton code may not be importable (when editing the syntax may not be valid).
So for me we need to move outside the Python code the definition of the classes loaded to the Pool.
So I propose to replace the register method by extending the tryton.cfg syntax. Such that trytond will use the tryton.cfg instead of registerif the file contains the proper keyword (this will ensure a backward compatible transition to this new syntax). Here is the proposed format (example forparty_siret` module):

[tryton]
version=7.5.0
depends:
    ir
    party
extras_depend:
    company
xml:
    party.xml
    address.xml
[register]
model:
    party.Party
    address.Address
[register company]
model:
     company.Company

So the idea it to provide the qualname relative to the module of the classes to register. And the register keyword can be following by space separated list of depends modules.
Another advantage of this change is that trytond will no more need to import all the modules to register its pool. It can just store the string of the qualname and resolve it only if it is used by a database.

Also for tryton_mypy.py the get_additional_deps will need to include the path of the sub modules used in register sections.

8

Not having to import all modules looks like a great advantage to me. Even if the other changes do not make it necessary.

Is it really necessary to read all those files and load all those strings? Maybe it can be lazy loaded?

9

We need to read anyway all the tryton.cfg to build the dependency graph.

10

I think this should be solved like this:

from typing import cast

class Party(metaclass=PoolMeta):
    __name__ = 'party.party'

PartyType = type[Party]


class Custom(Model):
    ...

    def func(self):
        pool = Pool()
        Party = cast(PartyType, pool.get('party.party'))
        ...

with Pool.get typed like:

class Pool:
     ....
     def get(self, name: str, type:str = 'model') -> type[Any]:
         ...
11

For this, I think we could define the class just for typing like:

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    class Product(metaclass=PoolMeta):
        __name__ = 'product.product'

    ProductType = type[Product]

class MyModule(Model):
    ...
    def get(self, product: Product) :
        ...

and in tryton.cfg:

[tryton]
...
[register]
model:
    mymodule.MyModule
[register_type_checking]
model:
    mymodule.Product

Like that the mypy plugin can include the dummy types when checking module that depends on this one.

12

For typing the relational fields, the problem is that the type must be of the type of the Model built with the dependencies of the module.
I think we could solve it by redefining the needed classes with specific typed fake field.
This would look like:

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    class PaymentTerm(metaclass=PoolMeta):
        __name__ = 'account.invoice.payment_term'


    class Party(metaclass=PoolMeta):
        __name__ = 'party.party'
        customer_payment_term = fields.typing.Many2One[PaymentTerm]()


class MyModule(Model):
    ...
    def get(self, party: Party):
        self.party.customer_payment_term

with in trytond.model.fields.typing:

from typing import TypeVar, Generic

T = TypeVar('T')

class Many2One(fields.Many2One, Generic[T]):
     def __init__(self):
         ...
     def __get__(self, inst, owner) -> T:
         ...

Here the goal is to convince the type checker that the field accessor return a specific type of instance.

13

First of all, thanks for working on this!

Your latest message suggests that we may have a lot of things to write in the TYPE_CHECKING block. If I understand properly:

  • All models that are used in the file
  • The relation fields of all those models
  • With possibly some duplicates to account for extras_depend

It feels like all of this this could be automatically generated in a separate file (per module) by a dedicated command?

14

Indeed it would only be needed if the module is using fields that are not defined in the first declaration of the model.

Well I think the linter will help on this case because it will detect the problem.
So I see that a little bit like a missing import.

15

That would probably still mean a lot of fields / models.

On the other hand, generating it would mean we could be certain that the field / model indeed exists in the module’s context.

The current proposal means that one may add a field to the type-checking part of the file to “fix” an error without realizing that there is a dependency issue somewhere.

(Just brainstorming, I am still not sure about what would be the best solution)

16

I like this idea btw!

17

I think we should strive to make module development in Tryton easier, not harder. It is already complex enough for newcomers.

If having this kind of introspection requires writing more code when creating a module, I’d rather not have it, to be honest.

2 Likes
18

Adding type will always be more work than not adding typing.
But the advantage of mypy is that it is optional checker.

For standard Tryton module, I think it will become mandatory if we succeed to have a decent working implementation.

19

It would require writing more code when creating / adding new features to a module, true, but (I think) it would also increase the readability / resiliency of those modules.

My initial experiment lead to a few squashed lurking bugs in our codebase, just by identifying typos in field names or modularity issues in untested code. Yes, better tests would have detected those, and they definitely were rare edge cases, but it was still “free” bug fixes.

20

Maybe it is possible to use get_dynamic_class_hook to return the proper class for Pool.get calls.