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

Exhaustiveness checking with classes #20512

Closed
mattroberts297 opened this issue Dec 6, 2017 · 3 comments
Closed

Exhaustiveness checking with classes #20512

mattroberts297 opened this issue Dec 6, 2017 · 3 comments
Labels
Question An issue which isn't directly actionable in code

Comments

@mattroberts297
Copy link

mattroberts297 commented Dec 6, 2017

I'm not sure if this is a bug or a suggestion as the documentation only talks about types (as opposed to classes) and I can understand not permitting discriminated unions, also known as tagged unions or algebraic data types that involve classes. I am coming from Scala which supports it, but each language is different and I guess you may be limited by Javascript. Here's some code:

TypeScript Version: 2.6.0

Code

Maybe.ts

export class Some<A> {
  readonly kind: "some";
  readonly value: A;

  constructor(value: A) {
    this.value = value;
  }

  then<B>(callback: (a: A) => B): Maybe<B> {
    return new Some<B>(callback(this.value)); // todo factory
  }
}

export class None<A> {
  readonly kind: "none";
  constructor() { }

  then<B>(callback: (a: A) => B): Maybe<B> {
    return new None<B>();
  }
}

export type Maybe<A> = Some<A> | None<A>;

export function print<A>(m: Maybe<A>) {
  switch (m.kind) {
    case "some": return `Some(${m.value})`;
    case "none": return "None";
  }
}

Maybe.spec.ts

import { print, Maybe, Some, None } from "./Maybe";
import "mocha";
import { expect } from "chai";

describe("maybe", () => {
  it("should permit exhaustive checking", () => {
    expect(print(new Some(10))).to.equal("Some(10)");
    expect(print(new None<number>())).to.equal("None");
  });
});

Expected behavior:

Test passes

  maybe
    ✓ should permit exhaustive checking

Actual behavior:

Test fails

  1) maybe should permit exhaustive checking:
     AssertionError: expected undefined to equal 'Some(10)'
      at Context.it (lib/Maybe.spec.js:8:63)

Work around code

Maybe.ts

export interface Some<A> {
  readonly kind: "some";
  readonly value: A;
}

export interface None {
  readonly kind: "none";
}

export type Maybe<A> = Some<A> | None;

export function print<A>(m: Maybe<A>) {
  switch (m.kind) {
    case "some": return `Some(${m.value})`;
    case "none": return "None";
  }
}

export function maybe<A>(a: A | null | undefined): Maybe<A> {
  if (a == null || a == undefined) {
    return { kind: "none" };
  } else {
    return { kind: "some", value: a};
  }
}

// todo then<A, B>(a: Maybe<A>, map: A => B): Maybe<B>

Maybe.spec.ts

import { print, Maybe, maybe } from "./Maybe";
import "mocha";
import { expect } from "chai";

describe("maybe", () => {
  it("should permit exhaustive checking", () => {
    expect(print(maybe(10))).to.equal("Some(10)");
    expect(print(maybe<number>(null))).to.equal("None");
  });
});

Thanks for the fun, manageable language.

@RyanCavanaugh RyanCavanaugh added the Question An issue which isn't directly actionable in code label Dec 6, 2017
@RyanCavanaugh
Copy link
Member

Here

  readonly kind: "none";

You need to write this

  readonly kind: "none" = "none";

The former only declares that there is a kind property with value "none" without actually initializing it to a value.

The minimal example to play with is

class A {
  kind: "a" /* = "a" */; 
}
var x = new A();
console.log(x.kind); // undefined, not "a"

@gcnew
Copy link
Contributor

gcnew commented Dec 6, 2017

This is a runtime error, not a compile-time one. The problem is that the kind field is never assigned to. You have provided a type for it, but not a value. This mistake is now caught by the latest compiler (typescript@next) if you turn --strictNullChecks and --strictPropertyInitialization on.

export class Some<A> {
  readonly kind: "some";
  readonly value: A;

  constructor(value: A) {
    this.value = value;
    this.kind = "some"; // fix in the constructor
  }

  ...
}

export class None<A> {
  readonly kind: "none" = "none"; // .. or inline

  ...
}

@mattroberts297
Copy link
Author

mattroberts297 commented Dec 7, 2017

Thank you @RyanCavanaugh and @gcnew. Really appreciate you taking the time to answer even though this turned out to be a question (not a bug or suggestion). I'm up and running now.

This mistake is now caught by the latest compiler (typescript@next) if you turn --strictNullChecks and --strictPropertyInitialization on.

Amazing that you're also guarding against developer incompetence in the compiler - thanks again!

@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Question An issue which isn't directly actionable in code
Projects
None yet
Development

No branches or pull requests

3 participants