runem / lit-analyzer

Monorepository for tools that analyze lit-html templates
MIT License
317 stars 36 forks source link

The `ifDefined` directive should return a non-null type, not just non-`undefined`. #296

Open bicknellr opened 1 year ago

bicknellr commented 1 year ago

The return type of ifDefined is determined with a special rule here:

https://github.com/runem/lit-analyzer/blob/92c084da4d33d6fbdd8b29603d2003387e65dbb3/packages/lit-analyzer/src/lib/rules/util/directive/get-directive.ts#L45-L63

But the analyzer doesn't realize that the return type can't be null in Lit 2, because it only removes undefined (matching Lit 1's behavior):

https://github.com/runem/lit-analyzer/blob/92c084da4d33d6fbdd8b29603d2003387e65dbb3/packages/lit-analyzer/src/lib/rules/util/directive/get-directive.ts#L52

bicknellr commented 1 year ago

I think it would be better if Lit 2's ifDefined didn't run through this path at all, since it doesn't have any special semantics, but TS seems to be returning the wrong type from checker.getTypeAtLocation(assignment.expression) during this stack:

https://github.com/runem/lit-analyzer/blob/92c084da4d33d6fbdd8b29603d2003387e65dbb3/packages/lit-analyzer/src/lib/rules/no-nullable-attribute-binding.ts#L24

https://github.com/runem/lit-analyzer/blob/92c084da4d33d6fbdd8b29603d2003387e65dbb3/packages/lit-analyzer/src/lib/rules/util/type/extract-binding-types.ts#L32

https://github.com/runem/lit-analyzer/blob/92c084da4d33d6fbdd8b29603d2003387e65dbb3/packages/lit-analyzer/src/lib/rules/util/type/extract-binding-types.ts#L69

in the new test:

https://github.com/runem/lit-analyzer/blob/942db2937d4f525dcc6fc61ff7c2398c444d5dc9/packages/lit-analyzer/src/test/rules/no-nullable-attribute-binding.ts#L29-L35

assignment.expression is the Node with text ifDefined(Math.random() < 0.5 ? 123 : null) but the result of checker.getTypeAtLocation(assignment.expression) gives the type "non-null" | 123 | null despite null not being possible. Not sure what's going on there yet.