Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rik de Graaff Added broader support for context managers in with statements which swallow exceptions. #2448

Merged
merged 3 commits into from
Oct 18, 2021

Conversation

rik-degraaff
Copy link
Contributor

No description provided.

@ghost
Copy link

ghost commented Oct 18, 2021

CLA assistant check
All CLA requirements met.

@erictraut
Copy link
Collaborator

This code won't work for call expressions in general. It doesn't handle overloads, functions with generics, functions with inferred return types, etc. As such, I'm conflicted about adding it. While it may address your specific needs, it's going to be a maintenance headache because it will inevitably result in bug reports from other users who expect it to work in these other cases with call expressions. We need to draw the line somewhere about what is supported and what's not, and this makes that line blurrier.

Let me think a bit more about this, and let me know if you have any thoughts about the above points.

@rik-degraaff
Copy link
Contributor Author

I think not supporting functions with inferred return types is probably acceptable, since that's seems line with how a context manager which swallows exceptions is detected. The declared return type has to be bool. Overloads and generics should probably be handled if they can, but I'm honestly not familiar enough with the project to do that in reasonable time. If you can think of how it would be done and can point me in the right direction, I could give it a go if that results in less work for you.

If they can't be handled with this approach, I agree this might not be worth the headache.

This is pure speculation, but I wonder if it would be possible to resolve this by doing two passes. One to get a first approximation of the flow graph in the function, where you assume the context manager swallows exceptions, then infer types based on that and then another pass where you use the type information from the first pass to determine if the context manager actually swallows exceptions. Though it might be that this would actually require fixed point iteration and I imagine even doing two passes for certain functions is not something that fits with the goals of pyright.

@erictraut
Copy link
Collaborator

Overloads and generics aren't feasible to support in this context. They require the full machinery of the type evaluator and code flow engine.

Supporting this in general would require a multi-pass evaluator. The original version of pyright used this architecture, but we abandoned it a couple of years ago because it was too slow and didn't meet other requirements for a language server. So we need to live with the current limitations in these edge cases.

You mentioned that your use case requires the use of a function call. Can you explain the thinking behind that decision? Is the context manager instance dynamically allocated, for example? Could you modify your code to use the more common forms that are already supported?

@erictraut
Copy link
Collaborator

Thinking about this a bit more... The limitations in your function call handling is consistent with the limitations in the existing supported cases (constructor calls and simple names), so I think that's defensible if someone complains about not handling more general cases.

I'm going to merge the PR. Thanks for the contribution.

@erictraut
Copy link
Collaborator

I'll fix the style issues after I merge.

@erictraut erictraut merged commit c074082 into microsoft:main Oct 18, 2021
@rik-degraaff
Copy link
Contributor Author

Thanks for the feedback and I'm very glad to see this get merged. I hope I didn't cause you too much work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants