Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bicknellr
Copy link
Contributor

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

case "ifDefined": {
// Example: html`<img src="${ifDefined(imageUrl)}">`;
// Take the argument to ifDefined and remove undefined from the type union (if possible).
// This new type becomes the actual type of the expression
const actualType = lazy(() => {
if (args.length >= 1) {
const returnType = toSimpleType(checker.getTypeAtLocation(args[0]), checker);
return removeUndefinedFromType(returnType);
}
return undefined;
});
return {
kind: "ifDefined",
actualType,
args
};
}

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):

return removeUndefinedFromType(returnType);

@bicknellr
Copy link
Contributor Author

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:

const { typeB } = extractBindingTypes(assignment, context);

const typeBInferred = inferTypeFromAssignment(assignment, checker);

return checker.getTypeAtLocation(assignment.expression);

in the new test:

tsTest("Return type of `ifDefined` is not `null`", t => {
const { diagnostics } = getDiagnostics(`
const ifDefined = <T>(x: T) => x ?? "non-null";
html\`<input some-attribute="$\{ifDefined(Math.random() < 0.5 ? 123 : null)}" />\`;
`);
hasNoDiagnostics(t, diagnostics);
});

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant