Thanks for the PR,@bradzacher!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently onhttps://opencollective.com/typescript-eslint.

☁️ Nx Cloud Report

CI is running/has finished running commands for commit3c387a9. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Sent with 💌 fromNxCloud.

✅ Deploy Preview fortypescript-eslint ready!

Name	Link
🔨 Latest commit	4c36891
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/64a7905efc36a000084beac7
😎 Deploy Preview	https://deploy-preview-6397--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to yourNetlify site configuration.

This is now "check complete" - all existing tests are passing as are the new tests.
Need to rebuild the fixer logic though.

This is going to be a big chunk of the logic to figure out the correct places to put an optional chain 🤔

☁️ Nx Cloud Report

CI is running/has finished running commands for commit4c36891. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Sent with 💌 fromNxCloud.

@typescript-eslint/triage-team - this is functionally complete in terms of raw logic.
However it is missing the fixer logic because:

1. frankly it's not a small piece of work to re-implement it.
2. it's the bit that made the rule unsafe to begin with (x != null && x.y returnsfalse | T, butx?.y returnsT | undefined)

In some future state I can definitely see us including:

• an autofixer if we detect it's safe (eg if it doesn't change the types), or
• a suggestion fixer in other cases

But I don't know if it's worth "holding up" this change to re-implement. Happy to do it if we think it's necessary, if not then we can review and land this as is.

😞 I do dislike removing functionality. Maybe we could put this out as an experimental rule without deleting the existing one?

Maybe we could put this out as an experimental rule without deleting the existing one?

That would create a worse state, IMO - as we would have two nearly identical rules that are mostly differentiated by having a fixer or not. What would be the value prop for a user to switch to test the rule? We'd need a breaking change to remove the experimental rule name.

Also "experimental" implies instability or that it may not be adopted - but the rule is perfectly stable (passes all the existing tests, and more) andwill be adopted - it just doesn't have the previously available (unsafe) fixer.

Yeah that's a good point.

Heh, I'm torn here. Expecting previously (perhaps slightly incorrect) satisfied users to be annoyed by the lack of autofixer.

Given that the autofixer today is unsafe, I think it's reasonable for us to say"we've rewritten and removed it for safety - welcoming community PRs to add it back".

It's really sad to see the fixer being removed, my team absolutely loved it when I turned the rule on since we worked on a code-base full of partial objects (and different patterns to deal with them).
I've never even once saw a case of false positive fix of the typeobject | false.
I did see a few false positives returningundefined instead ofnull (from nullish objects) but they only affected tests asserting on null, the real code had falsy checks catching both.

...maybe we could expose the old one under a name like "deprecated"?

I've never even once saw a case of false positive fix of the type object | false.

Also applies to0 | object and'' | object.
It's rare, for sure, but we've had a few users report it - which means it's not as rare as you might hope in the wild.

The bigger issue is the conversion of return types -x != null && x.y returnsfalse | T, whereasx?.y returnsundefined | T. In a lot of cases this distinction isn't going to matter - eg in boolean locations likeif or logical expressions, or in expression statements. But there are places where this can matter and the autofix will cause errors. Again we've had some reports of this!

Honestly I'd be happy with having a flag likeallowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing which enables people to opt-in to an unsafe fixer - the issue is that it's just a bigger chunk of work to build the fixer.

Fun edge case I just discovered:

declareconstfoo:{bar:number}|null;foo===null||foo.bar;foo!==null&&!foo.bar;

In the first casefoo.bar is seen by my code as a "truthy boolean" check - which isn't a valid operand in an "OR" chain.
Similarly in the second case!foo.bar is seen as a "falsy boolean" check - which isn't a valid operand in an "AND" chain.

Which means that my code will ignore it and try not to include it in the chain to report and fix.

Will need to add special case logic to handle this as it's obviously intended to be chained together.

This also opens up another question about how this rule functions (that we can probably punt on for now, but worth mentioning).
Currently the rule will ignore operands with "invalid" checks. For example:

foo&&foo.bar&&foo.bar.baz===1// rule will currently fix tofoo?.bar&&foo.bar.baz===1

Should the rule inspect through these "invalid" checks? Or should it ignore them entirely?
I think there's a subset of them that it should include

• !==/===/!=/== comparing against anything
• not>/>=/</<= because TS will hard error if one of the operands is nullish.
• nottypeof becausefoo && typeof foo.bar will returnundefined | typeof foo.bar whereastypeof foo?.bar will returntypeof foo.bar | 'undefined' - which is VERY different

Should the rule inspect through these "invalid" checks? Or should it ignore them entirely?

...hmm. My instinct is to ignore "invalid" checks as an odd edge case. If users complain we can add it as a bugfix later on - but if the code is roughly not valid, I'd hesitate to feel confident in making changes.

Note: I hate the tests with a passion because it's impossible for anyone to understand them as written without checking out and running the test 😢
However without utilities it'sreally hard to write and maintain the 700 cases with lots of repeated code.
Sigh this is a big pet peeve I have with ESLint rule tests. There has to be a better way...

Snapshot output would make it much simpler to review as you can see the expected reports and fixes properly rather than doing it opaquely.

The test coverage should show how much is covered overall - which should be all of the code!