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

Non-null assertions infringe a responsibility of optional chaining #35025

Open
falsandtru opened this issue Nov 10, 2019 · 15 comments
Open

Non-null assertions infringe a responsibility of optional chaining #35025

falsandtru opened this issue Nov 10, 2019 · 15 comments
Labels
Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript

Comments

@falsandtru
Copy link
Contributor

falsandtru commented Nov 10, 2019

Because @RyanCavanaugh couldn't understand what is the problem, I reexplain it.

In the following case, a responsibility of optional chaining is making a return type Element | undefined.

// a is string | null | undefined
const a = document.querySelector('_')?.textContent;

In the following case, non-null assertion has broken the safeness made by optional chaining.

// a is string
const a = document.querySelector('_')?.textContent!;

It is obvious that optional chaining was not considered when non-null assertion operator was designed. TypeScript has to consider what is the best design and what to do via reconsidering the design of non-null assertion operator.

TypeScript Version: 3.7.x-dev.20191105

Search Terms:

Code

const a = document.querySelector('_')?.textContent!;

Expected behavior:

a is string | undefined.

Actual behavior:

a is string.

Playground Link:

Related Issues:

@jcalz
Copy link
Contributor

jcalz commented Nov 10, 2019

Duplicate of #34875, for context

@fatcerberus
Copy link

fatcerberus commented Nov 10, 2019

For the benefit of anyone else reading this, what's wanted here is for the non-null assertion in a?.b?.c! to only affect the type of c. Which I totally understand the value in, but this is simply not how the ! operator works. It would need a new operator to avoid confusing people, I think.

@RyanCavanaugh RyanCavanaugh added the Unactionable There isn't something we can do with this issue label Nov 10, 2019
@RyanCavanaugh
Copy link
Member

The non-null assertion operator has never been safe. It can't be, because it's a downcast operator.

Now, you can say that an expression of the form a?.b can't be guaranteed to be not-null, so maybe ! just "shouldn't work" on it, but:

  • You might be asserting to non-null because you're using the expression in a place that claims to not allow undefined, but actually does
  • You might be using ?. out of laziness because you know for other reasons that a shouldn't ever be undefined

We also can't really do anything about this case

const x = a?.b;
const y = x!;

without introducing some notion of "No really we can prove it's maybe undefined", which just seems a bridge too far. We get daily reports of people complaining when refactoring out an expression changes the type system behavior and don't care to introduce yet another instance of this.

@fatcerberus
Copy link

fatcerberus commented Nov 10, 2019

To play devil’s advocate a bit, I suspect it’s reasonably idiomatic to do something like

if (foo != null)
    doSomething(foo.bar!);  // we know bar won’t be null here

It would be quite tempting to replace the above with an optional chain, but doing so is not safe because of the ! operator. It would be safe with the proposed change.

I can’t speak to how common the above pattern is, but I would definitely be tempted to do the unsafe refactoring at any rate. Would be nice if the type system could point out the mistake if I went through with it.

@RyanCavanaugh
Copy link
Member

What would you intend to refactor that into?

@fatcerberus
Copy link

@RyanCavanaugh

doSomething(foo?.bar!);

I’ve actually made this mistake a couple of times already; it changes the behavior of the code, but TS typically catches it because now there’s an undefined possibility in the type that I haven’t accounted for. If there were a non-null assertion on the final property access, though, the mistake might go unnoticed for a while.

@falsandtru
Copy link
Contributor Author

falsandtru commented Nov 11, 2019

You might be using ?. out of laziness because you know for other reasons that a shouldn't ever be undefined

The problem doesn't depend on that usage. document.querySelector('a')?.textContent! makes the completely same problem. You must understand the problem without depending on that edge case.

We also can't really do anything about this case
const x = a?.b;
const y = x!;

It is wrong understanding. I'm not saying such a poor scenario. I'm explaining a?.(b!) but you are just repeating (a?.b)!. You are just explaining the current type system.

a?.(b!) // Expected scope.
(a?.b)! // Actual scope of the current type system.

The difference is obvious. You couldn't understand what is explained here is a?.(b!). You must understand this point before revealing your misunderstanding.

@jcalz
Copy link
Contributor

jcalz commented Nov 11, 2019

