Skip to content

feat: add type narrow to support better type check #2352

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

Closed
wants to merge 60 commits into from
Closed

feat: add type narrow to support better type check #2352

Show file tree
Hide file tree
Changes from 34 commits
Commits
Show all changes
60 commits
Select commit Hold shift + click to select a range
df38498
feat: add type narrow to support better type check
HerrCai0907 Jun 30, 2022
f55e01a
Revert "feat: add type narrow to support better type check"
HerrCai0907 Jul 1, 2022
aab3bfd
add narrow map in flow and use narrowedType if available [WIP]
HerrCai0907 Jul 1, 2022
5fd12b3
fix: style
HerrCai0907 Jul 1, 2022
c623709
update
HerrCai0907 Jul 3, 2022
999ce39
add testcase
HerrCai0907 Jul 3, 2022
9bfac7e
fix: or operator
HerrCai0907 Jul 3, 2022
2ccd59b
rename function
HerrCai0907 Jul 3, 2022
3c7d0dd
support logic and / or in condition
HerrCai0907 Jul 3, 2022
bbc3fe1
test: add test case for #2359
HerrCai0907 Jul 4, 2022
86f7bce
fix: Fix `||` and `&&` will not inherit branch status
HerrCai0907 Jul 4, 2022
4ea7f9e
update test
HerrCai0907 Jul 4, 2022
58d53a4
distinguish condi kind
HerrCai0907 Jul 4, 2022
e6b9291
update
HerrCai0907 Jul 4, 2022
a9d22ea
refactory: use class replace map
HerrCai0907 Jul 4, 2022
f344b10
Merge branch 'fix/nullable-flag-not-set-after-bool-operator' into fea…
HerrCai0907 Jul 5, 2022
53d5811
fix: assign in condition does not reset element type correctly
HerrCai0907 Jul 5, 2022
df62468
refactory: clean useless code
HerrCai0907 Jul 5, 2022
5cac092
test: add assign in condition testcase
HerrCai0907 Jul 5, 2022
67ce5bd
fix: reduce memory usage
HerrCai0907 Jul 5, 2022
450d490
refactory: add invertedCondition
HerrCai0907 Jul 5, 2022
b0f3aef
test: add unknown condition testcase
HerrCai0907 Jul 5, 2022
3ae97bd
test: add assign testcase
HerrCai0907 Jul 5, 2022
3277d93
update testcase
HerrCai0907 Jul 5, 2022
c5d94fa
Update src/narrow.ts
HerrCai0907 Jul 5, 2022
b1ea921
Update src/narrow.ts
HerrCai0907 Jul 5, 2022
8979ac8
lazy init narrowedType
HerrCai0907 Jul 6, 2022
cecd1a8
Merge branch 'main' into feat/add-type-narrow
HerrCai0907 Jul 22, 2022
2abc9a4
refactory: merge nonnull flag and narrowedTypeMap
HerrCai0907 Jul 28, 2022
60547ae
Merge branch 'main' into feat/add-type-narrow
HerrCai0907 Jul 28, 2022
81a63e3
fix: unused variant
HerrCai0907 Jul 28, 2022
6625289
fix bugs
HerrCai0907 Jul 28, 2022
5b244a3
fix: split logic for type and nullable
HerrCai0907 Jul 28, 2022
403cae8
docs: add comment for function
HerrCai0907 Jul 28, 2022
9115a5c
Update src/compiler.ts
HerrCai0907 Jul 28, 2022
a01d883
feat: add type infer for `||` operator
HerrCai0907 Aug 3, 2022
4824b44
Update src/flow.ts
HerrCai0907 Aug 3, 2022
9b23ad2
Update src/narrow.ts
MaxGraey Aug 4, 2022
4ea58aa
revert: testcase update in 2abc9a4a81be4e30c1fb3386e598626406c3f3e5
HerrCai0907 Aug 4, 2022
f0763d0
fix: type narrowing only effect for Local
HerrCai0907 Aug 4, 2022
71f26aa
update other testcase
HerrCai0907 Aug 4, 2022
db96d70
revert `return assignmentExpression;`
HerrCai0907 Aug 4, 2022
2506962
replace `if(a == null) a = new expression` with `||`
HerrCai0907 Aug 4, 2022
e245743
fix: lint error
HerrCai0907 Aug 4, 2022
5d401b8
add more testcases for nullable
HerrCai0907 Aug 4, 2022
dc2f28a
rename `conditionalNarrowedType` with `typeNarrowChecker`
HerrCai0907 Aug 4, 2022
41d4e4b
refactory testcase
HerrCai0907 Aug 5, 2022
265eff1
fix logic or narrow bugs
HerrCai0907 Aug 5, 2022
e2256c8
remove `||` assert
HerrCai0907 Aug 5, 2022
9af6064
fix: remove default initialized typeNarrowChecker
HerrCai0907 Aug 5, 2022
ac12ee8
fix
HerrCai0907 Aug 5, 2022
0a0135b
refactory `forkTrueBranch` and `forkFalseBranch`
HerrCai0907 Aug 5, 2022
bfc9ed8
refactory type narrow in `inheritBranch`
HerrCai0907 Aug 6, 2022
85387c8
test: add initType testcase
HerrCai0907 Aug 10, 2022
fb117fa
switch position for `inheritBranch` and `freeScopedLocals`
HerrCai0907 Aug 10, 2022
3554be4
copy `narrowedTypes` when `inhertiMutual` inherit 1 flow
HerrCai0907 Aug 10, 2022
3edce9d
refacrory `getVariantType`
HerrCai0907 Aug 10, 2022
51de90e
Merge branch 'main' into feat/add-type-narrow
HerrCai0907 Aug 21, 2022
c11571f
fix merge conflict
HerrCai0907 Aug 21, 2022
5ff9408
Merge branch 'main' into feat/add-type-narrow
HerrCai0907 Sep 3, 2022
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
104 changes: 68 additions & 36 deletions src/compiler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@ import {
PropertyPrototype,
IndexSignature,
File,
mangleInternalName
mangleInternalName,
TypedElement
} from "./program";

