astor icon indicating copy to clipboard operation
astor copied to clipboard

Validate input AST for user facing functions

Open leonardt opened this issue 7 years ago • 4 comments

I can take a stab at this when I get some time, but posting this now in case someone else is interested.

Sometimes I pass in an incorrect AST to an astor method, e.g. astor.to_source(ast) (e.g. ast contains a raw int instead of wrapping it an ast.Num). This leads to tricky errors internally in astor that can be hard to understand for the user (this particular error resulted in a failed assertion in precedence_setter.set_precedence).

I propose that astor validates the input AST for any user facing API functions. This would allow any internal logic to assume that the AST is valid and for users to be immediately notified that the input AST is wrong, rather than having to parse errors or exceptions from the internal logic.

leonardt avatar May 24 '17 19:05 leonardt

Thanks for the report. I think that would be a good feature, but the tricky part is that we need to keep in mind different Python versions when doing validation. For example, wrapping int with ast.Num might be the correct solution for Python 3.x and 3.x+1. However, Python 3.x+2 might need ast.Number or ast.Guido in the future.

berkerpeksag avatar May 24 '17 19:05 berkerpeksag

In general, I'm not a big fan of slowing down the general case for exceptions.

I would support your proposal turned inside out; e.g. we wrap the top functions with try/except Exception , and if an exception occurs, we transfer control to a function that tries to do a better job of explaining to the caller what went wrong (e.g. re-raise a different exception).

pmaupin avatar May 24 '17 19:05 pmaupin

@pmaupin Interesting point of view! Makes sense to me and does seem like a better approach.

leonardt avatar May 24 '17 21:05 leonardt

Also, I don't have any time right now, but if someone is looking at this, we should probably merge my current PRs (or not) first...

pmaupin avatar May 24 '17 21:05 pmaupin