Oh, so the request here is for the non-null assertion operator to participate in long short-circuiting along with property accesses, method calls, and function calls. Since the non-null assertion operator is not part of JS, it's up to TS to decide whether or not to support this. Right now we don't.

I tend to agree with the current implementation. Still, it does seem a little weird for a supposedly type-system-only assertion to change the runtime behavior:

foo?.bar.baz; // foo == null ? undefined : foo.bar.baz 
foo?.bar!.baz; // (foo == null ? undefined : foo.bar)!.baz 

but long short-circuiting seems weird to me anyway, so who knows?

@falsandtru
Copy link
Contributor Author

falsandtru commented Nov 11, 2019

long short-circuiting

We can see there that optional chaining affects other operators after.

it's up to TS to decide whether or not to support this.

So I'm saying TS has to do the new decision. The old decision when non-null assertion operator was introduced is not directly applicable here. What decisions does TS make is not restricted. The present problem is TS still doesn't understand the extremely simple difference between a?.(b!) and (a?.b)!.

@RyanCavanaugh RyanCavanaugh removed the Unactionable There isn't something we can do with this issue label Nov 11, 2019
@RyanCavanaugh RyanCavanaugh reopened this Nov 11, 2019
@fatcerberus
Copy link

fatcerberus commented Nov 11, 2019

I note that there is literally no precedent whatsoever for a postfix operator which applies to only part of an expression. a?.(b!) isn't even valid syntax (or rather--it is, but it does something completely different). There's no place where you can take the expression apart to decide "Oh hey, ! should apply only to this subexpression." There are no subexpressions. a?.b is the entire expression. And if the type of that expression is T | undefined, and we remove the undefined... all that's left is T. While I agree this would be a useful change, from an engineering standpoint, I don't see how it can work.

@falsandtru
Copy link
Contributor Author

falsandtru commented Nov 11, 2019

If there is anything to consider about syntax, it would be !!. There are three options.

document.querySelector('_')?.textContent!!; // string | undefined
document.querySelector('_')?.textContent!!; // string
document.querySelector('_')?.textContent!!; // syntax error

I think !! should be invalid to make it available to define it as a new JavaScript syntax like ??.

@jcalz
Copy link
Contributor

jcalz commented Nov 11, 2019

I note that there is literally no precedent whatsoever for a postfix operator which applies to only part of an expression. `

It's not like the language has a large number of postfix operators to choose from.

More striking to me is that aside from long short-circuiting, I can't think of other code that is "unparenthesizable" in the sense that the meaning of a sub-expression changes when grouped. (edit: okay, that's mentioned here and ({x})=y is noted as being different from the destructuring assignment {x}=y.)

As far as I know, a.b.c.d is completely equivalent to ((a.b).c).d, even in potential edge cases like this context for method calls: foo.bar() and (foo.bar)() do the same thing even though let y = foo.bar; y() does not.

But long short-circuiting upends that, because now a?.b.c is not the same as (a?.b).c, despite the fact that syntactically those have the same structure. Semantically, a?.b.c acts like something you can't represent with parentheses. It's more like opt(a, a=>a.b.c); let's call it a?.⁽b.c⁾.

That feels like precedent for a?.b! to act like opt(a, a=>a.b!) or a?.⁽b!⁾. But not every operator works that way. We explicitly don't have a?.b + c act like opt(a, a=>a.b + c) / a?.⁽b + c⁾. So should ! be more like . or more like +?

@fatcerberus
Copy link

Long short-circuiting, while interesting syntactically, is not particularly surprising semantically: it is, for example, how the Maybe monad works. It’s nice to have an operator built into the language that models that for the most common cases (property access and function call) so we can all stop writing boilerplate Maybe monads 😄

@RyanCavanaugh RyanCavanaugh added Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript labels Nov 12, 2019
@falsandtru
Copy link
Contributor Author

I'm not sure what is really needed when an issue is labeled Needs Proposal because I've never seen accepted proposals nor them driving the work for implementation.

@falsandtru
Copy link
Contributor Author

Now I think ! and !! should work as follows:

const a = ''.match('')?.length ?? null; // number | null
const b = ''.match('')?.length! ?? null; // number | null
const c = ''.match('')?.length!! ?? null; // number

declare function f(a: number): void;
f(''.match('')?.length!); // error
f(''.match('')?.length!!); // ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

4 participants