Fix api.usages so it finds cross-module usages

[andrew-lee-work]

Jedi could not find certain cross-module usages. The problem was in how it compared contexts in api.usages.

I've included a test that fails without my included fix.

[davidhalter]

No. This is not how we work here. Did you maybe just randomly check this one in?

[andrew-lee-work]

Yeah, I didn't mean to add that. I'll remove it.

BTW, I added that part a long time ago because the usages function was running into errors and then giving up. Catching the exception and continuing the loop allowed jedi to find other usages that didn't raise errors. I logged the error because I didn't want the errors to go unnoticed. What is an alternative way you would accept?

[andrew-lee-work]

Reposting some parts because I don't know where github put my previous comment (and yours)

I've removed the try/except part. Sorry, I didn't mean to add that to the PR. I've also made a minor change to the column indices in the test code. It doesn't affect the functioning of the test but it seems more correct.

BTW the try/except was because goto was raising errors and stoppingusages from returning any other items it successfully found. I added logging so that the errors would not be hidden. What alternative would you suggest to solve this?

Note: The old stuff is at fix-api.usages-bad.

[davidhalter]

What alternative would you suggest to solve this?

Fixing the actual bug. Catching NotImplementedError is like the worst error that I've ever heard.

[andrew-lee-work]

I see. I (mistakenly) thought you just hadn't finished this part of Jedi, and I didn't want the unfinished part to block the user from seeing all the other usages that Jedi was able to find.

By your comment, I take it that Jedi should not be raising NotImplementedError, and when it does it means an abstractmethod was called by accident. In that case, I wonder why you are not using ABCMeta, which would catch this error earlier? Is it for compatibility reasons?

Also, would you be OK with catching RecursionError?

[davidhalter]

I (mistakenly) thought you just hadn't finished this part of Jedi

Maybe it even is, but that doesn't matter. I think open source libraries should strive for perfection and not work some of the time. If something is wrong we should fix it.

I wonder why you are not using ABCMeta, which would catch this error earlier? Is it for compatibility reasons?

I'm not really using ABCMeta, because it would make some things slower. Using NotImplementedError can also be very useful in case you have never really finished something - like an else statement.

Also, would you be OK with catching RecursionError?

No. I'm not ok with catching any exceptions if it's nested. These are actual bugs that you are hiding by removing RecursionError. It's not like this should ever happen. And if it does and it's fine, we should either increase the stack or change certain limits so that it does not happen again.

I get a lot of pull requests here that just "fix" an error by catching e.g. AttributeError. Now this makes debugging harder by a factor of 10. I think most Python people don't really understand that having the power of catching pretty much all exceptions does not mean that you should do it. In complex programs this always leads to extremely bad code. Abstractions are there for a reason. Catching an error that was thrown two levels deeper just doesn't make any sense 99% of the time.

[davidhalter]

Ok. This looks good. I think it's a good solution for now. I still think that usages is completely and utterly broken and we have to rewrite it, but it's a good fix for now.

Would it be possible to refactor the tests a bit into a form of jedi/test/completion/usages.py? That would be awesome, because until now all the usages tests are there.

[andrew-lee-work]

In order to refactor it into the form of jedi/test/completion/usages.py, I think I would need to change jedi.Script.usages to include the code from usages_with_additional_modules from my test code. Otherwise the integration test would ignore the explicitly included module. I'm not sure you want that though.

[davidhalter]

Ok. I see!