import {
Expand All @@ -96,7 +97,8 @@ import {
LocalFlags,
FieldFlags,
ConditionKind,
findUsedLocals
findUsedLocals,
invertedCondition
} from "./flow";

import {
Expand Down Expand Up @@ -2554,7 +2556,7 @@ export class Compiler extends DiagnosticEmitter {

// Compile the body assuming the condition turned out true
var bodyFlow = flow.fork();
bodyFlow.inheritNonnullIfTrue(condExpr);
bodyFlow.inheritNarrowedTypeIfTrue(condExpr);
this.currentFlow = bodyFlow;
var bodyStmts = new Array<ExpressionRef>();
var body = statement.statement;
Expand Down Expand Up @@ -2699,7 +2701,8 @@ export class Compiler extends DiagnosticEmitter {
var thenStmts = new Array<ExpressionRef>();
var thenFlow = flow.fork();
this.currentFlow = thenFlow;
thenFlow.inheritNonnullIfTrue(condExpr);
thenFlow.inheritNarrowedTypeIfTrue(condExpr);

if (ifTrue.kind == NodeKind.BLOCK) {
this.compileStatements((<BlockStatement>ifTrue).statements, false, thenStmts);
} else {
Expand All @@ -2717,7 +2720,7 @@ export class Compiler extends DiagnosticEmitter {
let elseStmts = new Array<ExpressionRef>();
let elseFlow = flow.fork();
this.currentFlow = elseFlow;
elseFlow.inheritNonnullIfFalse(condExpr);
elseFlow.inheritNarrowedTypeIfFalse(condExpr);
if (ifFalse.kind == NodeKind.BLOCK) {
this.compileStatements((<BlockStatement>ifFalse).statements, false, elseStmts);
} else {
Expand All @@ -2735,12 +2738,11 @@ export class Compiler extends DiagnosticEmitter {
module.flatten(elseStmts)
);
} else {
flow.inheritNarrowedTypeIfFalse(condExpr);
flow.inheritBranch(thenFlow);
flow.inheritNonnullIfFalse(condExpr,
thenFlow.isAny(FlowFlags.TERMINATES | FlowFlags.BREAKS)
? null // thenFlow terminates: just inherit
: thenFlow // must become nonnull in thenFlow otherwise
);
if (thenFlow.isAny(FlowFlags.TERMINATES | FlowFlags.BREAKS)) {
flow.inheritNarrowedTypeIfFalse(condExpr);
}
return module.if(condExpr,
module.flatten(thenStmts)
);
Expand Down Expand Up @@ -2972,6 +2974,7 @@ export class Compiler extends DiagnosticEmitter {
let name = declaration.name.text;
let type: Type | null = null;
let initExpr: ExpressionRef = 0;
let initType: Type | null = null;

// Resolve type if annotated
let typeNode = declaration.type;
Expand All @@ -2994,6 +2997,8 @@ export class Compiler extends DiagnosticEmitter {
);
pendingElements.delete(dummy);
flow.freeScopedDummyLocal(name);

initType = this.currentType;
Copy link
Member

Choose a reason for hiding this comment

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

Due to compiling with Constraincts.CONV_IMPLICIT, I think initType is always the same as type here. Sure that the additional initType variable is needed?

Copy link
Member Author

@HerrCai0907 HerrCai0907 Aug 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

consider this testcase. Actually initType and type are not always the same in nullable

export function testInit(a: Ref): void {
  let c: Ref | null = requireNonNull(a);
  if (isNullable(c)) ERROR("should be non-nullable");
}

}

// Otherwise infer type from initializer
Expand All @@ -3013,6 +3018,7 @@ export class Compiler extends DiagnosticEmitter {
continue;
}
type = this.currentType;
initType = this.currentType;

// Error if there's neither a type nor an initializer
} else {
Expand Down Expand Up @@ -3135,7 +3141,7 @@ export class Compiler extends DiagnosticEmitter {
}
if (initExpr) {
initializers.push(
this.makeLocalAssignment(local, initExpr, type, false)
this.makeLocalAssignment(local, initExpr, initType ? initType : type, false)
);
} else {
// no need to assign zero
Expand Down Expand Up @@ -3235,7 +3241,7 @@ export class Compiler extends DiagnosticEmitter {

// Compile the body assuming the condition turned out true
var bodyFlow = flow.fork();
bodyFlow.inheritNonnullIfTrue(condExpr);
bodyFlow.inheritNarrowedTypeIfTrue(condExpr);
Copy link
Member

Choose a reason for hiding this comment

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

It seems to me that once narrowing is in place, that the nonnull machinery becomes obsolete. Hence wondering if it would be sufficient to upgrade the nonnull machinery to narrowing so there is only one concept in the end?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, and it can resolve the nonull bug mentioned in the last of #2360.
But I think maybe we can do it one by one instead of doing everything in this PR? It maybe make this PR very hard to review

this.currentFlow = bodyFlow;
var bodyStmts = new Array<ExpressionRef>();
var body = statement.statement;
Expand Down Expand Up @@ -4543,7 +4549,7 @@ export class Compiler extends DiagnosticEmitter {

let rightFlow = flow.fork();
this.currentFlow = rightFlow;
rightFlow.inheritNonnullIfTrue(leftExpr);
rightFlow.inheritNarrowedTypeIfTrue(leftExpr);

// simplify if only interested in true or false
if (contextualType == Type.bool || contextualType == Type.void) {
Expand All @@ -4566,13 +4572,15 @@ export class Compiler extends DiagnosticEmitter {
expr = module.if(leftExpr, rightExpr, module.i32(0));
}
}
flow.inheritBranch(rightFlow, condKind);
this.currentFlow = flow;
this.currentType = Type.bool;

} else {
rightExpr = this.compileExpression(right, leftType, inheritedConstraints | Constraints.CONV_IMPLICIT);
rightType = this.currentType;
rightFlow.freeScopedLocals();
flow.inheritBranch(rightFlow);
this.currentFlow = flow;

// simplify if copying left is trivial
Expand All @@ -4587,7 +4595,7 @@ export class Compiler extends DiagnosticEmitter {
} else {
let tempLocal = flow.getTempLocal(leftType);
if (!flow.canOverflow(leftExpr, leftType)) flow.setLocalFlag(tempLocal.index, LocalFlags.WRAPPED);
if (flow.isNonnull(leftExpr, leftType)) flow.setLocalFlag(tempLocal.index, LocalFlags.NONNULL);
if (flow.isNonnull(leftExpr, leftType)) flow.setNarrowedType(tempLocal, leftType.nonNullableType);
expr = module.if(
this.makeIsTrueish(module.local_tee(tempLocal.index, leftExpr, leftType.isManaged), leftType, left),
rightExpr,
Expand All @@ -4607,7 +4615,7 @@ export class Compiler extends DiagnosticEmitter {

let rightFlow = flow.fork();
this.currentFlow = rightFlow;
rightFlow.inheritNonnullIfFalse(leftExpr);
rightFlow.inheritNarrowedTypeIfFalse(leftExpr);

// simplify if only interested in true or false
if (contextualType == Type.bool || contextualType == Type.void) {
Expand All @@ -4630,13 +4638,15 @@ export class Compiler extends DiagnosticEmitter {
expr = module.if(leftExpr, module.i32(1), rightExpr);
}
}
flow.inheritBranch(rightFlow, invertedCondition(condKind));
this.currentFlow = flow;
this.currentType = Type.bool;

} else {
rightExpr = this.compileExpression(right, leftType, inheritedConstraints | Constraints.CONV_IMPLICIT);
rightType = this.currentType;
rightFlow.freeScopedLocals();
flow.inheritBranch(rightFlow);
this.currentFlow = flow;

// simplify if copying left is trivial
Expand All @@ -4651,7 +4661,7 @@ export class Compiler extends DiagnosticEmitter {
} else {
let temp = flow.getTempLocal(leftType);
if (!flow.canOverflow(leftExpr, leftType)) flow.setLocalFlag(temp.index, LocalFlags.WRAPPED);
if (flow.isNonnull(leftExpr, leftType)) flow.setLocalFlag(temp.index, LocalFlags.NONNULL);
if (flow.isNonnull(leftExpr, leftType)) flow.setNarrowedType(temp, leftType.nonNullableType);
expr = module.if(
this.makeIsTrueish(module.local_tee(temp.index, leftExpr, leftType.isManaged), leftType, left),
module.local_get(temp.index, leftType.toRef()),
Expand Down Expand Up @@ -5938,7 +5948,8 @@ export class Compiler extends DiagnosticEmitter {
assert(targetType != Type.void);
var valueExpr = this.compileExpression(valueExpression, targetType);
var valueType = this.currentType;
return this.makeAssignment(

let assignmentExpression = this.makeAssignment(
target,
this.convertExpression(valueExpr, valueType, targetType, false, valueExpression),
valueType,
Expand All @@ -5947,6 +5958,7 @@ export class Compiler extends DiagnosticEmitter {
elementExpression,
contextualType != Type.void
);
return assignmentExpression;
}

/** Makes an assignment expression or block, assigning a value to a target. */
Expand Down Expand Up @@ -6155,28 +6167,33 @@ export class Compiler extends DiagnosticEmitter {
/** Whether to tee the value. */
tee: bool
): ExpressionRef {
let expressionRef: ExpressionRef;
var module = this.module;
var flow = this.currentFlow;
var type = local.type;
assert(type != Type.void);
var localIndex = local.index;

if (type.isNullableReference) {
if (!valueType.isNullableReference || flow.isNonnull(valueExpr, type)) flow.setLocalFlag(localIndex, LocalFlags.NONNULL);
else flow.unsetLocalFlag(localIndex, LocalFlags.NONNULL);
}
flow.setLocalFlag(localIndex, LocalFlags.INITIALIZED);
if (type.isShortIntegerValue) {
if (!flow.canOverflow(valueExpr, type)) flow.setLocalFlag(localIndex, LocalFlags.WRAPPED);
else flow.unsetLocalFlag(localIndex, LocalFlags.WRAPPED);
}
if (tee) { // local = value
this.currentType = type;
return module.local_tee(localIndex, valueExpr, type.isManaged);
expressionRef = module.local_tee(localIndex, valueExpr, type.isManaged);
} else { // void(local = value)
this.currentType = Type.void;
return module.local_set(localIndex, valueExpr, type.isManaged);
expressionRef = module.local_set(localIndex, valueExpr, type.isManaged);
}
if (type.isReference) {
let narrowedType = valueType;
if (!valueType.isNullableReference || flow.isNonnull(valueExpr, type)) {
narrowedType = valueType.nonNullableType;
}
flow.setAssignType(expressionRef, local, narrowedType);
}
return expressionRef;
}

/** Makes an assignment to a global. */
Expand All @@ -6190,24 +6207,33 @@ export class Compiler extends DiagnosticEmitter {
/** Whether to tee the value. */
tee: bool
): ExpressionRef {
let expressionRef: ExpressionRef;
var module = this.module;
var type = global.type;
var flow = this.currentFlow;
assert(type != Type.void);
var typeRef = type.toRef();

valueExpr = this.ensureSmallIntegerWrap(valueExpr, type); // globals must be wrapped
if (tee) { // (global = value), global
this.currentType = type;
return module.block(null, [
expressionRef = module.block(null, [
module.global_set(global.internalName, valueExpr),
module.global_get(global.internalName, typeRef)
], typeRef);
} else { // global = value
this.currentType = Type.void;
return module.global_set(global.internalName,
expressionRef = module.global_set(global.internalName,
valueExpr
);
}
if (type.isReference) {
let narrowedType = valueType;
if (!valueType.isNullableReference || flow.isNonnull(valueExpr, type)) {
narrowedType = valueType.nonNullableType;
}
flow.setAssignType(expressionRef, global, narrowedType);
}
return expressionRef;
}

/** Makes an assignment to a field. */
Expand Down Expand Up @@ -6768,7 +6794,7 @@ export class Compiler extends DiagnosticEmitter {
findUsedLocals(paramExpr, usedLocals);
// inlining is aware of wrap/nonnull states:
if (!previousFlow.canOverflow(paramExpr, paramType)) flow.setLocalFlag(argumentLocal.index, LocalFlags.WRAPPED);
if (flow.isNonnull(paramExpr, paramType)) flow.setLocalFlag(argumentLocal.index, LocalFlags.NONNULL);
if (flow.isNonnull(paramExpr, paramType) && paramType.isNullableReference) flow.setNarrowedType(argumentLocal, paramType.nonNullableType);
body.unshift(
module.local_set(argumentLocal.index, paramExpr, paramType.isManaged)
);
Expand Down Expand Up @@ -7761,7 +7787,8 @@ export class Compiler extends DiagnosticEmitter {
switch (target.kind) {
case ElementKind.LOCAL: {
let local = <Local>target;
let localType = local.type;
let narrowedType = flow.getNarrowedType(local);
let localType = narrowedType ? narrowedType : local.type;
assert(localType != Type.void);
if (this.pendingElements.has(local)) {
this.error(
Expand All @@ -7777,9 +7804,6 @@ export class Compiler extends DiagnosticEmitter {
}
let localIndex = local.index;
assert(localIndex >= 0);
if (localType.isNullableReference && flow.isLocalFlag(localIndex, LocalFlags.NONNULL, false)) {
localType = localType.nonNullableType;
}
this.currentType = localType;

if (target.parent != flow.parentFunction) {
Expand All @@ -7798,7 +7822,8 @@ export class Compiler extends DiagnosticEmitter {
if (!this.compileGlobal(global)) { // reports; not yet compiled if a static field
return module.unreachable();
}
let globalType = global.type;
let narrowedType = flow.getNarrowedType(global);
let globalType = narrowedType ? narrowedType : global.type;
if (this.pendingElements.has(global)) {
this.error(
DiagnosticCode.Variable_0_used_before_its_declaration,
Expand Down Expand Up @@ -7908,7 +7933,14 @@ export class Compiler extends DiagnosticEmitter {
this.currentType = Type.bool;
return this.module.unreachable();
}
return this.makeInstanceofType(expression, expectedType);
let instanceExpression = this.makeInstanceofType(expression, expectedType);
if (expression.expression.kind == NodeKind.IDENTIFIER) {
let element = flow.lookup((<IdentifierExpression>expression.expression).text);
if (element instanceof TypedElement) {
flow.setConditionNarrowedType(instanceExpression, <TypedElement>element, expectedType);
}
}
return instanceExpression;
}

private makeInstanceofType(expression: InstanceOfExpression, expectedType: Type): ExpressionRef {
Expand Down Expand Up @@ -9264,13 +9296,13 @@ export class Compiler extends DiagnosticEmitter {

var outerFlow = this.currentFlow;
var ifThenFlow = outerFlow.fork();
ifThenFlow.inheritNonnullIfTrue(condExpr);
ifThenFlow.inheritNarrowedTypeIfTrue(condExpr);
this.currentFlow = ifThenFlow;
var ifThenExpr = this.compileExpression(ifThen, ctxType);
var ifThenType = this.currentType;

var ifElseFlow = outerFlow.fork();
ifElseFlow.inheritNonnullIfFalse(condExpr);
ifElseFlow.inheritNarrowedTypeIfFalse(condExpr);
this.currentFlow = ifElseFlow;
var ifElseExpr = this.compileExpression(ifElse, ctxType == Type.auto ? ifThenType : ctxType);
var ifElseType = this.currentType;
Expand Down Expand Up @@ -10587,7 +10619,7 @@ export class Compiler extends DiagnosticEmitter {
var flow = this.currentFlow;
var temp = flow.getTempLocal(type);
if (!flow.canOverflow(expr, type)) flow.setLocalFlag(temp.index, LocalFlags.WRAPPED);
flow.setLocalFlag(temp.index, LocalFlags.NONNULL);
flow.setNarrowedType(temp, type);

var staticAbortCallExpr = this.makeStaticAbort(
this.ensureStaticString("unexpected null"),
Expand Down
Loading