coderaiser / putout

🐊 Pluggable and configurable JavaScript Linter, code transformer and formatter, drop-in ESLint superpower replacement 💪 with built-in support for js, jsx, typescript, flow, markdown, yaml and json. Write declarative codemods in a simplest possible way 😏
https://putout.cloudcmd.io/
MIT License
705 stars 40 forks source link

Package: conditions/convert-comparison-to-boolean - object properties marked as constant #140

Closed epreston closed 1 year ago

epreston commented 1 year ago

Package: conditions/convert-comparison-to-boolean Issue: non constant members are seen as constant by this rule

Example:

// constant object reference
const constObjRef = {
  notConstantProp: 0,
}

// we can update the property at any point
constObjRef.notConstantProp = 100;

// this rule sees this as a constant comparison: 0 > 40
// it is actually a comparison of: 100 > 40
if (constObjRef.notConstantProp > 40) {
  // do something
}

// the rule wants to replace it with
if (false) {
  // do something
}

// but the actual result is true during execution
if (true) {
  // do something
}

// it should just leave this alone because it can be updated at any time
if (constObjRef.notConstantProp > 40) {
  // do something
}

I am guessing that this rule sees the constant declaration of the reference to the object and treats all members as constant.

epreston commented 1 year ago

Issue reported on 29.3.0

Retesting for 29.4.0 update.

coderaiser commented 1 year ago

It is related to:

https://github.com/coderaiser/putout/blob/aec7696bdfadac022f760e006220bc80b3680105/packages/operate/lib/compute.js#L52-L75

Do you have ideas how to fix?

epreston commented 1 year ago

The way I would fix is by excluding property accessors from this rule.

I've only given it 10 minutes thought but, I can't think of how these would can be made constant.

epreston commented 1 year ago

Even a typescript enum gets boiled down to a mutable object.

Typescript example:

enum Direction {
  Up,
  Down,
  Left,
  Right
}

Gets converted to this javascript

const Direction = {
  Up: 'Up',
  Down: 'Down',
  Left: 'Left',
  Right: 'Right'
};

Only things know of is:

epreston commented 1 year ago

Issue might be with:

https://github.com/coderaiser/putout/blob/aec7696bdfadac022f760e006220bc80b3680105/packages/operate/lib/compute.js#L84

coderaiser commented 1 year ago

compute tries to determine value of AST node, you can change the code and check if it affects all 🐊Putout tests, if not we can change implementation.

But better to start from #139, it is much simpler.

epreston commented 1 year ago

Sounds good. I'll start from there.

coderaiser commented 1 year ago

Fixed with 20f19c8, landed in @putout/operate@8.10.0 🎉 . Please re-install 🐊Putout.

Is it works for you?