Skip to content

Improve NameError error suggestion for instances #99139

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

Closed
pablogsal opened this issue Nov 5, 2022 · 5 comments
Closed

Improve NameError error suggestion for instances #99139

pablogsal opened this issue Nov 5, 2022 · 5 comments
Labels
3.12 only security fixes type-feature A feature request or enhancement

Comments

@pablogsal
Copy link
Member

pablogsal commented Nov 5, 2022

If we are in a method and a NameError is raised for the name X, first check if X is an attribute of the instance as self.X is likely more common than whatever closest match we find in the scope.

@hauntsaninja
Copy link
Contributor

This is great! I learnt C++ before Python so I used to forget self all the time when I first started programming in Python. Thank you!

@kumaraditya303 kumaraditya303 added type-feature A feature request or enhancement 3.12 only security fixes labels Nov 6, 2022
@sobolevn
Copy link
Member

sobolevn commented Nov 6, 2022

Looks awesome! Lots of begginers going to love that!

What about a case like:

class Some:
   x = 1
   @classmethod 
   def method(cls):
       x + 2  # Should be cls.x

Do we care to support it? 🤔

@pablogsal
Copy link
Member Author

Looks awesome! Lots of begginers going to love that!

What about a case like:

class Some:

   x = 1

   @classmethod 

   def method(cls):

       x + 2  # Should be cls.x

Do we care to support it? 🤔

Thanks for the proposal!

I will consider it but is unlikely that will happen as we are not interested in supporting all possible variations of this, specially since the name of the variable here (cls) is less standardised than "self" and is less common.

Also, this will force us to distinguish the type of function to avoid inspecting random variables called "cls".

@EwoutH
Copy link

EwoutH commented Nov 7, 2022

Thanks for this, and improving error messages in general!

@ghost
Copy link

ghost commented Oct 19, 2023

In doing so, we are making a naive insertion that self.* is implied to be in scope. I'm not sure if this always true, and some long time ago the mantra was 'explicit' not 'implicit'.

'''first check if X is an attribute of the instance as self.X'''

Dangerous Reflection in error messages? Isn't this sort of what happened in when log4j2

Is there any concern of instance property values leaking through side channel? I agree that the "hint" is nice, however, might there be invocations where str X has been intentionally flattened as a means of input validation? Here is a rough example of using a control to ensure that X is stripped of nested namespaces. The control (say a lambda function) simply returns only what is on the right side of the last period character.

[lambda func arg dirty X] "self.secret.namespace.layers.foo"". # quoted string comes from any untrusted IO

[lambda func return clean X] return X.rpartition('.')[-1] # injected instance namespace inferred above has now been stripped so that in the lambda return X = "foo" (assume GIL, atomicity, and inlining X)

IIRC, in Py <= 3.11 agnostic ValueError is idempotent w.r.t truthiness of existence "self.secret.namespace.layers.foo"

IIRC 3.12+ changes that, destroying idempotence, and creating subtle logic fork in default error message.

Possible 3 dangers
(1) increased use of reflection across an instance attributes and property values. Before change, we are only considering explicit scopes in the exec frame. After change, we are implicitly creating a namespace bridge by a new invocation of eval() in our attempt to see if we can be helpful to user.

(2) If the error is not intentionally overloaded, after the change we will naively leak implementation details that while "foo" does not exist, "self.secret.namespace.layers" actually does exist, and within it can be found are "foo" target. -> leaks implementation detail "secret.namespace.layers"

(3) This is fairly simple in ascii, but in arbitrary string encoding, a maliciously chose set of code points for X may result in a program acting in an undefined matter that it would not have if X had simply been evaluated in the original context and discarded in local garbage collection as 3.11 and prior will do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.12 only security fixes type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

5 participants