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

"Stricter" TypeScript #274

Closed
RyanCavanaugh opened this issue Jul 28, 2014 · 40 comments
Closed

"Stricter" TypeScript #274

RyanCavanaugh opened this issue Jul 28, 2014 · 40 comments
Labels
Needs More Info The issue still hasn't been fully clarified Suggestion An idea for TypeScript

Comments

@RyanCavanaugh
Copy link
Member

This a meta-bug for tracking a set of things that we could address with a compiler flag that tightens certain aspects of TypeScript that users generally perceive as too loose.

Will add to this list as appropriate

Contentious issues that have been mentioned:

Done!

Not happening:

Note: This is not a "fork the language" flag. The type system itself would be unchanged under this flag; it would simply change some operations from being allowed to being errors. For example, we would not change the order of overload resolution, or change the type of null, because that would have non-local effects and everyone would have to agree on whether or not the flag was on.

@johnnyreilly
Copy link

Does it make sense for noImplicitAny to be rolled in as part of this stricter TypeScript? Or is there value keeping it separate?

@RyanCavanaugh
Copy link
Member Author

I'd like to hear feedback on that one -- would anyone want --strict without --noImplicitAny ?

@johnnyreilly
Copy link

I think --strict should include --noImplicitAny.

@basarat
Copy link
Contributor

basarat commented Jul 29, 2014

I think --strict should include --noImplicitAny.

👍

@vladimir-i
Copy link

It'd be nice to have a flag to tighten the type compatibility rules (e.g. #222)

@knazeri
Copy link

knazeri commented Jul 29, 2014

I think --strict should include --noImplicitAny. 👍

@NoelAbrahams
Copy link

This is a great meta-issue. My preference is for all these features (including noImplicitAny) to be included in the default compilation (i.e. no flags).

