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

Check non-undefined properties are initialized in the constructor with `--strictNullChecks` #8476

Closed
mhegazy opened this Issue May 5, 2016 · 75 comments

Comments

Projects
None yet
@mhegazy
Copy link
Contributor

mhegazy commented May 5, 2016

class C {
   p: number; // Should be an error under --strictNullChecks
   method() {
    this.p; 
  }
}
@ahejlsberg

This comment has been minimized.

Copy link
Member

ahejlsberg commented May 5, 2016

Since we're not able to track assignment effects between methods, we'd break code that calls a separate initialize method (or some such) to initialize instance data. I'm not a fan of that pattern, but I have certainly run across it.

Also, presumably we want to permit uninitialized properties in abstract classes. That means we'd have to check not just locally declared properties but also inherited properties in non-abstract classes.

@mhegazy

This comment has been minimized.

Copy link
Contributor

mhegazy commented May 5, 2016

under a flag then?

@ahejlsberg

This comment has been minimized.

Copy link
Member

ahejlsberg commented May 5, 2016

Yeah, probably something like --allowUninitializedProperties, but since it only happens under the new --strictNullChecks I suppose we could wait on adding the flag and gauge the feedback.

@ahejlsberg

This comment has been minimized.

Copy link
Member

ahejlsberg commented May 11, 2016

Having looked at several code bases, I don't think it is feasible to implement this feature as we have discussed it. There are simply too many common scenarios where users would need to turn the feature off if we insist that the constructor itself must initialize every non-nullable property. Effectively, it would become more of an irritant than a help.

@ahejlsberg ahejlsberg closed this May 11, 2016

@ahejlsberg ahejlsberg added Won't Fix Suggestion and removed Bug labels May 11, 2016

@malibuzios

This comment has been minimized.

Copy link

malibuzios commented May 12, 2016

ummm....

class NonAbstractClass { // <-- Non-abstract class, so can be instantiated by itself
    readonly imNotInitialized: number; // <-- can only be initialized here

    constructor() {
        // <-- or here
    }
}
class Base {
    readonly imNotInitializedToTheRightType: any;

    constructor() {
        this.imNotInitializedToTheRightType = "hi";
    }
}

class Derived extends Base {
    readonly imNotInitializedToTheRightType: number; // <-- is this initialized?

    constructor() {
        super();
    }
}
@malibuzios

This comment has been minimized.

Copy link

malibuzios commented May 14, 2016

@ahejlsberg

I think I've shown one common scenario where this would be a help, not an irritant: namelyreadonly members in a non-abstract class, and one less common, but probably more 'severe' one: inherited members initialized to incompatible or less capable types than the type they are redeclared to in a derived class.

I haven't yet got any response from you on this. Why would you think it would be an irritant here?

(also, I haven't got any responses for the questions about inheritance of readonly members by writable members and vice-versa. It has been a week so far, and neither you or anyone of the team have shown serious intentions to engage in that discussion. I can't really see a reason why not - if your original reasoning was solid it should be relatively easy to explain..)

@ahejlsberg

This comment has been minimized.

Copy link
Member

ahejlsberg commented May 14, 2016

Yes, we might be able to do something for readonly members without it being an irritant (since we already only allow assignments in initializers or the constructor).

@peterkelly

This comment has been minimized.

Copy link

peterkelly commented Jun 5, 2016

I'd be in favour of Swift's approach. A constructor is required to initialise all non-nullable properties. The compiler checks all branches to make sure this happens in every case.

As a simple example, here's a Swift class with a non-nullable field. It's required to have a constructor, because otherwise there's no way of initializing the field (this would not be the case if the type was String? instead of String). This gives the error Class 'Person' has no initializers:

class Person {
    var name: String;
}

If we add an init method, but don't initialize the field, we get the following error: Return from initializer without initializing all stored properties

class Person {
    var name: String;
    init() {
    }
}

If we have an init method which initializes the field in some branches but not others, the same error is reported: Return from initializer without initializing all stored properties

class Person {
    var name: String;
    init(theName: String, setName: Bool) {
        if setName {
            name = theName;
        }
    }
}

