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

Remove the non-null assertion operator #9637

Closed
chrisprice opened this issue Jul 12, 2016 · 7 comments
Closed

Remove the non-null assertion operator #9637

chrisprice opened this issue Jul 12, 2016 · 7 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@chrisprice
Copy link

Whilst quite hard to search for so I may have missed something, the only justification for the non-null assertion operator I can find is in the design notes #7395 -

Non-null assertion operator

  • A new ! postfix expression-level operator.
  • Produces no runtime-dependent code.
// Compiled with --strictNullChecks
function validateEntity(e: Entity?) {
    // Throw exception if e is null or invalid entity
}
function processEntity(e: Entity?) {
    validateEntity(e);
    let s = e!.name;  // Assert that e is non-null and access name
}
  • Why can't a type predicate take care of that?
  • It could but it's just cumbersome for some scenarios.

So a type predicate would look look like -

function validateEntity(e: Entity?): e is Entity {
    if (e == null) {
        throw new Error();
    }
    return true;
}

 function processEntity(e: Entity?) {
     if (validateEntity(e)) {
         let s = e.name; // Guard asserts e is an Entity (and therefore non-null) and access name
     }
 }

Or implicitly -

 function processEntity(e: Entity?) {
     if (e != null) {
         let s = e.name; // Guard guarantees e is non-null and access name
     }
 }

But am I right in thinking that a type assertion would also work?

 function processEntity(e: Entity?) {
     validateEntity(e);
     let s = (<Entity>e).name; // Assert e is an Entity (and therefore non-null) and access name
 }

If so, this would be my default expectation of how this would work. Adding the new operator means -

  • A new (but fundamentally almost equivalent) operator to learn.
  • When reviewing code, as I would generally treat a type assertion as a code-smell, I now also have to learn to treat ! in the same way.
  • Potential conflict with JavaScript should it ever adopt it as an operator.

Adding this new operator seems like an extreme step to support an edge case with so many existing solutions. However, maybe I'm missing a killer scenarios where this would be useful?

@JabX
Copy link

JabX commented Jul 12, 2016

There are a lot of cases that aren't covered by a simple type predicate, especially when it involves multiple variables whose values are linked (like if one variable is undefined, the other can't be, or if both variables must be defined or undefined at the same time). You might even want to do these guards (and throw errors) in the constructor of a class and use the result of these guards in class methods, without having to cast everytime. And casting can be verbose with complex types too.

When I converted some of my code to --strictNullChecks, I didn't know of this ! operator, and I had some problems because I wanted to be able to do just that sometimes. Then, I decided to look in the issues to see if other people wanted this too, and I discovered it was indeed already there.

Consider this example:

function test(store?: Store, nodes?: string[]) {
    if (!store && nodes || !nodes && store) {
        throw new Error("store and nodes must be both defined or undefined");
    }

    return (nodes || []).map(node => store!.get(node));
}

I have a lot of this in my code and I really appreciate being able to use ! here, because I am sure it can't be undefined but the compiler can't know that.

@chrisprice
Copy link
Author

Thanks @JabX, I hadn't appreciated those particular limits of the type inference. Although I'm still unsure why the ! operator is preferential to a type assertion in this case -

// Adding interface for completeness
interface Store { get: (string) => number }

function test(store?: Store, nodes?: string[]) {
    if (!store && nodes || !nodes && store) {
        throw new Error("store and nodes must be both defined or undefined");
    }

    return (nodes || []).map(node => (<Store>store).get(node));
}

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Jul 12, 2016
@RyanCavanaugh
Copy link
Member

A type assertion instead in that position to a short type name is relatively benign. But consider something like this:

  let x = foo!.bar.baz!.toString();

which might turn into

  let x = (<Some.Namespace.Baz>(<{ bar: Something }Foo>foo).bar).baz).toString();

which has several disadvantages:

  • Super hard to read (the types are in the wrong order, though you could mitigate with as)
  • You have to hardcode in the type names, so if there's a refactoring that is otherwise benign to this expression, you might have a code break when you wouldn't otherwise
  • The types of the subexpressions may not have names, requiring you to write an error-prone inline type literal

@chrisprice
Copy link
Author

Thanks for responding @RyanCavanaugh. I understand you've closed this issue so I'm not going to press it other than to respond to the disadvantages -

  • In the example quoted it is very hard to read but I would argue this is a worst case which mixes deep nested access, namespaces and anonymous types and is unlikely to be realistic. It's an interesting use of as though, I'd never considered it's use as a trailing equivalent.
  • Equally what if there's an otherwise benign refactoring which leads to the assumption behind the use of ! being invalidated?
  • I would expect to name the type rather than use an error-prone inline type literal.

Navigating deeply into a tree structure with nullable fields without intermediate guards really doesn't feel safe to me. I wonder if there's been too much focus on ASTs which atypically contain such guarded deep nested accesses.

@pensacola1989
Copy link

pensacola1989 commented Nov 25, 2016

interface Parent {
    Child?: {
        Name?: string;
    }
}

let p: Parent = {}


// without non-null assert
let processParent = (p?: Parent): void => {
    if (!p || !p.Child || !p.Child.Name) {
        return;
    }
    console.log(p.Child.Name);
}

// with non-null assert
let processParent2 = (p?: Parent): void => {
    console.log(p!.Child!.Name);
} 

and the js that emmited below

var p = {};
// without non-null assert
var processParent = function (p) {
    if (!p || !p.Child || !p.Child.Name) {
        return;
    }
    console.log(p.Child.Name);
};
// with non-null assert
var processParent2 = function (p) {
    console.log(p.Child.Name);
};

please run at playground on typescriptlang.org, I can still get 'undefined',it seems compiler do nothing, maybe i'm misunderstand the point, need help

@JabX
Copy link

JabX commented Nov 26, 2016

! is just a type annotation, it doesn't change the emitted code at all.

@pensacola1989
Copy link

need more sample code thx

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

4 participants