Skip to content

Introduce boolean literal freshness #27042

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

Merged
merged 1 commit into from
Sep 14, 2018
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 49 additions & 31 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
@@ -296,8 +296,8 @@ namespace ts {
createPromiseType,
createArrayType,
getBooleanType: () => booleanType,
getFalseType: () => falseType,
getTrueType: () => trueType,
getFalseType: (fresh?) => fresh ? falseType : regularFalseType,
getTrueType: (fresh?) => fresh ? trueType : regularTrueType,
getVoidType: () => voidType,
getUndefinedType: () => undefinedType,
getNullType: () => nullType,
@@ -405,9 +405,22 @@ namespace ts {
const nullWideningType = strictNullChecks ? nullType : createIntrinsicType(TypeFlags.Null | TypeFlags.ContainsWideningType, "null");
const stringType = createIntrinsicType(TypeFlags.String, "string");
const numberType = createIntrinsicType(TypeFlags.Number, "number");
const falseType = createIntrinsicType(TypeFlags.BooleanLiteral, "false");
const trueType = createIntrinsicType(TypeFlags.BooleanLiteral, "true");
const booleanType = createBooleanType([falseType, trueType]);
const falseType = createIntrinsicType(TypeFlags.BooleanLiteral, "false") as FreshableIntrinsicType;
const regularFalseType = createIntrinsicType(TypeFlags.BooleanLiteral, "false") as FreshableIntrinsicType;
const trueType = createIntrinsicType(TypeFlags.BooleanLiteral, "true") as FreshableIntrinsicType;
const regularTrueType = createIntrinsicType(TypeFlags.BooleanLiteral, "true") as FreshableIntrinsicType;
falseType.flags |= TypeFlags.FreshLiteral;
trueType.flags |= TypeFlags.FreshLiteral;
trueType.regularType = regularTrueType;
regularTrueType.freshType = trueType;
falseType.regularType = regularFalseType;
regularFalseType.freshType = falseType;
const booleanType = createBooleanType([regularFalseType, regularTrueType]);
// Also mark all combinations of fresh/regular booleans as "Boolean" so they print as `boolean` instead of `true | false`
// (The union is cached, so simply doing the marking here is sufficient)
createBooleanType([regularFalseType, trueType]);
createBooleanType([falseType, regularTrueType]);
createBooleanType([falseType, trueType]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we ever get a union of falseType | regularFalseType? Note that it doesn't matter how we print that, I'm just curious whether it can happen.

Copy link
Member Author

@weswigham weswigham Sep 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uhmmm, I don't think so, just like how we can't have "word" | "word"(fresh edition), since both have the same type "identity". Specifically, removeRedundantLiteralTypes handles removing redundant fresh types when the regular one is also present.

const esSymbolType = createIntrinsicType(TypeFlags.ESSymbol, "symbol");
const voidType = createIntrinsicType(TypeFlags.Void, "void");
const neverType = createIntrinsicType(TypeFlags.Never, "never");
@@ -4170,7 +4183,7 @@ namespace ts {
const baseType = t.flags & TypeFlags.BooleanLiteral ? booleanType : getBaseTypeOfEnumLiteralType(<LiteralType>t);
if (baseType.flags & TypeFlags.Union) {
const count = (<UnionType>baseType).types.length;
if (i + count <= types.length && types[i + count - 1] === (<UnionType>baseType).types[count - 1]) {
if (i + count <= types.length && getRegularTypeOfLiteralType(types[i + count - 1]) === getRegularTypeOfLiteralType((<UnionType>baseType).types[count - 1])) {
result.push(baseType);
i += count - 1;
continue;
@@ -6157,7 +6170,7 @@ namespace ts {
if (type.flags & TypeFlags.UniqueESSymbol) {
return `__@${type.symbol.escapedName}@${getSymbolId(type.symbol)}` as __String;
}
if (type.flags & TypeFlags.StringOrNumberLiteral) {
if (type.flags & (TypeFlags.StringLiteral | TypeFlags.NumberLiteral)) {
return escapeLeadingUnderscores("" + (<LiteralType>type).value);
}
return Debug.fail();
@@ -8810,7 +8823,7 @@ namespace ts {
t.flags & TypeFlags.StringLiteral && includes & TypeFlags.String ||
t.flags & TypeFlags.NumberLiteral && includes & TypeFlags.Number ||
t.flags & TypeFlags.UniqueESSymbol && includes & TypeFlags.ESSymbol ||
t.flags & TypeFlags.StringOrNumberLiteral && t.flags & TypeFlags.FreshLiteral && containsType(types, (<LiteralType>t).regularType);
t.flags & TypeFlags.Literal && t.flags & TypeFlags.FreshLiteral && containsType(types, (<LiteralType>t).regularType);
if (remove) {
orderedRemoveItemAt(types, i);
}
@@ -9810,8 +9823,8 @@ namespace ts {
}

function getFreshTypeOfLiteralType(type: Type): Type {
if (type.flags & TypeFlags.StringOrNumberLiteral && !(type.flags & TypeFlags.FreshLiteral)) {
if (!(<LiteralType>type).freshType) {
if (type.flags & TypeFlags.Literal && !(type.flags & TypeFlags.FreshLiteral)) {
if (!(<LiteralType>type).freshType) { // NOTE: Safe because all freshable intrinsics always have fresh types already
const freshType = createLiteralType(type.flags | TypeFlags.FreshLiteral, (<LiteralType>type).value, (<LiteralType>type).symbol);
freshType.regularType = <LiteralType>type;
(<LiteralType>type).freshType = freshType;
@@ -9822,7 +9835,7 @@ namespace ts {
}

function getRegularTypeOfLiteralType(type: Type): Type {
return type.flags & TypeFlags.StringOrNumberLiteral && type.flags & TypeFlags.FreshLiteral ? (<LiteralType>type).regularType :
return type.flags & TypeFlags.Literal && type.flags & TypeFlags.FreshLiteral ? (<LiteralType>type).regularType :
type.flags & TypeFlags.Union ? getUnionType(sameMap((<UnionType>type).types, getRegularTypeOfLiteralType)) :
type;
}
@@ -11035,11 +11048,11 @@ namespace ts {
}

function isTypeRelatedTo(source: Type, target: Type, relation: Map<RelationComparisonResult>) {
if (source.flags & TypeFlags.StringOrNumberLiteral && source.flags & TypeFlags.FreshLiteral) {
source = (<LiteralType>source).regularType;
if (source.flags & TypeFlags.Literal && source.flags & TypeFlags.FreshLiteral) {
source = (<FreshableType>source).regularType;
}
if (target.flags & TypeFlags.StringOrNumberLiteral && target.flags & TypeFlags.FreshLiteral) {
target = (<LiteralType>target).regularType;
if (target.flags & TypeFlags.Literal && target.flags & TypeFlags.FreshLiteral) {
target = (<FreshableType>target).regularType;
}
if (source === target ||
relation === comparableRelation && !(target.flags & TypeFlags.Never) && isSimpleTypeRelatedTo(target, source, relation) ||
@@ -11194,11 +11207,11 @@ namespace ts {
* * Ternary.False if they are not related.
*/
function isRelatedTo(source: Type, target: Type, reportErrors = false, headMessage?: DiagnosticMessage): Ternary {
if (source.flags & TypeFlags.StringOrNumberLiteral && source.flags & TypeFlags.FreshLiteral) {
source = (<LiteralType>source).regularType;
if (source.flags & TypeFlags.Literal && source.flags & TypeFlags.FreshLiteral) {
source = (<FreshableType>source).regularType;
}
if (target.flags & TypeFlags.StringOrNumberLiteral && target.flags & TypeFlags.FreshLiteral) {
target = (<LiteralType>target).regularType;
if (target.flags & TypeFlags.Literal && target.flags & TypeFlags.FreshLiteral) {
target = (<FreshableType>target).regularType;
}
if (source.flags & TypeFlags.Substitution) {
source = relation === definitelyAssignableRelation ? (<SubstitutionType>source).typeVariable : (<SubstitutionType>source).substitute;
@@ -12766,7 +12779,7 @@ namespace ts {
return type.flags & TypeFlags.EnumLiteral && type.flags & TypeFlags.FreshLiteral ? getBaseTypeOfEnumLiteralType(<LiteralType>type) :
type.flags & TypeFlags.StringLiteral && type.flags & TypeFlags.FreshLiteral ? stringType :
type.flags & TypeFlags.NumberLiteral && type.flags & TypeFlags.FreshLiteral ? numberType :
type.flags & TypeFlags.BooleanLiteral ? booleanType :
type.flags & TypeFlags.BooleanLiteral && type.flags & TypeFlags.FreshLiteral ? booleanType :
type.flags & TypeFlags.Union ? getUnionType(sameMap((<UnionType>type).types, getWidenedLiteralType)) :
type;
}
@@ -12820,7 +12833,7 @@ namespace ts {
return type.flags & TypeFlags.Union ? getFalsyFlagsOfTypes((<UnionType>type).types) :
type.flags & TypeFlags.StringLiteral ? (<LiteralType>type).value === "" ? TypeFlags.StringLiteral : 0 :
type.flags & TypeFlags.NumberLiteral ? (<LiteralType>type).value === 0 ? TypeFlags.NumberLiteral : 0 :
type.flags & TypeFlags.BooleanLiteral ? type === falseType ? TypeFlags.BooleanLiteral : 0 :
type.flags & TypeFlags.BooleanLiteral ? (type === falseType || type === regularFalseType) ? TypeFlags.BooleanLiteral : 0 :
type.flags & TypeFlags.PossiblyFalsy;
}

@@ -12837,7 +12850,8 @@ namespace ts {
function getDefinitelyFalsyPartOfType(type: Type): Type {
return type.flags & TypeFlags.String ? emptyStringType :
type.flags & TypeFlags.Number ? zeroType :
type.flags & TypeFlags.Boolean || type === falseType ? falseType :
type.flags & TypeFlags.Boolean || type === regularFalseType ? regularFalseType :
type === falseType ? falseType :
type.flags & (TypeFlags.Void | TypeFlags.Undefined | TypeFlags.Null) ||
type.flags & TypeFlags.StringLiteral && (<LiteralType>type).value === "" ||
type.flags & TypeFlags.NumberLiteral && (<LiteralType>type).value === 0 ? type :
@@ -14092,7 +14106,10 @@ namespace ts {
if (assignedType.flags & TypeFlags.Never) {
return assignedType;
}
const reducedType = filterType(declaredType, t => typeMaybeAssignableTo(assignedType, t));
let reducedType = filterType(declaredType, t => typeMaybeAssignableTo(assignedType, t));
if (assignedType.flags & (TypeFlags.FreshLiteral | TypeFlags.Literal)) {
reducedType = mapType(reducedType, getFreshTypeOfLiteralType); // Ensure that if the assignment is a fresh type, that we narrow to fresh types
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why didn't we have to do this before, for string and number literal types?

Copy link
Member Author

@weswigham weswigham Sep 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably should have been but as it turns out people narrow boolean made via !!expr || !expr a ton more than narrowing a string literal union ("" | "b") made via x === "" || x === "b", so it's a lot more obvious an issue with booleans.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And as I said in person, this is going to break one RWC project along the lines of this:

type RuleSeverity = "warning" | "error" | "off";
interface IOptions {
    ruleSeverity: RuleSeverity;
}

function parseRuleOptions(): Partial<IOptions> {
    let defaultRuleSeverity: RuleSeverity = "error";
    let ruleSeverity = defaultRuleSeverity;

    return {
        ruleSeverity,
    };
}

previously this worked because defaultRuleSeverity was narrowed to the nonfresh "error" when it was used to initialize ruleSeverity, whereas now we choose the fresh "error" which widens (and triggers an error in the return type below). So this is a (probably small) break.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this change appears to be the cause of #27259. I'm really not sure I like this. The intuitive rule was always that once you add a type annotation to let, const or var declaration, values read from that variable are not fresh. But now they are unless the annotated type is a unit type.

const x1: 'x' = 'x';
let z1 = x1;  // 'x'

const x2: 'x' | 'y' = 'x';
let z2 = x2;  // Was 'x', now string

I find it hard to construct a rationale for that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW the individual unit type only behaves differently because it skips the narrowing codepath. Had I realized as much I probably would've altered the unit type behavior, too. In any case, removing this bit if we don't like it is easy; but it's going to make boolean literals a tad more breaky.

}
// Our crude heuristic produces an invalid result in some cases: see GH#26130.
// For now, when that happens, we give up and don't narrow at all. (This also
// means we'll never narrow for erroneous assignments where the assigned type
@@ -14145,8 +14162,8 @@ namespace ts {
}
if (flags & TypeFlags.BooleanLike) {
return strictNullChecks ?
type === falseType ? TypeFacts.FalseStrictFacts : TypeFacts.TrueStrictFacts :
type === falseType ? TypeFacts.FalseFacts : TypeFacts.TrueFacts;
(type === falseType || type === regularFalseType) ? TypeFacts.FalseStrictFacts : TypeFacts.TrueStrictFacts :
(type === falseType || type === regularFalseType) ? TypeFacts.FalseFacts : TypeFacts.TrueFacts;
}
if (flags & TypeFlags.Object) {
return isFunctionObjectType(<ObjectType>type) ?
@@ -28351,19 +28368,20 @@ namespace ts {
function isLiteralConstDeclaration(node: VariableDeclaration | PropertyDeclaration | PropertySignature | ParameterDeclaration): boolean {
if (isDeclarationReadonly(node) || isVariableDeclaration(node) && isVarConst(node)) {
const type = getTypeOfSymbol(getSymbolOfNode(node));
return !!(type.flags & TypeFlags.StringOrNumberLiteral && type.flags & TypeFlags.FreshLiteral);
return !!(type.flags & TypeFlags.Literal && type.flags & TypeFlags.FreshLiteral);
}
return false;
}

function literalTypeToNode(type: LiteralType, enclosing: Node): Expression {
const enumResult = type.flags & TypeFlags.EnumLiteral && nodeBuilder.symbolToExpression(type.symbol, SymbolFlags.Value, enclosing);
return enumResult || createLiteral(type.value);
function literalTypeToNode(type: FreshableType, enclosing: Node): Expression {
const enumResult = type.flags & TypeFlags.EnumLiteral ? nodeBuilder.symbolToExpression(type.symbol, SymbolFlags.Value, enclosing)
: type === trueType ? createTrue() : type === falseType && createFalse();
return enumResult || createLiteral((type as LiteralType).value);
}

function createLiteralConstValue(node: VariableDeclaration | PropertyDeclaration | PropertySignature | ParameterDeclaration) {
const type = getTypeOfSymbol(getSymbolOfNode(node));
return literalTypeToNode(<LiteralType>type, node);
return literalTypeToNode(<FreshableType>type, node);
}

function createResolver(): EmitResolver {
@@ -29735,7 +29753,7 @@ namespace ts {

function checkAmbientInitializer(node: VariableDeclaration | PropertyDeclaration | PropertySignature) {
if (node.initializer) {
const isInvalidInitializer = !(isStringOrNumberLiteralExpression(node.initializer) || isSimpleLiteralEnumReference(node.initializer));
const isInvalidInitializer = !(isStringOrNumberLiteralExpression(node.initializer) || isSimpleLiteralEnumReference(node.initializer) || node.initializer.kind === SyntaxKind.TrueKeyword || node.initializer.kind === SyntaxKind.FalseKeyword);
const isConstOrReadonly = isDeclarationReadonly(node) || isVariableDeclaration(node) && isVarConst(node);
if (isConstOrReadonly && !node.type) {
if (isInvalidInitializer) {
15 changes: 12 additions & 3 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
@@ -3034,8 +3034,8 @@ namespace ts {
/* @internal */ getStringType(): Type;
/* @internal */ getNumberType(): Type;
/* @internal */ getBooleanType(): Type;
/* @internal */ getFalseType(): Type;
/* @internal */ getTrueType(): Type;
/* @internal */ getFalseType(fresh?: boolean): Type;
/* @internal */ getTrueType(fresh?: boolean): Type;
/* @internal */ getVoidType(): Type;
/* @internal */ getUndefinedType(): Type;
/* @internal */ getNullType(): Type;
@@ -3731,7 +3731,7 @@ namespace ts {
Unit = Literal | UniqueESSymbol | Nullable,
StringOrNumberLiteral = StringLiteral | NumberLiteral,
/* @internal */
StringOrNumberLiteralOrUnique = StringOrNumberLiteral | UniqueESSymbol,
StringOrNumberLiteralOrUnique = StringLiteral | NumberLiteral | UniqueESSymbol,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change doesn't seem related (and I'm not sure it's an improvement?)

Copy link
Member Author

@weswigham weswigham Sep 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, I changed it because I actually removed StringOrNumberLiteral at first because it was practically unused within our code after the change, but then added it back to avoid an API break.

/* @internal */
DefinitelyFalsy = StringLiteral | NumberLiteral | BooleanLiteral | Void | Undefined | Null,
PossiblyFalsy = DefinitelyFalsy | String | Number | Boolean,
@@ -3802,6 +3802,15 @@ namespace ts {
intrinsicName: string; // Name of intrinsic type
}

/* @internal */
export interface FreshableIntrinsicType extends IntrinsicType {
freshType: IntrinsicType; // Fresh version of type
regularType: IntrinsicType; // Regular version of type
}

/* @internal */
export type FreshableType = LiteralType | FreshableIntrinsicType;

// String literal types (TypeFlags.StringLiteral)
// Numeric literal types (TypeFlags.NumberLiteral)
export interface LiteralType extends Type {
2 changes: 1 addition & 1 deletion src/services/codefixes/fixStrictClassInitialization.ts
Original file line number Diff line number Diff line change
@@ -110,7 +110,7 @@ namespace ts.codefix {

function getDefaultValueFromType (checker: TypeChecker, type: Type): Expression | undefined {
if (type.flags & TypeFlags.BooleanLiteral) {
return type === checker.getFalseType() ? createFalse() : createTrue();
return (type === checker.getFalseType() || type === checker.getFalseType(/*fresh*/ true)) ? createFalse() : createTrue();
}
else if (type.isLiteral()) {
return createLiteral(type.value);
2 changes: 1 addition & 1 deletion tests/baselines/reference/ambientConstLiterals.js
Original file line number Diff line number Diff line change
@@ -63,7 +63,7 @@ declare const c3 = "abc";
declare const c4 = 123;
declare const c5 = 123;
declare const c6 = -123;
declare const c7: boolean;
declare const c7 = true;
declare const c8 = E.A;
declare const c8b = E["non identifier"];
declare const c9: {
6 changes: 3 additions & 3 deletions tests/baselines/reference/booleanLiteralTypes1.types
Original file line number Diff line number Diff line change
@@ -82,13 +82,13 @@ function f4(t: true, f: false) {
>false : false

var x1 = t && f;
>x1 : boolean
>x1 : false
>t && f : false
>t : true
>f : false

var x2 = f && t;
>x2 : boolean
>x2 : false
>f && t : false
>f : false
>t : true
@@ -100,7 +100,7 @@ function f4(t: true, f: false) {
>f : false

var x4 = f || t;
>x4 : boolean
>x4 : true
>f || t : true
>f : false
>t : true
8 changes: 4 additions & 4 deletions tests/baselines/reference/booleanLiteralTypes2.types
Original file line number Diff line number Diff line change
@@ -82,25 +82,25 @@ function f4(t: true, f: false) {
>false : false

var x1 = t && f;
>x1 : boolean
>x1 : false
>t && f : false
>t : true
>f : false

var x2 = f && t;
>x2 : boolean
>x2 : false
>f && t : false
>f : false
>t : true

var x3 = t || f;
>x3 : boolean
>x3 : true
>t || f : true
>t : true
>f : false

var x4 = f || t;
>x4 : boolean
>x4 : true
>f || t : true
>f : false
>t : true
2 changes: 1 addition & 1 deletion tests/baselines/reference/constDeclarations.js
Original file line number Diff line number Diff line change
@@ -24,6 +24,6 @@ for (const c5 = 0, c6 = 0; c5 < c6;) {


//// [constDeclarations.d.ts]
declare const c1: boolean;
declare const c1 = false;
declare const c2: number;
declare const c3 = 0, c4: string, c5: any;
2 changes: 1 addition & 1 deletion tests/baselines/reference/constDeclarations2.js
Original file line number Diff line number Diff line change
@@ -19,7 +19,7 @@ var M;

//// [constDeclarations2.d.ts]
declare module M {
const c1: boolean;
const c1 = false;
const c2: number;
const c3 = 0, c4: string, c5: any;
}
Loading