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

add compiler flag to enable granular inference outside var/let #17785

Closed

Conversation

KiaraGrouwstra
Copy link
Contributor

@KiaraGrouwstra KiaraGrouwstra commented Aug 14, 2017

This is a new compiler flag (idea: #16389 (comment)) to cancel type widening for objects / arrays (and even infer those as tuples) in const declarations as well as parameters:

// @widenTypes: false 
let a = [1, 2, 3];
// as before: number[]
a.push(4);
// ok
const b = [1, 2, 3];
// now [1, 2, 3]
b.push(4);
// boom
let c = { a: true };
// as before: { a: boolean }
const d = { a: true };
// now { a: true }

This would fix inference issues that spawned #3369, #6574, #7799, #8276, #9216, #9217, #10195, #12308, #12955, #15656, #16276, #16389, #16523, #16656, #17181. (Probably not exhaustive, just followed some links.)
Some of those propose alternate solutions like new keywords, though I presume this would address their issues as well.

Sometimes one may want granular types, sometimes one may want widened types. And in parameter positions, unfortunately only one of the two can be default, so either will end up wrong in some cases.
For declarations this holds as well, but the appeal of this flag there is that it yields users more control, based on whether they use the let or const keyword.

Notes:

  • Yes, it overloads the semantics of these JS keywords, if in a way that makes a surprising amount of sense.
  • it feels a bit unfortunate I had to propagate this boolean across 20 methods, should their params (new one and checkMode, or even the nodes too) be standardized into some options objects?

@dead-claudia
Copy link

Question: does this affect either of theses? If so, you'll need to add a special case to allow them, or you'll need an ungodly number of casts for things like options objects, etc.

interface SomeInterface { a: boolean }
declare function foo(arg: number[]): void
declare function bar(arg: SomeInterface): void

// As variable assignees
const a: number[] = [1, 2, 3];
const b: SomeInterface = {a: true};

// Same, but as arguments
foo([1, 2, 3]);
bar({a: true});

@KiaraGrouwstra
Copy link
Contributor Author

KiaraGrouwstra commented Aug 15, 2017

@isiahmeadows:

// @widenTypes: false 

interface SomeInterface { a: boolean }
declare function foo(arg: number[]): void
declare function bar(arg: SomeInterface): void

// As variable assignees
const a: number[] = [1, 2, 3]; // number[]
const b: SomeInterface = {a: true}; // SomeInterface

// Same, but as arguments
foo([1, 2, 3]); // ok, tuple assignable to array
bar({a: true}); // ok, assignable

const a: number[] = [1, 2, 3];

I thought of still typing a as the more granular inferred type as well, as for people it's convenient to annotate with a short-hand, while the compiler is better at inferring the real value. And while the granular version is at least as powerful, that might work. Tuples would lack the ability to meaningfully use mutating methods though, so it's kinda up in the air:

  • have the user-annotated number[] override trump, allowing control even within const.
  • figure out a way to ensure tuple types will be full-fledged array types (assignable / featuring their mutating methods).

I'm kinda neutral there, I presume things will gravitate toward option 1.

you'll need to add a special case to allow them, or you'll need an ungodly number of casts for things like options objects, etc.

Well, { a: true } is assignable to { a: boolean }.
Otherwise this idea would be nothing but terrible, yeah.

Edit: updated.

@KiaraGrouwstra
Copy link
Contributor Author

KiaraGrouwstra commented Aug 17, 2017

Okay, got parameters down too.

Now, this was dubbed a huge / massive breaking change, so... what's the damage?
I decided to try this on TS and all of its tests to find out:

  • obviously, since granular types become default for const and parameter positions, use let or explicit annotations/casts if you want mutability.
  • similarly, for functions like <T>(a: T, b: T) it becomes the users responsibility to ensure a widened type is passed as the first param or generic. If T becomes an array, then tuples should be widened in either position, though if Make tuples have known length #17765 brings array-tuple assignability that would be resolved.

That's it; couple dozen casts on a humongous code-base.

So the good news is, this doesn't even depend on #17765; it seems good now. I'm ready for feedback.

@pelotom
Copy link

pelotom commented Oct 19, 2017

This looks great! I am constantly annoyed by the fact that TypeScript doesn't always infer the most specific type. Is anyone taking a look at this PR?

@JohnNilsson
Copy link

I came here looking for a "noImplicitWidening" option to disable all, well, implicit type widening. Is this PR an implementation of that?

@KiaraGrouwstra
Copy link
Contributor Author

@JohnNilsson: pretty much.

The exception here is the right-hand side of let declarations, intentionally left to widen expressions such as to offer users some control.
This means let arr = []; can still be used with mutating methods, unlike const arr = [];.

(Explicit annotations like const arr: number[] = []; work too -- just more effort.)

@pelotom
Copy link

pelotom commented Oct 22, 2017

@JohnNilsson same, I just want a flag to disable type widening everywhere. If I ever need to widen a tuple to an array I’m fine with doing that via a manual cast.

@bcherny
Copy link

bcherny commented Apr 22, 2018

Any updates on this? Is this PR blocked on something, or can we make a yes/no call on whether this can make it in?

@mhegazy
Copy link
Contributor

mhegazy commented Apr 23, 2018

I do not think this is a direction we are willing to peruse at the time being.

First, we really do not want flags that change type system behavior since that effectively creates dialects of the language; the ones we have allowed in the past fall into one of two categories, 1. temporary patches for breaking changes behavior (e.g. --noStrictGenericChecks) and 2. stricter behaviors that we think users should move to, but cost of migration is high (e.g. --strictNullChecks). The common part here is there is an eventual state, we believe, where the patching flags are not uses and the strict options are turned on; and that is our recommendation for new projects, and what we advertise in tsc --init.
This feature falls in neither of these two categories, so the flag is not something we would want to take.

Second, overloading the meaning of const is not something we are crazy about. we have done something similar with function parameters, but do not think we want to sanction this meaning of const declarations.

Sorry for the delay.

@mhegazy mhegazy closed this Apr 23, 2018
@pelotom
Copy link

pelotom commented Apr 23, 2018

@mhegazy is the TypeScript team thinking about how to solve the usability issues around type widening? In my mind and I think a lot of others it is the biggest wart on the language currently.

@mhegazy
Copy link
Contributor

mhegazy commented Apr 23, 2018

is the TypeScript team thinking about how to solve the usability issues around type widening?

Yes, and we continue to think of ways to alleviate these pains. One thing to note is it took us a few iterations to get where we are now. and there are many scenarios on both sides of any change at this point that would break. we try to balance all these as we introduce changes.

@KiaraGrouwstra
Copy link
Contributor Author

Thanks for the response.

fwiw, I used the flag figuring this fell under # 2 -- stricter, (imo) preferable, some (imo minor) migration. I'd prefer not needing flags as well.

The semantic overloading of const I'll admit may be controversial.

@microsoft microsoft locked and limited conversation to collaborators Jul 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants