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

non-null assertion not working on IndexedAccess where K is a TypeParameter #21369

Closed
ajafff opened this issue Jan 23, 2018 · 3 comments
Closed
Labels
Breaking Change Would introduce errors in existing code

Comments

@ajafff
Copy link
Contributor

ajafff commented Jan 23, 2018

TypeScript Version: 2.7.0-dev.20180123

Search Terms: indexed access, non null assertion, type parameter

Code

// --strictNullChecks enabled

interface I {
    foo?: string;
    bar?: string;
}

declare function take<T>(p: T): void;

function fails<K extends keyof I>(o: I, k: K) {
    take<string | undefined>(o[k]); // works
    take<string>(o[k]!); // Argument of type 'I[K]' is not assignable to parameter of type 'string'. Type 'string | undefined' is not assignable to type 'string'. Type 'undefined' is not assignable to type 'string'.
}

// If the key is not generic, everything works as expected
function works<T extends I>(o: T, k: keyof I) {
    take<string>(o[k]!); // no error here, which should stay that way
}

Expected behavior:
No error as this worked very well in typescript@2.7.0-rc

Actual behavior:
Error as stated in the inline comment.

Playground Link:

Related Issues:
Another recent change to IndexedAccess inference: #21368

@mhegazy
Copy link
Contributor

mhegazy commented Feb 1, 2018

A general issue is that null assertion operator does not have any effect on generic type variables, that is type parameters, and indexed access types.. so t! is just the same as t. Adding support for null assertion on generic types is tracked by #14366.

With that in mind, previously, in TS 2.6 and before, the logic for getting a constraint on a indexed access type was if it has an index signature, then use that, otherwise it was any. In the example above, o[k] was constrained to any, then applying the null assertion operator had no effect, and thus was assignable to string in that case. That means that also take<number>(o[k]) would have been permissible.

In TS 2.7.1, the compiler is a bit smarter here, and will compute the constraint to be the union of all possible properties here, "foo" | "bar" and that makes o[k] constraint string | undefined, which is not a string..

@mhegazy mhegazy added Breaking Change Would introduce errors in existing code and removed Bug A bug in TypeScript labels Feb 1, 2018
@mhegazy mhegazy removed this from the TypeScript 2.7.2 milestone Feb 1, 2018
@zpdDG4gta8XKpMCd
Copy link

this is ridiculous, you made a special case at the price of breaking a fundamental concept, what is a top type that can fit o[k] now?

@SimonSchick
Copy link

I think this also affects me?

export class InjectionError extends Error {
  constructor(msg: string, public readonly dependency: string) {
    super(msg);
  }
}

export class DependencyNotFoundError extends InjectionError {
  constructor(dependency: string) {
    super(`${dependency} has not been provided`, dependency);
  }
}

/**
 * Indicates a circular depedency during injection.
 */
export class CircularInjectionError extends InjectionError {
  constructor(dependency: string, public readonly dependencyStack: string[]) {
    super(`Circular injection detected for ${dependency} ${dependencyStack.join('->')}`, dependency);
  }
}

/**
 * DI style dependency injector.
 */
export class Injector<T extends object> {
  private injectMap: {
    [K in keyof T]?: T[K];
  } = {};

  private provideMap: {
    [K in keyof T]?: () => T[K];
  } = {};

  private injectStack: string[] = [];

  /**
   * Attempts to load a dependency from the injector.
   * Will throw if the dependency is not present or a circular
   * dependency was detected.
   */
  public inject<K extends keyof T>(name: K): T[K] {
    if (this.injectStack.includes(name)) {
      const err = new CircularInjectionError(name, this.injectStack.slice());
      this.injectStack = [];
      throw err;
    }
    this.injectStack.push(name);
    const inj = this.injectMap[name];
    if (inj) {
      this.injectStack.pop();
      return inj; // < ~~~~~~~~~~~~~~~~~~~~ complains about potentially being undefied
    }
    const provider = this.provideMap[name];
    if (!provider) {
      this.injectStack = [];
      throw new DependencyNotFoundError(name);
    }
    const ret = this.injectMap[name] = provider();
    this.injectStack.pop();
    return ret;
  }

  /**
   * Adds a depedency for later injection.
   */
  public provide<K extends keyof T>(name: K, val: () => T[K]) {
    delete this.injectMap[name];
    this.provideMap[name] = val;
  }
}

@mhegazy mhegazy closed this as completed Mar 9, 2018
@microsoft microsoft locked and limited conversation to collaborators Jul 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Breaking Change Would introduce errors in existing code
Projects
None yet
Development

No branches or pull requests

6 participants