The compiler thus enforces you to initialize it in all branches:

class Person {
    var name: String;
    init(theName: String, setName: Bool) {
        if setName {
            name = theName;
        }
        else {
            name = "Unknown";
        }
    }
}
@peterkelly

This comment has been minimized.

Copy link

peterkelly commented Jun 5, 2016

I'd also add that if the purpose of non-nullable types is to ensure that a value (be it an argument, variable, or property) can never be null, then the lack of such checks in constructors effectively defeats the purpose of the feature. Swift is very strict on this matter and IMHO gets it right - if you tell the compiler that the type of a given value cannot be null, you are guaranteed this will be the case.

Omitting such checks on properties at initialization time severely limits the utility of this feature, particularly given that it's likely that potentially uninitialised properties can be assigned to values which have a non-nullable type, thus violating the guarantees that the type system supposedly guarantees.

In the following code, s will be given the (non-nullable) type string, but its actual value will be undefined.

class Person {
    public name: string;
}

const p = new Person();
const s = p.name;
@debuuu

This comment has been minimized.

Copy link

debuuu commented Jun 14, 2016

Is this feature definitely not getting implemented then? I've been really excited about strictNullChecks since I first heard TS would introduce it, but for me this exception would pretty much be a deal breaker. I feel like a feature like this needs to be all or nothing. Other languages that deal with this, like Rust, are entirely consistent with their approach.

I also think that anyone who's irritated by this will also be irritated enough in general about strict null checking, that they are likely not to use it anyway.

@nippur72

This comment has been minimized.

Copy link

nippur72 commented Jul 18, 2016

Please re-consider adding this feature (default off and turned on with --noUninitializedClassProperties).

IMO this is a big potential source of errors, have stumbled on this issue only 4 days after switching to strict null checks (which is awesome btw).

@raveclassic

This comment has been minimized.

Copy link

raveclassic commented Oct 9, 2017

What is the status of this? I see that the issue was dropped from TS2.6 milestone recently. Is this because of some latest design decision or the status is just unknown?

@mhegazy

This comment has been minimized.

Copy link
Contributor

mhegazy commented Oct 9, 2017

What is the status of this? I see that the issue was dropped from TS2.6 milestone recently. Is this because of some latest design decision or the status is just unknown?

it is on our list of features to support. we do not have an ETA for it at the time being.

@mgenware

This comment has been minimized.

Copy link

mgenware commented Nov 5, 2017

OK, as pointed out by someone in another thread, this is one of the non-goals of TypeScript.


I'd suggest that, under some flags, instead of warning user of type safety caused by the default undefined value, give it a default value like C#, if the default value can't be determined, emit an error.

// under a flag like `--auto_init_properties`
class Person {
  id: number; // auto initialized to 0
  name: string; // auto initialized to ""
  description: string|null; // auto initialized to null

  // **Compilation error: Can't determine default value for this prop, all properties must be initialized. **
  opt: string|number|Option; 

  // To fix this, user must specify an value, for example:
  opt: string|number|Option = 'abc';
  opt: string|number|Option = new Option();
}

In this way, all properties are actually defined in runtime:

const obj = new Person();
for (const property in obj) {
  console.log(property);
}

// Output: id, name, description, opt

Also, we don't need to write boilerplate code to enforce strict type safety:

class Person {
  constructor {
    this.id = 0;
    this.name = '';
    this.description = null;
    this.opt = new Option();
  }
}

Started a new issues at #19750

@ahejlsberg

This comment has been minimized.

Copy link
Member

ahejlsberg commented Nov 16, 2017

This suggestion is now implemented by #20075.

@mhegazy mhegazy added this to the TypeScript 2.7 milestone Nov 17, 2017

@whitneyland

This comment has been minimized.

Copy link

whitneyland commented Nov 17, 2017

@ahejlsberg thank you for keeping an open mind. all the work of the team over some big milestones has been greatly appreciated.

@peterkelly

This comment has been minimized.

Copy link

peterkelly commented Nov 17, 2017

A huge win for TypeScript. Thank you so much for listening to feedback and for your efforts on this!!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.