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.

Sign up for GitHub

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

Introduce boolean literal freshness #27042

Merged
merged 1 commit into from
Sep 14, 2018
+165 −58
Merged

Introduce boolean literal freshness #27042

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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 •
edited
Loading

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");
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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;
Expand All @@ -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;
}
Expand Down Expand Up @@ -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) ||
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}

Expand All @@ -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 :
Expand Down Expand Up @@ -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 •
edited
Loading

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
Expand Down Expand Up @@ -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) ?
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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) {
Expand Down
15 changes: 12 additions & 3 deletions src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 •
edited
Loading

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,
Expand Down Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion src/services/codefixes/fixStrictClassInitialization.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/ambientConstLiterals.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand Down
6 changes: 3 additions & 3 deletions tests/baselines/reference/booleanLiteralTypes1.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
8 changes: 4 additions & 4 deletions tests/baselines/reference/booleanLiteralTypes2.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/constDeclarations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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