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

Should unions and non-primitives be allowed for relational comparisons? #5156

Closed
DanielRosenwasser opened this issue Oct 7, 2015 · 11 comments
Labels
Committed The team has roadmapped this issue Suggestion An idea for TypeScript

Comments

@DanielRosenwasser
Copy link
Member

To quote 4.19.1 of the spec:

4.19.3 The <, >, <=, >=, ==, !=, ===, and !== operators

These operators require one or both of the operand types to be assignable to the other. The result is always of the Boolean primitive type.

Any Boolean Number String Other
Any Boolean Boolean Boolean Boolean Boolean
Boolean Boolean Boolean
Number Boolean Boolean
String Boolean Boolean
Other Boolean Boolean

This has probably unintended consequences in the case of union types:

var x: string | number = "hello";
var y: number = 10;
var z: string | number = -1;

if (x < y && x <= z) {
}

The question is: is this still desirable behavior?

@ahejlsberg @RyanCavanaugh @danquirk @mhegazy @JsonFreeman

@DanielRosenwasser DanielRosenwasser added Question An issue which isn't directly actionable in code Spec Issues related to the TypeScript language specification Discussion Issues which may not have code impact labels Oct 7, 2015
@RyanCavanaugh RyanCavanaugh removed the Discussion Issues which may not have code impact label Oct 7, 2015
@LPGhatguy
Copy link
Contributor

Comparing strings and numbers is perfectly valid (if not a little wacky) JavaScript, isn't it?

@weswigham
Copy link
Member

I mean, comparing anything is "valid javascript" (objects, arrays...), if not obviously meaningful.

@DanielRosenwasser
Copy link
Member Author

From an offline discussion, the consensus seems to be that the equality operations could keep something like this behavior, whereas other comparison operators should have their own rules.

@mhegazy
Copy link
Contributor

mhegazy commented Feb 20, 2016

We should keep the idea of a new relationship on the backlog, i do not think we solved the underlying issue. closing this to avoid duplication though.

@DanielRosenwasser
Copy link
Member Author

This wasn't regarding the comparable relation, and even if it was, we've decided to use it for nullable types.

@DanielRosenwasser DanielRosenwasser removed the Question An issue which isn't directly actionable in code label Mar 9, 2016
@DanielRosenwasser DanielRosenwasser added Bug A bug in TypeScript and removed Spec Issues related to the TypeScript language specification labels Mar 25, 2016
@DanielRosenwasser
Copy link
Member Author

I think that this is a bug and that >, >=, <, and <=, should probably be separated out from this chart and treated independently.

@DanielRosenwasser DanielRosenwasser changed the title Should union types be comparable? Should unions and non-primitives be allowed for relational comparisons Mar 25, 2016
@DanielRosenwasser DanielRosenwasser changed the title Should unions and non-primitives be allowed for relational comparisons Should unions and non-primitives be allowed for relational comparisons? Mar 25, 2016
@jasonswearingen
Copy link

why are relational operators valid for non-primitives in typescript?

given that typescript is to make type checking pracical in javascript, I find it remarkable that things like the following do not give syntax errors:

var a={};
var b=2;
if(a>b){ } //i would expect a syntax error

@jasonswearingen
Copy link

further, why allow relational operations at all for non-primitives as they are useless and indicative of a coding or logic mistake?

var a={};
var b={};
if(a>b){ } //typescript should give a syntax error, but does not.

@jasonswearingen
Copy link

after reading the original bug, maybe what I am posting is slightly different (a regression?) so maybe this should be a separate issue.

edit: hmm, it seems like some object types can do relational operations correctly (such as Moment objects) so maybe I am wrong about this. would like to hear other opinions!

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript In Discussion Not yet reached consensus and removed Bug A bug in TypeScript labels Mar 28, 2016
@DanielRosenwasser
Copy link
Member Author

I have been looking into this again - one direction that I've taken has been that two variables of type string | number should not be comparable in a relational manner (i.e. using <, >=, etc.), since each value could be of the other type. I've also gone down the path that a type parameter is only relationally comparable to a type if its constraint is.

One question I ran into is whether this code should be allowd to be wrong:

export function compareValues<T>(a: T, b: T): Comparison {
    if (a === b) return Comparison.EqualTo;
    if (a === undefined) return Comparison.LessThan;
    if (b === undefined) return Comparison.GreaterThan;
    return a < b ? Comparison.LessThan : Comparison.GreaterThan;
}

Now, this function is "bad" in some sense - substituting T with anything other than number or string (and I guess boolean) will have questionable behavior. (Though keep in mind that the function has never been used badly!)

In any case, let's assume that we want to fix this. Let's start writing two overloads:

export function compareValues(a: number, b: number): Comparison;
export function compareValues(a: string, b: string): Comparison;

Looks decent so far. Now what about an implementation signature?

export function compareValues(a: ???, b: ???): Comparison {
    if (a === b) return Comparison.EqualTo;
    if (a === undefined) return Comparison.LessThan;
    if (b === undefined) return Comparison.GreaterThan;
    return a < b ? Comparison.LessThan : Comparison.GreaterThan;
}

What can you put in for ???? number | string won't work because then one can be a number and one can be a string. Really what I'd like to say is that if a has a union type, then b is of an appropriate branch of that type for the purposes of comparison.

It's not entirely clear what to do here, but I think "good enough" might be the place to land.

@RyanCavanaugh RyanCavanaugh removed the In Discussion Not yet reached consensus label Apr 24, 2017
@RyanCavanaugh
Copy link
Member

Seems we fixed this. Sometime things just work out!

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 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 Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

6 participants