search

Zac-HD / shed

`shed` canonicalises Python code. Shed your legacy, stop bikeshedding, and move on. Black++
https://pypi.org/project/shed/
GNU Affero General Public License v3.0
343 stars 23 forks source link

`shed --refactor` should only convert `== True` to `is True` within an `if` or `while` #74

Closed Zac-HD closed 1 year ago

Zac-HD commented 1 year ago

shed --refactor has a small pass that replaces == comparisons to True, False, or None with the is operator:

https://github.com/Zac-HD/shed/blob/f3653b21987172d4c7b3b369d8067116f5edc919/src/shed/_codemods.py#L137-L145

In many cases this is purely better style... but in some it's actually a semantic change. In https://github.com/Zac-HD/shed/commit/570386a6810ed3ac44e1c6568229fab336f0f148 I disabled this inside indexing contexts which un-broke (most) Pandas code; but https://github.com/Zac-HD/shed/issues/73#issuecomment-1433420561 points out that this pattern is also used by SQLAlchemy.

I propose that we should only change these expressions if they are the test argument in an If, While, or IfExp node. This might be a bit fiddly to implement, but should be quite precise.

DRMacIver commented 1 year ago

I don't know if this ever matters in real code, but this is semantics changing in a fairly boring way for numpy arrays. if x == True: will error if x is a numpy array with more than one element, while if x is True will not.

It's also easy to construct classes where this changes the meaning silently (basically anything that has the same behaviour as numpy arrays but rather than erroring interprets __bool__ as any or all), although again I don't know if that ever matters in practice.

DRMacIver commented 1 year ago

I don't know if this ever matters in real code, but this is semantics changing in a fairly boring way for numpy arrays. if x == True: will error if x is a numpy array with more than one element, while if x is True will not.

Wait sorry this is more serious. np.array([1]) will give a different non-error answer here.

DRMacIver commented 1 year ago

As, for that matter, will 1.0 == True.

I think this refactoring is just fundamentally unsafe.

Zac-HD commented 1 year ago

Yeah, OK, let's just delete it entirely.

DRMacIver commented 1 year ago

One thing maybe to consider is whether to keep it for is None.

It's certainly easy to construct examples where this is not semantics preserving, but I can't think of any examples of this happening in practice.