-
Notifications
You must be signed in to change notification settings - Fork 26.3k
[jit] Improve error message for missing attribute #20779
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
Conversation
facebook-github-bot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Chillee has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Chillee has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
Good start! I think this new error is actually inaccurate in the case where someone has explicitly assigned a NoneType to a module attribute: Let's see if we can put this error message in a branch on
Then, we'll have the old message (directing the user to add the attr to __constants) when there's an explicit None attribute, but the new message when we truly don't have the attribute. |
|
Nice catch - didn't realize that those had the same behavior in Python. Added your example in a test. |
jamesr66a
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Let's see if the tests pass and then I'll approve
c8acadd to
c70cc6f
Compare
|
I believe the remaining failing tests are flaky/broken. |
facebook-github-bot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Chillee is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Fixes #20495 .
Now for
it does
I added a test in
test_jit.pyas well.