ipatalas / vscode-postfix-ts

Postfix notation for TypeScript/Javascript - extension for VS Code
MIT License
158 stars 43 forks source link

Skip unnecessary scopes for `.not` #108

Closed zardoy closed 1 year ago

zardoy commented 1 year ago

I was thinking about this for a long time. I don't think .not should suggest to invert identifier in this locations:

// simply invalid
if (tab.input instanceof TabInputText) continue
// suggesting to invert b here is annoying for me for a long time
if (a === b) continue

// if (a === !b) continue TS allows this only if both sides are of type boolean, but again, why don't just use `!==` ? 
ipatalas commented 1 year ago

Well, couldn't agree more, I have nothing in my defense :) I'll add a test case for that and see how easy it is to fix it.

ipatalas commented 1 year ago

Ok, had a quick look on the first one (instanceof) and it seems like the only valid template there would be await because in theory you can do:

if (await tab instanceof TabInputText) {}
// or
if (await fun() instanceof TabInputText) {}

Rather unlikely but still legal: image

ipatalas commented 1 year ago

Hmm, interesting. We're back to recent problem with using full node for analysis :D I reverted back to using only the code before the dot for most of the cases except type references when it had to use full node to get full context. It's a similar problem now because code before the dot has no idea it's in a instanceof context because instanceof is after the dot. Passing full context though is not as helpful as it should because the dot is changing the context a bit: image

First line is all good, AST looks perfect. The second one though is no longer a binary expression but rather a property access expression (well, technically true: expr.instanceof)

I am now torn between just implementing it in a little hacky way (so using full node and leveraging property access expression) or making another rework to do it properly (trim the dot before building AST to have ideal AST shape). For now I think I'm gonna do it the hacky way because that rework can possibly require a lot of fixing for all templates (AST will possibly change).

zardoy commented 1 year ago

Also there is still issue with arrow callbacks eg:

fn = () => {
    if (b{not}) return // suggests to invert function body O_o
}

(can you look, maybe its easier to fix 😃)

trim the dot before building AST to have ideal AST shape

What about transforming ast instead? The idea of transformers in TS is not new, it already has transformers eg to do d.ts from .ts