We could have a flag that people can use to relax the rules, for example --lenient or --dontCatchErrors (that's a joke, btw).

A --strict flag might be confused with "use strict".

@basarat
Copy link
Contributor

basarat commented Jul 29, 2014

A --strict flag might be confused with "use strict".

that is what I thought when I first looked at the title

@danquirk
Copy link
Member

Yeah it may be the case that we need to use a different term than 'strict' to minimize confusion but it's ultimately a small issue that's easy to change at any time before the feature goes into a final release.

@sophiajt
Copy link
Contributor

@NoelAbrahams - it'd need to be behind a flag to not break backward compatibility with code that's building against the current 1.0 compiler.

Not that I want to start a woodshed about naming, but I 👍 that "strict" is too overloaded, especially in the JS world.

@RyanCavanaugh
Copy link
Member Author

It's too late to make noImplicitAny the default; this would break too many people.

I agree with the sentiment that strict is overloaded in JS and is probably not the best name for the flag should it be implemented. Ideas on that front?

@basarat
Copy link
Contributor

basarat commented Aug 5, 2014

It's too late to make noImplicitAny the default; this would break too many people.

👍

Ideas on that front?

--safer or --loud

@johnnyreilly
Copy link

How about --hardcore or --optionExplicit? 😄

In all seriousness I think --safe works well as would --rigorous. Both communicate that the compiler should be more exacting but both words have positive connotations (which invite usage rather than forbid it).

@NoelAbrahams
Copy link

--ultraprecise

I also like --safer (or simply --safe)

@knazeri
Copy link

knazeri commented Aug 5, 2014

--safe sounds good, I also like --warn:<n> flag of the C# compiler to adjust the warning level for the compiler.

@robertknight
Copy link

Perhaps the important question here is not so much the name but the compatibility promise associated with the flag. Will it only enforce a certain set of restrictions forevermore or can new restrictions be added under the same flag in future?

My personal preference would be to enable new restrictions by default under such a flag but provide opt-outs on a per-module basis that can be easily enabled to keep existing code compiling under new versions of tsc. The rationale being that it will improve the quality of the code in the TypeScript ecosystem as a whole.

I'd like to see --noImplicitAny made the default but that would break the important feature that new users can convert existing JS code to TypeScript just by changing the file extension.

@NoelAbrahams
Copy link

--warn:<n> seems like a nice idea. It's a bit more future proof, and deals with @robertknight's comment about adding new restrictions.

  • warn:0 - The default. Same as compiling without noimplicitany.
  • warn:1 - Adds --noimplicitany.
  • warn:2 - Disallow use before definition, etc.
    ...
  • warn:N - A future requirement

@basarat
Copy link
Contributor

basarat commented Aug 6, 2014

C# compiler to adjust the warning level for the compiler.

@KamyarNazeri Is the error code from the compiler non-zero (e.g. 1) on warnings?

@knazeri
Copy link

knazeri commented Aug 6, 2014

The warning level for C# compiler is a zero-index value, 0 being the lowest (turns off emission of all warning messages) while higher numbers show more warnings:
http://msdn.microsoft.com/en-us/library/13b90fz7.aspx

As mentioned by others, I too believe having levels of warnings could be extensible for future use

@basarat
Copy link
Contributor

basarat commented Aug 6, 2014

Is the error code from the compiler non-zero (e.g. 1) on warnings?

The answer was on the page @KamyarNazeri mentioned for C#:

Use /warnaserror to treat all warnings as errors.

Since we want all of these to be errors perhaps we should use --error:<n> or we would need --warnAsError as well

@RyanCavanaugh
Copy link
Member Author

Our general plan is to provide simpler flags (probably just --noImplicitAny and --stricter or whatever we would call it) for tsc and let third-party build tools provide fancier stuff. Since everything will have an error code, it will be straightforward for tools like grunt-ts to provide filtering on the errors provided by the compiler.

@andrewvarga
Copy link

Not sure if this has been mentioned before, but how about an option to restrict multiple uses of interface declarations? I've run into a case where I forgot that I had already used an interface name somewhere and in this case the 2 interfaces get merged - which I understand is useful - but in some cases it would be useful to be able to say that you want this interface to not collide with any other one, so in case another exists already, throw a compile error and let you know that you need to pick another name?
Maybe with something like:

unique interface IMyInterface {}

@RyanCavanaugh
Copy link
Member Author

This is a tracking issue for other issue, all of which are in the category of changes to the type system rather than additional features or syntax. Please post suggestions for new syntax or features elsewhere. Thanks!

@fdecampredon
Copy link

perhaps this suggestion #185 could be included in the list

@RyanCavanaugh
Copy link
Member Author

Again, this is only for additional "rules", not new features or new syntax.

@danquirk
Copy link
Member

Note #1740 as a potential additional error level, although it may be too much of the 'fork the language' case and there're better solutions if we do real 'this' typing in the core language.

@samwgoldman
Copy link

I think it would be really nice if I could ask the compiler to warn me when I do this:

var data:Model = JSON.parse(json);

Instead of "silently" assigning the value of type any to data:Model, I would want the compiler to complain.

The compiler would stop complaining if I provided a type assertion. For example:

var data:Model = <Model>JSON.parse(json);

I like this because I am assuming responsibility for the type cast. If json doesn't represent data that conforms to the Model interface, that's my fault.

Thoughts?

@RyanCavanaugh
Copy link
Member Author

You can get the desired behavior by changing the return type of JSON.parse to {}. Disallowing all uses of any in its intended form seems far too strict.

@hesselink
Copy link

Since you're looking for motivating examples for disallowing function argument bivariance: we have code where we pass around constructor functions. These constructors accept an argument of an interface, but one of them incorrectly assumed it would get one specific concrete implementation of this interface instead. We expected this kind of thing to be caught but instead it was a runtime bug.

@ghost
Copy link

ghost commented Sep 13, 2015

It would be good to have ability to raise warning/error for all properties which don't have   AccessibilityModifier like private | public | protected in the stricter mode.

@afrische
Copy link

@markbook2 you can do that easily today with a regular expression( see below).
Edit: But use tslint.

Plus, this is a meta-bug, so you should probably file a new issue and reference this.

@adidahiya
Copy link
Contributor

@markbook2 @afrische TSLint has a rule for enforcing explicit visibility on class members / methods called member-access

@milesrout
Copy link

I think --strict should include --noImplicitAny.

👍 --noImplicitAny should be default, but obviously for backcompat reasons it can't be. But it should definitely be part of any '--strict' mode.

@skogsbaer
Copy link

Here is another motivating examples for disallowing function argument bivariance:

In our typescript code base, we have an interface for classes having an equals method

export interface Eq {
    equals(x: any): boolean;
}

Because of function argument bivariance, classes can implement this interface "incorrectly", that is by narrowing the argument type any. For example:

class C implements Eq {
    constructor(private c: number) {}
    // I would like to get an error here, stating that the method signature in 
    // the Eq interface does not match the implementation given here. 
    // In OO-languages such as C# or Java, you get this error.
    equals(that: C) {
        return this.getNum() === that.getNum();
    }
    getNum() {
        return this.c;
    }
}

class D {
    constructor(private d: number) {}
    equals(that: D) {
        return this.d === that.d;
    }
}

We now write a function that searches in an array of Eq-objects for an element:

function findInArray(arr: Array<Eq>, x: Eq): number {
    for (let i = 0; i < arr.length; i++) {
        if (arr[i].equals(x)) {
            return i;
        }
    }
    return -1;
}

Now it's rather easy to trigger an unexpected runtime error:

const arr = [new C(1), new C(2)];
const i = findInArray(arr, new D(2));
console.log(i);

The program now aborts with TypeError: undefined is not a function

@mhegazy
Copy link
Contributor

mhegazy commented Feb 20, 2016

We have moved to a more piecemeal approach to such checks, e.g. control flow checks, strict this, strict null, etc.. feels like this issue is obsolete. @RyanCavanaugh any objections to closing it?

@magnushiie
Copy link
Contributor

Another case where bivariant arguments really hurt is the React and Redux world, where mismatches in React component props are one of the most frequent source of errors.

Currently it's perfectly valid to have:

interface MyComponentProps {
  title: string;
}
export class MyComponent extends React.Component<MyComponentProps, void> {
  // code that needs this.props.title
}
const MyComponentAlias: React.ComponentClass<{}> = MyComponent;
// use of MyComponentAlias blows up, as it doesn't require title, but title is needed for MyComponent

The const in the above example is not a common use case, but I'm currently trying to make react-redux (especially the connect function) typings stricter, so props interface changes would highlight other changes that need to be made, but the bivariance only allows checking that the property field types are right, not that all properties are present where they need to be (and therefore also allows typos).

@DemiMarie
Copy link

I would like a flag under which TypeScript would reject any program that it could not prove to be free of type errors at runtime.

In addition to proving that a large class of bugs (type errors) are impossible, an AOT compiler (for servers) for "sound" typescript could erase the types at runtime for a perf gain.

@RyanCavanaugh
Copy link
Member Author

@DemiMarie what you're describing is not broadly possible given the constraints of our type system. As one example of many, we have no plausible way to prevent mutation after aliasing a structure with a less-specific type introducing an incorrect value into that structure, e.g. :

const a = [1, 2, 3];
const b: Array<string | number> = a;
b[0] = 'oops';
console.log(a[0] + 1); // 'oops1'

We've done a good job implementing almost everything in the OP so I'm going to close this due to a lack of usefulness of a meta-issue. Some good discussion happening at #9642 about how to get these stricter options on by default in places where it wouldn't be troublesome.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Needs More Info The issue still hasn't been fully clarified Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests