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

Type checking element typed arrays #16389

Closed
bradzacher opened this issue Jun 9, 2017 · 27 comments
Closed

Type checking element typed arrays #16389

bradzacher opened this issue Jun 9, 2017 · 27 comments
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@bradzacher
Copy link
Contributor

TypeScript Version: 2.3.4

Code

function fn(arg : ('a' | 'b')[]) { }

// ONE
// type checked okay
fn(['a', 'b'])

// TWO
const x = ['a', 'b']
fn(x)
// throws type check error:
// Argument of type 'string[]' is not assignable to parameter of type '("a" | "b")[]'. Type 'string' is not assignable to type '"a" | "b"'.

Expected behavior:
calling case TWO should pass the type check fine

Actual behavior:
case TWO throws type check error:
Argument of type 'string[]' is not assignable to parameter of type '("a" | "b")[]'. Type 'string' is not assignable to type '"a" | "b"'.

typescript automatically casts the variable x to type string[], and in doing so loses all information about its contents, which is why it fails the strict value check.

this is a problem because it means you can't pass reusable arrays without strictly typing the original variable, which causes problems when using generics (a bit of a contrived example, but still):

function fn<T>() {
   return function<TField extends keyof T>(fields : TField[]) { }
}

const StructType = {
  a: 1,
  b: 2,
}
// have to manually define the same type as the inner generics, which kind of ruins encapsulation.
const x : (keyof typeof StructType)[] = ['a', 'b']

fn<typeof StructType>()(x)
// other stuff...
fn<typeof StructType>()(x)
@KiaraGrouwstra
Copy link
Contributor

Yeah, might be nicer if it'd just infer type ['a', 'b'] on assignment, deferring degeneration until needed (if at all).

@Arlen22
Copy link

Arlen22 commented Jun 14, 2017

But how on earth can you guarantee that nothing called x.push()? The compiler can't, but you can know that and are telling the compiler that. But it would be really nice if there was a separate syntax to declare a Tuple that would keep the typing as a tuple.

@mhegazy mhegazy added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Jun 14, 2017
@mhegazy
Copy link
Contributor

mhegazy commented Jun 14, 2017

literal types are only inferred, in mutable locations, if there is a contextual type. this is mainly a backward compatibility guarantee.

@KiaraGrouwstra
Copy link
Contributor

KiaraGrouwstra commented Jun 14, 2017

@mhegazy:

backward compatibility

Hm. If ['a', 'b'] < string[], how could switching it to the former break code? I must be missing something...

@Arlen22
Copy link

Arlen22 commented Jun 14, 2017

It is actually more strict, as you could be assigning ['x', 'y', 'z'] as string[] to something typed as ['a', 'b'].

@bradzacher
Copy link
Contributor Author

Yeah it would be nice if I could strictly define the array as a tuple so that the compiler can be sure I haven't pushed to it, etc.

It'd be good because then people could choose to use it as the way they default create arrays, same way generally you default to const for variable decls.

I.e. Have a tuple array construct which forces the compiler to treat the array as truly immutable (but still emit a normal js array).

@Arlen22
Copy link

Arlen22 commented Jun 14, 2017

Well you can do const myTup: [ string, string ] = ['a', 'b'], or even better const myTup: [ 'a','b' ] = ['a', 'b'], but I doubt you need that in a tuple. Actually, const myTup: [ 'a','b' ] will eventually become ('a'|'b')[], so I guess that still doesn't keep the ordering.

You could always spin your own definitely typed class, I suppose, though I don't know for sure how it would exactly work. Seems like it would be doable.

A tuple syntax or marker would be really nice so we don't have to declare the type and the value separately. Makes it really handy for chaining promises and observables.

@KiaraGrouwstra
Copy link
Contributor

KiaraGrouwstra commented Jun 15, 2017

Even if defaulting to granular types (tuples, string/number/boolean literals) would somehow break some edge cases, perhaps using a compiler flag like for strictNullChecks could prevent that?

@Arlen22
Copy link

Arlen22 commented Jun 15, 2017

It would break most code, because most of the time you don't think about it, but any literal that is not a constant is predicted to change in value, though not in type. An Array literal might not be as likely to, but it would probably still break a lot of code.

But adding new syntax wouldn't break anything.

@KiaraGrouwstra
Copy link
Contributor

KiaraGrouwstra commented Jun 15, 2017

Actually, if limited to const, is this really still a breaking change? I believe ++ is already disallowed there; not sure if array push is disallowed for const as well (on phone so can't check), but if so, I'm not sure that should be considered more feature than bug.

Edit:

Sounds like a candidate for a compiler flag. Libraries won't suffer much, as their types will get distributed as .d.ts, making them immune to the user's compiler flags.
In user land, I've taken it immutability has gained some traction. I'd say let people try if they like it and see how it goes.
That said, maybe there could be a middle ground where a granular mode would actually distinguish const and let.

@Arlen22
Copy link

Arlen22 commented Jun 15, 2017

No, array push is definitely allowed on const. This is due to the intent of const, where a variable's pointer is declared readonly, so you know you will never accidently assign a different value to (for instance) the NodeJS Path module and you will always be using the same object reference.

@Arlen22
Copy link

Arlen22 commented Jun 15, 2017

If you want a definite readonly or frozen object, it is not hard to spin your own. Some of the functionality is already included in browsers by default (Object.frozen).

@KiaraGrouwstra
Copy link
Contributor

Thanks.
In that case I'll concede a flag + const distinction would probably be more appropriate.

I won't argue against your own use case of explicit per-case opt-in; I think both sound legit.
That said, this may go beyond marking just tuples.

If you'd wanna use Ramda's path function, for one, the type of the path you're passing must be preserved as a tuple of string/number literals for inference to work.
I imagine having to explicitly mark the array and every single element there would feel cumbersome.
If that's solved by checking a compiler flag at the cost of push on const arrays, I could see users in the immutability camp considering that a small price to pay for the considerably more powerful type inference it could potentially enable.

@bradzacher
Copy link
Contributor Author

I'm not sure a blanket flag would be the best idea, as it would mean you would be severely break from standard JS syntax.

Maybe a new keyword (which compiles to const) would be a good alternative. Eg
pure arr = ['a', 'b'] would mean that the only options available are reads and pure function calls?

You could then also apply this keyword to methods to signify that a function is pure. Which is how the compiler could tell what functions on an object are safe to call for a pure declared variable.

This keyword could be enforced by the compiler, meaning that a function marked with the pure keyword cannot have a body which:
(a) writes to properties on this,
(b) writes to properties on input arguments,
(c) calls functions not marked as pure.

Additionally the compiler could automatically mark methods as pure if they don't violate any of the above rules.

@KiaraGrouwstra
Copy link
Contributor

@bradzacher: sounds cool to me, but I fear detecting function purity is far off... callbacks, Promises and dynamic imports probably aren't helping.

@KiaraGrouwstra
Copy link
Contributor

I hadn't initially understood this from mhegazy's response, but it appears string literals already discriminate between mutable locations (e.g. let) vs. other places (literals, const):

const foo = 'foo'; // 'foo'
let bar = 'bar'; // string

So it appears that arrays (and objects) are still handled differently in that regard.
Considering this kind of branching already exists in the current implementation, I suppose this would make a flag to extend this behavior to others (array, object) pretty viable.

@Arlen22
Copy link

Arlen22 commented Jun 26, 2017

Considering this kind of branching already exists in the current implementation, I suppose this would make a flag to extend this behavior to others (array, object) pretty viable.

Yes, but a global flag would never work, because there are use cases where an array is declared with an element in it and then added to. Since this is perfectly valid Javascript, I can hardly see this flag working. An array is an array, and if we want to declare a Tuple we will have to use something different at that line of code. Something like this works (almost).

function newTuple<T0>(item0: T0): [T0];
function newTuple<T0, T1>(item0: T0, item1: T1): [T0, T1];
function newTuple<T0, T1, T2>(item0: T0, item1: T1, item2: T2): [T0, T1, T2];
function newTuple<T0, T1, T2, T3>(item0: T0, item1: T1, item2: T2, tem3: T3): [T0, T1, T2, T3];
function newTuple<T0, T1, T2, T3, T4>(item0: T0, item1: T1, item2: T2, tem3: T3, item4: T4): [T0, T1, T2, T3, T4];
function newTuple<T0, T1, T2, T3, T4, T5>(item0: T0, item1: T1, item2: T2, tem3: T3, item4: T4, item5: T5): [T0, T1, T2, T3, T4, T5];
function newTuple<T0, T1, T2, T3, T4, T5, T6>(item0: T0, item1: T1, item2: T2, tem3: T3, item4: T4, item5: T5, item6: T6): [T0, T1, T2, T3, T4, T5, T6];
function newTuple<T0, T1, T2, T3, T4, T5, T6, T7>(item0: T0, item1: T1, item2: T2, tem3: T3, item4: T4, item5: T5, item6: T6, item7: T7): [T0, T1, T2, T3, T4, T5, T6, T7];
function newTuple<T0, T1, T2, T3, T4, T5, T6, T7, T8>(item0: T0, item1: T1, item2: T2, tem3: T3, item4: T4, item5: T5, item6: T6, item7: T7, item8: T8): [T0, T1, T2, T3, T4, T5, T6, T7, T8];
function newTuple<T0, T1, T2, T3, T4, T5, T6, T7, T8, T9>(item0: T0, item1: T1, item2: T2, tem3: T3, item4: T4, item5: T5, item6: T6, item7: T7, item8: T8, item9: T9): [T0, T1, T2, T3, T4, T5, T6, T7, T8, T9];
function newTuple<T0, T1, T2, T3, T4, T5, T6, T7, T8, T9, TR>(item0: T0, item1: T1, item2: T2, tem3: T3, item4: T4, item5: T5, item6: T6, item7: T7, item8: T8, item9: T9, rest: TR): [T0, T1, T2, T3, T4, T5, T6, T7, T8, T9] & TR;
function newTuple(...args: any[]): any {
    return args;
}

The only thing that doesn't work is & doesn't concat two tuples.

@Arlen22
Copy link

Arlen22 commented Jun 26, 2017

Here is a small update that allows chaining the value, but shows the problem with chaining the type.

function newTuple<T0>(item0: T0): [T0];
function newTuple<T0, T1>(item0: T0, item1: T1): [T0, T1];
function newTuple<T0, T1, T2>(item0: T0, item1: T1, item2: T2): [T0, T1, T2];
function newTuple<T0, T1, T2, T3>(item0: T0, item1: T1, item2: T2, tem3: T3): [T0, T1, T2, T3];
function newTuple<T0, T1, T2, T3, T4>(item0: T0, item1: T1, item2: T2, tem3: T3, item4: T4): [T0, T1, T2, T3, T4];
function newTuple<T0, T1, T2, T3, T4, T5>(item0: T0, item1: T1, item2: T2, tem3: T3, item4: T4, item5: T5): [T0, T1, T2, T3, T4, T5];
function newTuple<T0, T1, T2, T3, T4, T5, T6>(item0: T0, item1: T1, item2: T2, tem3: T3, item4: T4, item5: T5, item6: T6): [T0, T1, T2, T3, T4, T5, T6];
function newTuple<T0, T1, T2, T3, T4, T5, T6, T7>(item0: T0, item1: T1, item2: T2, tem3: T3, item4: T4, item5: T5, item6: T6, item7: T7): [T0, T1, T2, T3, T4, T5, T6, T7];
function newTuple<T0, T1, T2, T3, T4, T5, T6, T7, T8>(item0: T0, item1: T1, item2: T2, tem3: T3, item4: T4, item5: T5, item6: T6, item7: T7, item8: T8): [T0, T1, T2, T3, T4, T5, T6, T7, T8];
function newTuple<T0, T1, T2, T3, T4, T5, T6, T7, T8, T9>(item0: T0, item1: T1, item2: T2, tem3: T3, item4: T4, item5: T5, item6: T6, item7: T7, item8: T8, item9: T9): [T0, T1, T2, T3, T4, T5, T6, T7, T8, T9];
function newTuple<T0, T1, T2, T3, T4, T5, T6, T7, T8, T9, TR>(item0: T0, item1: T1, item2: T2, tem3: T3, item4: T4, item5: T5, item6: T6, item7: T7, item8: T8, item9: T9, restTuple: TR): [T0, T1, T2, T3, T4, T5, T6, T7, T8, T9] & TR;
function newTuple(...args: any[]): any {
    //it will never be more than one deep because function arguments are resolved
    //before the function call
    if (args.length === 11) {
        return args.concat(args.pop());
    } else {
        return args;
    }
}
const test = newTuple("", "", "", "", "", "", "", "", "", "", newTuple("", 0, "", "test" as "test", "test"));

var t0 = test[0]; // type: string
var t1 = test[1]; // type: string & number
var t2 = test[2]; // type: string
var t3 = test[3]; // type: string & "test"
var t4 = test[4]; // type: string
var t5 = test[5]; // type: string
var t6 = test[6]; // type: string
var t7 = test[7]; // type: string
var t8 = test[8]; // type: string
var t9 = test[9]; // type: string
var t10 = test[10]; // type: string | (string & number)
var t11 = test[11]; // type: string | (string & number)
var t12 = test[12]; // type: string | (string & number)
var t13 = test[13]; // type: string | (string & number)
var t14 = test[14]; // type: string | (string & number)
var t15 = test[15]; // type: string | (string & number)
var t16 = test[16]; // type: string | (string & number)
var t17 = test[17]; // type: string | (string & number)
// repeats t17 on up.

@Arlen22
Copy link

Arlen22 commented Jun 26, 2017

Or using a constructor interface

interface TupleConstructor {
    new <T0>(item0: T0)
        : [T0];
    new <T0, T1>(item0: T0, item1: T1)
        : [T0, T1];
    new <T0, T1, T2>(item0: T0, item1: T1, item2: T2)
        : [T0, T1, T2];
    new <T0, T1, T2, T3>(item0: T0, item1: T1, item2: T2, tem3: T3)
        : [T0, T1, T2, T3];
    new <T0, T1, T2, T3, T4>(item0: T0, item1: T1, item2: T2, tem3: T3, item4: T4)
        : [T0, T1, T2, T3, T4];
    new <T0, T1, T2, T3, T4, T5>(item0: T0, item1: T1, item2: T2, tem3: T3, item4: T4, item5: T5)
        : [T0, T1, T2, T3, T4, T5];
    new <T0, T1, T2, T3, T4, T5, T6>(
        item0: T0, item1: T1, item2: T2, tem3: T3, item4: T4, item5: T5, item6: T6)
        : [T0, T1, T2, T3, T4, T5, T6];
    new <T0, T1, T2, T3, T4, T5, T6, T7>(
        item0: T0, item1: T1, item2: T2, tem3: T3, item4: T4, item5: T5, item6: T6, item7: T7)
        : [T0, T1, T2, T3, T4, T5, T6, T7];
    new <T0, T1, T2, T3, T4, T5, T6, T7, T8>(
        item0: T0, item1: T1, item2: T2, tem3: T3, item4: T4, item5: T5, item6: T6, item7: T7, item8: T8)
        : [T0, T1, T2, T3, T4, T5, T6, T7, T8];
    new <T0, T1, T2, T3, T4, T5, T6, T7, T8, T9>(
        item0: T0, item1: T1, item2: T2, tem3: T3, item4: T4, item5: T5, item6: T6, item7: T7, item8: T8, item9: T9)
        : [T0, T1, T2, T3, T4, T5, T6, T7, T8, T9];
    new <T0, T1, T2, T3, T4, T5, T6, T7, T8, T9, TR extends any[]>(
        item0: T0, item1: T1, item2: T2, tem3: T3, item4: T4, item5: T5, item6: T6, item7: T7, item8: T8, item9: T9, restTuple: TR
    ): [T0, T1, T2, T3, T4, T5, T6, T7, T8, T9] & TR;
}

const Tuple: TupleConstructor = (((...args: any[]) => {
    //it will never be more than one deep because function arguments are resolved
    //before the function call
    if (args.length === 11) {
        return args.concat(args.pop());
    } else {
        return args;
    }
}) as any) as TupleConstructor;

@Arlen22
Copy link

Arlen22 commented Jun 26, 2017

I only have 4 years of experience in JavaScript, and 6 months in Typescript, so if more experienced users want to weigh in on this, please do.

@KiaraGrouwstra
Copy link
Contributor

KiaraGrouwstra commented Jun 27, 2017

@Arlen22: experience-wise that sounds fine to me; JS only got good from ES6 anyway. :D

If you have a nice solution for the object case, it might yield you a bounty on SO.

That said, your approach doesn't appear recursive -- the ideal inferred contents, for both heterogeneous arrays (= tuples) and objects would still be granular (e.g. string literals), or it'd fail for the purpose of e.g. properly inferring navigation. It should stay as granular as possible until forced out of it by expected mutation.

Yes, but a global flag would never work, because there are use cases where an array is declared with an element in it and then added to. Since this is perfectly valid Javascript, I can hardly see this flag working.

I'm not saying there's be no drawback, otherwise it shouldn't need a flag. The limitation is pretty easy to deal with though: if you must use mutation on a variable, use let over const -- it's guaranteed to work just as well for those cases (only the type will be less granular to support mutation). If you'd rather not use this behavior though, it'd be opt-in anyway.

Alternatively, the behavior could be relegated to a new keyword, in which case a flag would no longer be needed. But those interested in granular inference seem more likely to favor a functional approach over mutation (precise inference matters most there), and if they had to, presumably may not mind a let over further JS-incompatible syntax pollution.
In the event granular inference turns out hugely popular and within these users there'd still turn out to be a huge demand for that particular combination though, we could still look into the options.

@KiaraGrouwstra
Copy link
Contributor

KiaraGrouwstra commented Jul 1, 2017

@Arlenn22: yeah, looks like methods like push (-> generic in LHS/co-variant position) would fail if stored types were concrete; I get that now.
Having experimented a bit now, I'm a bit confused though:

  • primitives like number: normally protected from values mismatching type by disabling ++ on const. (this protection fails if you specifically assign the literal type, saving as let, then call ++ though.)
  • const arrays are protected from pop / push type mismatches by having tuple types degenerate to arrays and specific contents degenerate from literals to more general types as well. (again, the type will not stay correct if you manually specify the type as tuple then mutate.)
  • objects: here the keys stay intact, though even using const their values aren't retained as literals. if I got it right, this is to ensure the type will stay somewhat accurate even after mutation. interestingly though, in this case you don't need to manually specify a type for the type to be 'unsafe': use any mutation (Object.assign / manually add a property) and the static type can no longer match for both before and after.

Obviously this is super hard with static type systems while at run-time things can change all over the place, but yeah. At least for the case of objects, I'm not so sure the degeneration is aiding for backward compatibility concerns like in the push case. That said, good catch for immediately realizing what'd fail there.

Edit: if the following is considered legitimate, then we may have a solution to the tuple + push errors:

const x = [90210];
x.push('Beverly Hills');

@Arlen22
Copy link

Arlen22 commented Jul 2, 2017

The way Javascript works is that you have 4 primitives: string, bool, number, and reference (am I missing one). A reference refers to any object (whether an Array or Hashmap). A variable gets assigned a primitive (whether let, const, or var), but for const, that primitive may never change. So it was from that perspective that I saw the problem with using const :)

@Arlen22
Copy link

Arlen22 commented Jul 2, 2017

use any mutation (Object.assign / manually add a property) and the static type can no longer match for both before and after.

You can use & to merge to object types. { hello: string } & { world: string } results in the intellisense showing both properties.

However, the new type must be assigned to a new variable. The type of a variable cannot be changed once it is assigned. For this reason, I use interfaces to specify the eventual properties of a variable. Depending on how I use it, I may or may not make it optional. If I am generating an object and returning it, I usually will not make it optional.

interface IMy {
    hello: string;
    world: string;
}
const me: IMy = {} as any; //as any will prevent an error 
IMy.hello = "test";

@KiaraGrouwstra
Copy link
Contributor

KiaraGrouwstra commented Jul 2, 2017

I'll admit it's a tough problem. Rather than the flag, I wonder if an alternative solution might be to add an overload for tuple interfaces for e.g. push so as to allow pushing any element. That way, perhaps any breaking changes could be prevented.

Edit: I further worked this out into concrete tuple overloads at #16656 (comment).

@KiaraGrouwstra
Copy link
Contributor

I think my proposed changes here might also help address issues raised in #212 (comment) w.r.t. typing Function.prototype methods.

@mhegazy
Copy link
Contributor

mhegazy commented Aug 24, 2017

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

4 participants