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

Enum Equality #1748

Closed
basarat opened this issue Jan 21, 2015 · 10 comments
Closed

Enum Equality #1748

basarat opened this issue Jan 21, 2015 · 10 comments
Assignees
Labels
Committed The team has roadmapped this issue Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@basarat
Copy link
Contributor

basarat commented Jan 21, 2015

/// <reference path="./node_modules/typescript/bin/typescript.d.ts"/>
/// <reference path="./node_modules/typescript/bin/typescriptServices.d.ts"/>

import typescript = require('typescript');

var foo: typescript.CompilerOptions;
var bar: ts.CompilerOptions;

foo = bar; // Error: ModuleKind is not assignable to type ModuleKind

I expected the two CompilerOptions to be type compatible.

@RyanCavanaugh RyanCavanaugh added the By Design Deprecated - use "Working as Intended" or "Design Limitation" instead label Jan 21, 2015
@RyanCavanaugh
Copy link
Member

These enums are declared separately and are thus distinct in the type system. We don't use structural typing to determine enum compatibility.

@basarat
Copy link
Contributor Author

basarat commented Jan 21, 2015

👍

@dbaeumer
Copy link
Member

@billti @RyanCavanaugh I stumbled over this today as well and found it annoying. What is the reasons behind that this is by design.

@alexdima
Copy link
Member

👍 I think enums and classes should follow the same or as much as possible similar rules?

module A {
    export class C {
        bar: string;
    }
    export enum E {
        Prop = 0
    }
}
module B {
    export class C {
        bar: string
    }
    export enum E {
        Prop = 0
    }
}

var c1:A.C;
var c2:B.C;
c2 = c1; // <-- no type error here

var e1:A.E;
var e2:B.E;
e2 = e1; // <-- type error here

@mhegazy
Copy link
Contributor

mhegazy commented Nov 30, 2015

Enums are compared in a nominal fashion. similarly classes with private or protected members. so in the example above, if you add a private member the behavior will be identical to enums.

The rational was that enums represent disjoint subtypes of number, and that trying to assign one value from one enum to another (even if the enums were identical in structure) is probably an error that the user want to know about. e.g.:

const enum Direction {
   up = 0,
   down = 1
}

const enum Colors {
   red = 0,
   blue = 1
}

var color: Colors = Direction.up;  // valid if structurally compared, as both have the same value, but probably an error.

Having said that We have discussed this topic in the last design meeting, and i think we will be doping the nominal comparison for enums eventually.

@alexdima
Copy link
Member

alexdima commented Dec 1, 2015

@mhegazy

I am not sure I understand the definition you propose for structurally comparing enums where Colors would equal Directions. I believe an enum should be structurally equal if both name and values are equal. E.g. I would not expect that the following classes are structurally equal and TypeScript agrees and correctly flags this as an error:

module A {
    export class C {
        bar = "hello";
    }
}
module B {
    export class C {
        foo = "hello";
    }
}
var a:A.C;
var b:B.C;
a=b; // <-- I correctly get an error here

👍

Also please consider the following case (which I think @dbaeumer actually hit):

All node modules are authored in TypeScript and provide .d.ts files in npm such that the .d.ts file is discoverable with TypeScript's node module lookup rules.

// node module a
export enum Type { File = 0, Folder = 1 }
// node module b has a dependency on a
import {Type} from 'a';
export function compute(/*...*/): Type;
// node module c depends on both a and b
import {Type} from 'a';
import {compute} from 'b';

var foo = compute(/*...*/);
// cannot really use foo in a typed manner

The problem is that the fs has the following structure:

c
|--node_modules
   |--a
   |--b
      |--node_modules
         |--a

so TypeScript sees two different Type enums according to nominal rules even if a is used at the same version or even if a is used at different versions but keeps the exact same definition for Type.

@mhegazy
Copy link
Contributor

mhegazy commented Dec 1, 2015

this sounds like #4800. Are the modules the same? are they flattened? can they be flattened?

@alexdima
Copy link
Member

alexdima commented Dec 1, 2015

@mhegazy I actually hit this in another context (not with node modules) where I really just want enum structural typing.

See https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/api/browser/pluginHost.api.impl.ts#L112 and the many <any> casts.

The problem is the following:

  • we want to provide API to VSCode extensions
  • we specify our api in a vscode.d.ts (we must do this otherwise the automatic .d.ts generation would mean exporting all our type universe, it also helps to see the full api surface we have and to track it in one file)
  • the vscode.d.ts is self contained and can be consumed easily by extensions written in TypeScript
  • we then wish to express that we implement this api in pluginHost.api.impl.ts > PluginHostAPIImplementation

@mhegazy mhegazy added Suggestion An idea for TypeScript In Discussion Not yet reached consensus and removed By Design Deprecated - use "Working as Intended" or "Design Limitation" instead labels Dec 1, 2015
@mhegazy mhegazy reopened this Dec 1, 2015
@mhegazy
Copy link
Contributor

mhegazy commented Dec 7, 2015

As per the design meeting notes #5943, enums should compare in a semi-nominal manner. where the enum names have to match, and the target has members with the same names as the source.

@mhegazy mhegazy added Committed The team has roadmapped this issue and removed In Discussion Not yet reached consensus labels Dec 7, 2015
@mhegazy mhegazy added this to the TypeScript 1.8 milestone Dec 7, 2015
@alexdima
Copy link
Member

@mhegazy @sandersn Thank you!! ❤️

@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Jan 4, 2016
@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Committed The team has roadmapped this issue Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

6 participants