From a110f9b5b58306c4e8f90ea51fa82bd27e35a141 Mon Sep 17 00:00:00 2001 From: Vsevolod Pukhov Date: Fri, 22 Sep 2023 17:48:48 +0300 Subject: [PATCH] [ArkTS Linter] Permit more ClassAsObject cases, add advanced checks disabled by default Signed-off-by: Vsevolod Pukhov --- linter-4.2/src/TypeScriptLinter.ts | 38 +++++++---- linter-4.2/src/Utils.ts | 12 ++-- linter-4.2/test/class_as_object.ts | 19 +++++- linter-4.2/test/oh_modules/ohos_factory.ts | 11 +++- linter/src/TypeScriptLinter.ts | 63 ++++++++++++++----- linter/src/utils/TsUtils.ts | 40 ++++++++++-- .../functions/identiferUseInValueContext.ts | 6 +- linter/test/class_as_object.ts | 21 +++++-- linter/test/oh_modules/ohos_factory.ts | 11 +++- 9 files changed, 169 insertions(+), 52 deletions(-) diff --git a/linter-4.2/src/TypeScriptLinter.ts b/linter-4.2/src/TypeScriptLinter.ts index 0eb571450..39fef04c0 100644 --- a/linter-4.2/src/TypeScriptLinter.ts +++ b/linter-4.2/src/TypeScriptLinter.ts @@ -1556,9 +1556,29 @@ export class TypeScriptLinter { } } + private isAllowedClassValueContext(tsIdentifier: ts.Identifier, tsIdentSym: ts.Symbol): boolean { + let ctx: ts.Node = tsIdentifier; + while (ts.isPropertyAccessExpression(ctx.parent) || ts.isQualifiedName(ctx.parent)) { + ctx = ctx.parent; + } + if (ts.isPropertyAssignment(ctx.parent) && ts.isObjectLiteralExpression(ctx.parent.parent)) { + ctx = ctx.parent.parent; + } + if (ts.isArrowFunction(ctx.parent) && ctx.parent.body == ctx) { + ctx = ctx.parent; + } + + if (ts.isCallExpression(ctx.parent) || ts.isNewExpression(ctx.parent)) { + let callee = ctx.parent.expression; + if (callee != ctx && this.tsUtils.hasLibraryType(callee)) { + return true; + } + } + return false; + } + private handleRestrictedValues(tsIdentifier: ts.Identifier, tsIdentSym: ts.Symbol) { - const illegalValues = ts.SymbolFlags.Class | ts.SymbolFlags.ConstEnum | ts.SymbolFlags.RegularEnum | - ts.SymbolFlags.ValueModule; + const illegalValues = ts.SymbolFlags.ConstEnum | ts.SymbolFlags.RegularEnum | ts.SymbolFlags.ValueModule | ts.SymbolFlags.Class; // If module name is duplicated by another declaration, this increases the possibility // of finding a lot of false positives. Thus, do not check further in that case. @@ -1567,22 +1587,14 @@ export class TypeScriptLinter { return; } } - // No check for ArkUI struct. - if (this.tsUtils.isStruct(tsIdentSym)) { - return; - } - if ((tsIdentSym.flags & illegalValues) == 0 || !this.identiferUseInValueContext(tsIdentifier, tsIdentSym)) { + if ((tsIdentSym.flags & illegalValues) == 0 || this.tsUtils.isStruct(tsIdentSym) || + !this.identiferUseInValueContext(tsIdentifier, tsIdentSym)) { return; } if ((tsIdentSym.flags & ts.SymbolFlags.Class) != 0) { - let ctxNode: ts.Node = tsIdentifier; - while (ts.isPropertyAccessExpression(ctxNode.parent) || ts.isQualifiedName(ctxNode.parent)) { - ctxNode = ctxNode.parent; - } - ctxNode = ctxNode.parent; - if (ts.isCallExpression(ctxNode) && this.tsUtils.hasLibraryType((ctxNode as ts.CallExpression).expression)) { + if (this.isAllowedClassValueContext(tsIdentifier, tsIdentSym)) { return; } } diff --git a/linter-4.2/src/Utils.ts b/linter-4.2/src/Utils.ts index 489a63400..e45d6f68a 100644 --- a/linter-4.2/src/Utils.ts +++ b/linter-4.2/src/Utils.ts @@ -698,10 +698,14 @@ export class TsUtils { } public isObjectType(tsType: ts.Type): boolean { - return ( - tsType && tsType.isClassOrInterface() && tsType.symbol && - (tsType.symbol.name === 'Object' || tsType.symbol.name === 'object') - ); + if (!tsType) { + return false; + } + if (tsType.symbol && (tsType.isClassOrInterface() && tsType.symbol.name === 'Object')) { + return true; + } + let node = this.tsTypeChecker.typeToTypeNode(tsType, undefined, undefined); + return node != undefined && node.kind === ts.SyntaxKind.ObjectKeyword; } public isCallToFunctionWithOmittedReturnType(tsExpr: ts.Expression): boolean { diff --git a/linter-4.2/test/class_as_object.ts b/linter-4.2/test/class_as_object.ts index 7a20f0a9f..2b3b3b1e5 100644 --- a/linter-4.2/test/class_as_object.ts +++ b/linter-4.2/test/class_as_object.ts @@ -13,7 +13,7 @@ * limitations under the License. */ -import { Something, SomethingFactory, SomethingBar } from "./oh_modules/ohos_factory"; +import { Something, SomethingFactory, SomethingBar, Bar } from "./oh_modules/ohos_factory"; class C { static a = 5; @@ -63,7 +63,20 @@ type X = D; namespace test1 { class SomethingFoo extends Something { } + namespace NS { export class SomethingFoo extends Something { } } - let x = SomethingFactory.getInstance().create(SomethingFoo).beep(); - let y = SomethingFactory.getInstance().create(SomethingBar).beep(); + let fact = SomethingFactory.getInstance(); + + let x1 = fact.create1(SomethingFoo).beep(); + let x2 = fact.create1(SomethingBar).beep(); + let x3 = fact.create1(NS.SomethingFoo).beep(); + + let x4 = fact.create2({ o: SomethingFoo }); + let x5 = fact.create2({ o: SomethingBar }); + let x6 = fact.create2({ o: NS.SomethingFoo }); + + let x7 = fact.create3(() => SomethingFoo); + let x8 = fact.create4(() => SomethingFoo); + + let x9 = new Bar(SomethingFoo); } diff --git a/linter-4.2/test/oh_modules/ohos_factory.ts b/linter-4.2/test/oh_modules/ohos_factory.ts index 506f00e62..e26c4c911 100644 --- a/linter-4.2/test/oh_modules/ohos_factory.ts +++ b/linter-4.2/test/oh_modules/ohos_factory.ts @@ -5,7 +5,14 @@ export declare class SomethingFactory { public static getInstance(): SomethingFactory; - public create(smth: { new(): T }): T; + public create1(arg: { new(): T }): T; + public create2(arg: { o: { new(): T } }): T; + public create3(arg: () => { new(): T }): T; + public create4(arg: Function): T; } -export declare class SomethingBar extends Something {} +export declare class SomethingBar extends Something { } + +export declare class Bar { + constructor(arg: { new(): T }); +} diff --git a/linter/src/TypeScriptLinter.ts b/linter/src/TypeScriptLinter.ts index f37ba9bce..ac3d45c47 100644 --- a/linter/src/TypeScriptLinter.ts +++ b/linter/src/TypeScriptLinter.ts @@ -47,6 +47,7 @@ import { scopeContainsThis } from './utils/functions/ContainsThis'; import { isStructDeclaration, isStruct } from './utils/functions/IsStruct'; import { IncrementalLintInfo } from './IncrementalLintInfo'; import { cookBookRefToFixTitle } from './AutofixTitles'; +import { isStdLibraryType } from './utils/functions/IsStdLibrary'; const logger = Logger.getLogger(); @@ -84,6 +85,8 @@ export class TypeScriptLinter { static ideMode: boolean = false; static testMode: boolean = false; + static advanced_class_checks = false; + constructor( private tsTypeChecker: ts.TypeChecker, private autofixesInfo: AutofixInfoSet, @@ -93,7 +96,7 @@ export class TypeScriptLinter { private incrementalLintInfo?: IncrementalLintInfo, private tscStrictDiagnostics?: Map ) { - this.tsUtils = new TsUtils(this.tsTypeChecker, TypeScriptLinter.testMode); + this.tsUtils = new TsUtils(this.tsTypeChecker, TypeScriptLinter.testMode, TypeScriptLinter.advanced_class_checks); this.currentErrorLine = 0; this.currentWarningLine = 0; this.staticBlocks = new Set(); @@ -653,7 +656,14 @@ export class TypeScriptLinter { if(!!symbol && this.tsUtils.isSymbolAPI(symbol)) { this.incrementCounters(node, FaultID.SymbolType); } - + let lhsType = this.tsTypeChecker.getTypeAtLocation(node); + if ((lhsType.flags & ts.TypeFlags.Union) !== 0) { + // nothing is reported, bug? + } + if (TypeScriptLinter.advanced_class_checks && this.tsUtils.isClassObjectExpression(propertyAccessNode.expression)) { + // missing exact rule + this.incrementCounters(propertyAccessNode.expression, FaultID.ClassAsObject); + } if (this.tsUtils.hasEsObjectType(propertyAccessNode.expression)) { this.incrementCounters(node, FaultID.EsObjectAccess); } @@ -1278,9 +1288,31 @@ export class TypeScriptLinter { } } + // hard-coded alternative to TypeScriptLinter.advanced_class_checks + private isAllowedClassValueContext(tsIdentifier: ts.Identifier, tsIdentSym: ts.Symbol): boolean { + let ctx: ts.Node = tsIdentifier; + while (ts.isPropertyAccessExpression(ctx.parent) || ts.isQualifiedName(ctx.parent)) { + ctx = ctx.parent; + } + if (ts.isPropertyAssignment(ctx.parent) && ts.isObjectLiteralExpression(ctx.parent.parent)) { + ctx = ctx.parent.parent; + } + if (ts.isArrowFunction(ctx.parent) && ctx.parent.body == ctx) { + ctx = ctx.parent; + } + + if (ts.isCallExpression(ctx.parent) || ts.isNewExpression(ctx.parent)) { + let callee = ctx.parent.expression; + if (callee != ctx && this.tsUtils.hasLibraryType(callee)) { + return true; + } + } + return false; + } + private handleRestrictedValues(tsIdentifier: ts.Identifier, tsIdentSym: ts.Symbol) { - const illegalValues = ts.SymbolFlags.Class | ts.SymbolFlags.ConstEnum | ts.SymbolFlags.RegularEnum | - ts.SymbolFlags.ValueModule; + const illegalValues = ts.SymbolFlags.ConstEnum | ts.SymbolFlags.RegularEnum | ts.SymbolFlags.ValueModule | + (TypeScriptLinter.advanced_class_checks? 0 : ts.SymbolFlags.Class); // If module name is duplicated by another declaration, this increases the possibility // of finding a lot of false positives. Thus, do not check further in that case. @@ -1289,22 +1321,13 @@ export class TypeScriptLinter { return; } } - // No check for ArkUI struct. - if (isStruct(tsIdentSym)) { - return; - } - if ((tsIdentSym.flags & illegalValues) == 0 || !identiferUseInValueContext(tsIdentifier, tsIdentSym)) { + if ((tsIdentSym.flags & illegalValues) == 0 || isStruct(tsIdentSym) || !identiferUseInValueContext(tsIdentifier, tsIdentSym)) { return; } if ((tsIdentSym.flags & ts.SymbolFlags.Class) != 0) { - let ctxNode: ts.Node = tsIdentifier; - while (ts.isPropertyAccessExpression(ctxNode.parent) || ts.isQualifiedName(ctxNode.parent)) { - ctxNode = ctxNode.parent; - } - ctxNode = ctxNode.parent; - if (ts.isCallExpression(ctxNode) && this.tsUtils.hasLibraryType((ctxNode as ts.CallExpression).expression)) { + if (!TypeScriptLinter.advanced_class_checks && this.isAllowedClassValueContext(tsIdentifier, tsIdentSym)) { return; } } @@ -1590,6 +1613,16 @@ export class TypeScriptLinter { private handleNewExpression(node: ts.Node) { let tsNewExpr = node as ts.NewExpression; + + if (TypeScriptLinter.advanced_class_checks) { + let calleeExpr = tsNewExpr.expression; + let calleeType = this.tsTypeChecker.getTypeAtLocation(calleeExpr); + if (!this.tsUtils.isClassTypeExrepssion(calleeExpr) && !isStdLibraryType(calleeType) && + !this.tsUtils.isLibraryType(calleeType) && !this.tsUtils.hasEsObjectType(calleeExpr)) { + // missing exact rule + this.incrementCounters(calleeExpr, FaultID.ClassAsObject); + } + } this.handleGenericCallWithNoTypeArgs(tsNewExpr); this.handleStructIdentAndUndefinedInArgs(tsNewExpr); } diff --git a/linter/src/utils/TsUtils.ts b/linter/src/utils/TsUtils.ts index a68c46636..080deb47e 100644 --- a/linter/src/utils/TsUtils.ts +++ b/linter/src/utils/TsUtils.ts @@ -25,7 +25,7 @@ import { isStructDeclaration } from './functions/IsStruct'; import { pathContainsDirectory } from './functions/PathHelper'; export class TsUtils { - constructor(private tsTypeChecker: ts.TypeChecker, private testMode: boolean) { + constructor(private tsTypeChecker: ts.TypeChecker, private testMode: boolean, private advanced_class_checks: boolean) { } public isAssignmentOperator(tsBinOp: ts.BinaryOperatorToken): boolean { @@ -502,6 +502,10 @@ export class TsUtils { if (this.isLibraryType(typeTo)) { return false; } + if (this.advanced_class_checks && this.isClassValueType(typeFrom) && typeTo != typeFrom && !this.isObjectType(typeTo)) { + // missing exact rule + return true; + } let res = typeTo.isClassOrInterface() && typeFrom.isClassOrInterface() && !this.relatedByInheritanceOrIdentical(typeFrom, typeTo); @@ -522,10 +526,14 @@ export class TsUtils { } public isObjectType(tsType: ts.Type): boolean { - return ( - tsType && tsType.isClassOrInterface() && tsType.symbol && - (tsType.symbol.name === 'Object' || tsType.symbol.name === 'object') - ); + if (!tsType) { + return false; + } + if (tsType.symbol && (tsType.isClassOrInterface() && tsType.symbol.name === 'Object')) { + return true; + } + let node = this.tsTypeChecker.typeToTypeNode(tsType, undefined, undefined); + return node != undefined && node.kind === ts.SyntaxKind.ObjectKeyword; } public isCallToFunctionWithOmittedReturnType(tsExpr: ts.Expression): boolean { @@ -804,7 +812,7 @@ export class TsUtils { return true; } - return !ts.isTypeLiteralNode(typeNode) && !ts.isTypeQueryNode(typeNode) && + return !ts.isTypeLiteralNode(typeNode) && (this.advanced_class_checks || !ts.isTypeQueryNode(typeNode)) && !ts.isIntersectionTypeNode(typeNode) && !ts.isTupleTypeNode(typeNode) && this.isSupportedTypeNodeKind(typeNode.kind); } @@ -1117,4 +1125,24 @@ export class TsUtils { } return false; } + + public isClassValueType(type: ts.Type): boolean { + if ((type.flags & ts.TypeFlags.Object) === 0 || ((type as ts.ObjectType).objectFlags & ts.ObjectFlags.Anonymous) === 0) { + return false; + } + return type.symbol && (type.symbol.flags & ts.SymbolFlags.Class) !== 0; + } + + public isClassObjectExpression(expr: ts.Expression): boolean { + if (!this.isClassValueType(this.tsTypeChecker.getTypeAtLocation(expr))) { + return false; + } + let symbol = this.trueSymbolAtLocation(expr); + return !symbol || (symbol.flags & ts.SymbolFlags.Class) === 0; + } + + public isClassTypeExrepssion(expr: ts.Expression): boolean { + let sym = this.trueSymbolAtLocation(expr); + return sym !== undefined && (sym.flags & ts.SymbolFlags.Class) !== 0; + } } diff --git a/linter/src/utils/functions/identiferUseInValueContext.ts b/linter/src/utils/functions/identiferUseInValueContext.ts index 6927c5cf8..d0bb0e9a5 100644 --- a/linter/src/utils/functions/identiferUseInValueContext.ts +++ b/linter/src/utils/functions/identiferUseInValueContext.ts @@ -48,8 +48,8 @@ function isEnumPropAccess(ident: ts.Identifier, tsSym: ts.Symbol, context: ts.No !!(tsSym.flags & ts.SymbolFlags.Enum); } -function isTypeQuery(node: ts.Node): boolean { - return ts.isTypeNode(node) && !ts.isTypeOfExpression(node); +function isValidTypeNode(node: ts.TypeNode): boolean { + return !ts.isTypeOfExpression(node); } export function identiferUseInValueContext( @@ -59,7 +59,7 @@ export function identiferUseInValueContext( let parent = qualifiedStart.parent; return !( // treat TypeQuery as valid because it's already forbidden (FaultID.TypeQuery) - isTypeQuery(parent) || + ts.isTypeNode(parent) && isValidTypeNode(parent) || // If identifier is the right-most name of Property Access chain or Qualified name, // or it's a separate identifier expression, then identifier is being referenced as an value. isEnumPropAccess(ident, tsSym, parent) || diff --git a/linter/test/class_as_object.ts b/linter/test/class_as_object.ts index 6a8807056..a0b9d940b 100644 --- a/linter/test/class_as_object.ts +++ b/linter/test/class_as_object.ts @@ -13,7 +13,7 @@ * limitations under the License. */ -import { Something, SomethingFactory, SomethingBar } from "./oh_modules/ohos_factory"; +import { Something, SomethingFactory, SomethingBar, Bar } from "./oh_modules/ohos_factory"; class C { static a = 5 @@ -63,7 +63,20 @@ type X = D; namespace test1 { class SomethingFoo extends Something { } - - let x = SomethingFactory.getInstance().create(SomethingFoo).beep(); - let y = SomethingFactory.getInstance().create(SomethingBar).beep(); + namespace NS { export class SomethingFoo extends Something { } } + + let fact = SomethingFactory.getInstance(); + + let x1 = fact.create1(SomethingFoo).beep(); + let x2 = fact.create1(SomethingBar).beep(); + let x3 = fact.create1(NS.SomethingFoo).beep(); + + let x4 = fact.create2({ o: SomethingFoo }); + let x5 = fact.create2({ o: SomethingBar }); + let x6 = fact.create2({ o: NS.SomethingFoo }); + + let x7 = fact.create3(() => SomethingFoo); + let x8 = fact.create4(() => SomethingFoo); + + let x9 = new Bar(SomethingFoo); } diff --git a/linter/test/oh_modules/ohos_factory.ts b/linter/test/oh_modules/ohos_factory.ts index 506f00e62..e26c4c911 100644 --- a/linter/test/oh_modules/ohos_factory.ts +++ b/linter/test/oh_modules/ohos_factory.ts @@ -5,7 +5,14 @@ export declare class SomethingFactory { public static getInstance(): SomethingFactory; - public create(smth: { new(): T }): T; + public create1(arg: { new(): T }): T; + public create2(arg: { o: { new(): T } }): T; + public create3(arg: () => { new(): T }): T; + public create4(arg: Function): T; } -export declare class SomethingBar extends Something {} +export declare class SomethingBar extends Something { } + +export declare class Bar { + constructor(arg: { new(): T }); +} -- Gitee