Should astroid adhere to ast's module API ?

[Pierre-Sassoulas]

Context: @tristanlatr opened issues lately after trying to replace ast by astroid in their project. And we're not experienced enough with astroid to decide if we should add astroid's version of ast's api function

We decided recentely that we were an "interface above ast" and rejected a merge request fixing consistency lately. I don't have strong opinion about past decisions, there's probably arguments for both side of the issue.

Pro:

• Clearly being able to switch from ast to astroid seamlessly would be really useful for astroid adoption and ease of use in general.

Con:

• But it would also be a lot of breaking changes and upgrade to do. In fact every-time there is a change in ast, we must check astroid's consistency, which seem like a lot of work.

@hippo91 @shlomme @brycepg @AWhetter what's your opinion on the matter ? Should astroid implement ast's API so we can switch astroid's objects with ast's objects and so astroid has ast's adapted functions too ? I suppose @thejcannon and @cdce8p would be interested in the discussion too :)

Also @tristanlatr as you see there's positions open for astroid maintenance. From the issues you opened you seem to know what you're talking about :)

[cdce8p]

I was the one who argued against merging #1218. Although we should try to keep roughly the same structure as the AST, astroid in its current form isn't and more importantly doesn't want to be a 1-on-1 replacement for the ast module. E.g. we may choose to insert additional nodes into the AST if they help with inference. That's still the goal after all.

I would be interested to know why you wanted to replace ast with astroid. What do you hope to accomplish with the transition? Since both focus on different aspects, a change what probably be an expensive rewrite of (possibly) working code.

As it stands now, I would suggest against it.

[tristanlatr]

Hello,

[astroid] doesn't want to be a 1-on-1 replacement for the ast module

I understand that. Some nodes are not present in ast module, and that's OK. I just want to help out and provide a more ast-like inferface and helper functions. Just so the move ast -> astroid cost a little bit less of effort.

I would be interested to know why you wanted to replace ast with astroid. What do you hope to accomplish with the transition?

Thanks for your intersest, happy to provide additionnal informations. I'm building a parser to extract complete project structure in order to generate API documentation or other things. You can check the repository here: https://github.com/tristanlatr/pydocspec . With this project, I aim to replace pydoctor's AST builder to fix some issues and provide additionnal information thanks to inference. I hope astroid will be help to solve the following issues: twisted/pydoctor#10 (MRO issue), twisted/pydoctor#208 (introspection issues, building AST from live module is a great feature), twisted/pydoctor#364 and twisted/pydoctor#348 semantic analysis).

To be more precise, lteral_eval (proposed in #1347 (review)) utility function is used to infer simple standard types annotations, and also at other places. As you can see, I have also the need for the fix_missing_locations() and copy_location() since I programatically create some nodes.

Overall these utility function are very useful and I think astroid should offer the alternatives for functions that are present in the standard library. Converting code from ast to astoid should not require users to adapt the standard library function for their workflow (not talking about a seamless transition here, but beeing able to keep the overhaul code algorithm) that worked with ast module to continue to work with astroid.

Two weeks ago, the documentation said that the astroid nodes extends standard library nodes, so I had this idea of compatibility? ^^

If the features are well tested and not very complex, I don't see why not accepting such improvments.

Talk to you later,

[cdce8p]

Thanks for taking the time to explain it!

Overall these utility function are very useful and I think astroid should offer the alternatives for functions that are present in the standard library. Converting code from ast to astoid should not require users to adapt the standard library function for their workflow (not talking about a seamless transition here, but beeing able to keep the overhaul code algorithm) that worked with ast module to continue to work with astroid.
[...]
If the features are well tested and not very complex, I don't see why not accepting such improvments.

I would to disagree. In contrast to ast, astroid isn't designed for round-tripping. I.e. converting astroid nodes back to fully executable code. The most we do is as_string which tries to provide a valid string representation of the node. Although keep in mind that it doesn't work in every case.

So why not add it regardless? Someone still needs to maintain it which does increase the overall workload. Even if it's tested who is to say that sometime in the future a Python change won't break it or worse, in the case of literal_eval, introduce a security vulnerability. Furthermore, new features aren't always used the way they are designed to be used. In the case of copy_location it's easy enough to imaging that someone misuses it. If custom nodes are added by plugins for the purposes of interference / use in pylint, the plugin authors should try to set a reasonable location or none at all, not copy it from another node.

In summary, I do think that your use case is interesting. However, since it's not entirely what astroid is designed for, I don't think we should add these helpers for the time being.

[tristanlatr]

I don’t see why you say that I’m using astroid for a purpose that it wasn’t designed for.

All I’m doing to switch from ast to astroid. Adapting my code. Use infer() here and there to get better results. Have you looked at the source code of the function infer_type_annotation ?

Can you elaborate on why the copy location usage seems wrong to you (the lineno information is going to be correct) ?

All I’m saying is that it’s much more difficult that I thought to switch to astroid from ast and I would like to help out make this easier. And I don’t see any reason not to do such moves if it’s well tested. I mean, litera_eval… no way this function is going to have some major changes in the standard library in the future, it’s too fundamental.

So I’m a bit disappointed here, I realize that I’ll have to keep all my code that replicate de standard library, but with NodeNG instances… probably other people already did that, or did not because the cost of doing this is too high.

talk to you later,

[cdce8p]

I don’t see why you say that I’m using astroid for a purpose that it wasn’t designed for.

The way I see it, astroid is there to provide ast helper with inference for pylint. It may be used differently, but in that case you would be on your own. I.e. no guarantees that astroid will officially support it.

Can you elaborate on why the copy location usage seems wrong to you (the lineno information is going to be correct) ?

In your cases it isn't. That doesn't mean though that plugins should use it.

All I’m saying is that it’s much more difficult that I thought to switch to astroid from ast and I would like to help out make this easier. And I don’t see any reason not to do such moves if it’s well tested. I mean, litera_eval… no way this function is going to have some major changes in the standard library in the future, it’s too fundamental.

Again, astroid wasn't designed for this. That's why it might be difficult. All I'm saying here is that I think we shouldn't included these helpers in the main codebase. You're of course free to continue using them. If you think that other projects might find them useful, you could also create a new pypi package to distribute them independently
IMO your use case just isn't how most people use astroid. Therefore I don't think these helpers should be included.