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

Django. Several DoesNotExist exceptions not reached #3918

Closed
FMZPNP opened this issue Feb 7, 2023 · 8 comments
Closed

Django. Several DoesNotExist exceptions not reached #3918

FMZPNP opened this issue Feb 7, 2023 · 8 comments
Assignees
Labels
fixed in next version (main) A fix has been implemented and will appear in an upcoming version typestub Issue relating to our bundled type stubs

Comments

@FMZPNP
Copy link

FMZPNP commented Feb 7, 2023

When we try to catch mutiple DoesNotExist exceptions in different code blocks pylance says that those which are not the first will not be reached. But in fact, they will be reached and raise the corresponding exception.

Environment data

  • Language Server version: v2023.2.10
  • OS and version: macOS Monterrey v12.5.1
  • Python version (& distribution if applicable, e.g. Anaconda): 3.10.9

Code Snippet

from django.db import models

class User(models.Model):
    email = models.EmailField(max_length=250)

class Notification(models.Model):
    message = models.CharField(max_length=250)

class Media(models.Model):
    file = models.FileField()

def do_things(user_id: int, notification_id: int, media_id: Media) -> None:
    try:
        user = User.objects.get(id=user_id)
        note = Notification.objects.get(id=notification_id)
        media = Media.objects.get(id=media_id)
    except User.DoesNotExist:
        raise Exception(f"User with id {user_id} does not exist.")
    except Notification.DoesNotExist:
        raise Exception(f"Notification with id {notification_id} does not exist.") # <- pylance marks this as not reached
    except Media.DoesNotExist:
        raise Exception("Example exception") # <- also marked as unreachable

Expected behavior

Do not mark the following exceptions as unreachable code

Actual behavior

From the second DoesNotExist exception the code is marked as unreachable.

@erictraut
Copy link
Contributor

Please provide a self-contained, minimal code sample that demonstrates your issue. The code sample above references many symbols that are not stdlib symbols. You are presumably importing them from another file or defining them outside of the code snippet. Without the additional context, it's difficult to diagnose your problem.

@judej judej added the waiting for user response Requires more information from user label Feb 7, 2023
@FMZPNP
Copy link
Author

FMZPNP commented Feb 8, 2023

Please provide a self-contained, minimal code sample that demonstrates your issue. The code sample above references many symbols that are not stdlib symbols. You are presumably importing them from another file or defining them outside of the code snippet. Without the additional context, it's difficult to diagnose your problem.

Sorry. I've just updated it with additional context and the version of the Django package that we are using. Anything you need, just let me know and I will get back to you as soon as possible. Thank you.

@erictraut
Copy link
Contributor

Thanks for the additional information.

This looks like a bug in the django type stubs that are bundled with pylance. The django implementation dynamically registers a DoesNotExist exception class for each model class, but the stubs model all of these as the same class. Pyright (the static type analyzer that underlies pylance) therefore concludes that the three except clauses are targeting the same exception class.

We can fix this by updating the bundled type stubs. In particular, we should update the Model class and replace class DoesNotExist(ObjectDoesNotExist): ... with DoesNotExist: ClassVar[type[ObjectDoesNotExist]].

@debonte debonte added typestub Issue relating to our bundled type stubs and removed waiting for user response Requires more information from user labels Feb 10, 2023
@brdacost
Copy link

brdacost commented Mar 10, 2023

Are there any updates on this matter? I'm also facing this issue, but It only showed up suddently on my VS Code, maybe it was something on Pylance's latest update? ( I'm on version 2023.3.10 (1 March 2023) )

@debonte
Copy link
Contributor

debonte commented Mar 16, 2023

I mistakenly fixed this in https://github.com/microsoft/python-type-stubs instead of https://github.com/sbdchd/django-types.

A new PR is pending.

@debonte debonte removed the fixed in next version (main) A fix has been implemented and will appear in an upcoming version label Mar 16, 2023
kodiakhq bot pushed a commit to sbdchd/django-types that referenced this issue Mar 16, 2023
Addresses microsoft/pylance-release#3918

As currently defined, type checkers see the `Model.DoesNotExist` and `Model.MultipleObjectsReturned` exception types as being the same on all `Model` types.

I'm replacing the exception `class` declarations on `Model` with `ClassVar` so each `Model` type will have a distinct set of exception types, allowing the usage below without type checkers thinking that the later `except` clauses are unreachable.

```python
from django.db import models

class User(models.Model):
    email = models.EmailField(max_length=250)

class Notification(models.Model):
    message = models.CharField(max_length=250)

class Media(models.Model):
    file = models.FileField()

def do_things(user_id: int, notification_id: int, media_id: Media) -> None:
    try:
        user = User.objects.get(id=user_id)
        note = Notification.objects.get(id=notification_id)
        media = Media.objects.get(id=media_id)
    except User.DoesNotExist:
        raise Exception(f"User with id {user_id} does not exist.")
    except Notification.DoesNotExist:
        raise Exception(f"Notification with id {notification_id} does not exist.") # <- Pylance marks this as unreachable
    except Media.DoesNotExist:
        raise Exception("Example exception") # <- Pylance marks this as unreachable
```
@debonte debonte added the fixed in next version (main) A fix has been implemented and will appear in an upcoming version label Mar 16, 2023
@debonte
Copy link
Contributor

debonte commented Mar 17, 2023

This issue has been fixed in prerelease version 2023.3.23, which we've just released. You can find the changelog here: CHANGELOG.md

@debonte debonte closed this as completed Mar 17, 2023
@mateenkasim
Copy link

I am obviously late to this, but could I ask why pylance uses a fork off of typeddjango/django-stubs that is so many commits behind the parent? This issue is an example of something that was already fixed in the parent, leading to duplicate work.

@debonte
Copy link
Contributor

debonte commented Mar 17, 2023

why pylance uses a fork off of typeddjango/django-stubs that is so many commits behind the parent?

@bschnurr would know better, but I believe this issue mentioned in the README is the main reason:

Note: this project was forked from https://github.com/typeddjango/django-stubs with the goal of removing the mypy plugin dependency so that mypy can't crash due to Django config, and that non-mypy type checkers like pyright will work better with Django.

Happy to discuss if this is no longer true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed in next version (main) A fix has been implemented and will appear in an upcoming version typestub Issue relating to our bundled type stubs
Projects
None yet
Development

No branches or pull requests

6 participants