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

fix: Handle relative imports within framework libs #505

Merged
merged 1 commit into from
Feb 14, 2025
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
47 changes: 35 additions & 12 deletions src/linter/ui5Types/SourceFileLinter.ts
Original file line number Diff line number Diff line change
@@ -13,6 +13,7 @@ import {
getPropertyAssignmentsInObjectLiteralExpression,
findClassMember,
isClassMethod,
findAmbientModuleByDeclarationName,
} from "./utils.js";
import {taskStart} from "../../utils/perf.js";
import {getPositionsForNode} from "../../utils/nodePosition.js";
@@ -1439,11 +1440,13 @@ export default class SourceFileLinter {
}

const extractVarName = (node: ts.PropertyAccessExpression | ts.ElementAccessExpression) => {
const nodeName = ts.isPropertyAccessExpression(node) ?
node.name.text :
node.argumentExpression.getText();

return nodeName.replaceAll("\"", "");
if (ts.isPropertyAccessExpression(node)) {
return node.name.text;
} else if (ts.isStringLiteralLike(node.argumentExpression)) {
return node.argumentExpression.text;
} else {
return node.argumentExpression.getText();
}
};

let exprNode = node.expression;
@@ -1453,13 +1456,35 @@ export default class SourceFileLinter {
namespace.unshift(extractVarName(exprNode));
exprNode = exprNode.expression;
}
const exprType = this.checker.getTypeAtLocation(exprNode);
const potentialLibImport = exprType.symbol?.getName().replaceAll("\"", "") ?? "";
const exprTypeSymbol = this.checker.getTypeAtLocation(exprNode)?.symbol;
let potentialLibImport = "";
if (exprTypeSymbol?.valueDeclaration && ts.isModuleDeclaration(exprTypeSymbol.valueDeclaration)) {
potentialLibImport = exprTypeSymbol.valueDeclaration.name.text;
}

// Checks if the left hand side is a library import.
// It's sufficient just to check for "/library" at the end of the string by convention
if (!potentialLibImport.endsWith("/library")) {
return;
// Fallback in case of relative imports within framework libraries, where the type does not have a symbol
const exprSymbol = this.checker.getSymbolAtLocation(exprNode);
const exprDeclaration = exprSymbol?.declarations?.[0];
if (!exprDeclaration) {
return;
}
if (!ts.isImportDeclaration(exprDeclaration.parent)) {
return;
}
const moduleSpecifier = exprDeclaration.parent.moduleSpecifier;
const moduleSpecifierSymbol = this.checker.getSymbolAtLocation(moduleSpecifier);
const moduleName = moduleSpecifierSymbol?.valueDeclaration?.getSourceFile().fileName;
if (
!moduleName?.startsWith("/resources/") ||
!(moduleName?.endsWith("/library.js") || moduleName?.endsWith("/library.ts"))
) {
return;
}
// Cut off "/resources/" (11 chars) and the ".(js|ts)" extension (3 chars)
potentialLibImport = moduleName.slice(11, -3);
}

const varName = extractVarName(node);
@@ -1471,10 +1496,8 @@ export default class SourceFileLinter {

// Check if the module is registered within ambient modules
const ambientModules = this.checker.getAmbientModules();
const libAmbientModule = ambientModules.find((module) =>
module.name === `"${potentialLibImport}"`);
const isRegisteredAsUi5Module = ambientModules.some((module) =>
module.name === `"${moduleName}"`);
const libAmbientModule = findAmbientModuleByDeclarationName(ambientModules, potentialLibImport);
const isRegisteredAsUi5Module = !!findAmbientModuleByDeclarationName(ambientModules, moduleName);

let isModuleExported = true;
let libExports = libAmbientModule?.exports ?? new Map();
11 changes: 11 additions & 0 deletions src/linter/ui5Types/utils.ts
Original file line number Diff line number Diff line change
@@ -176,3 +176,14 @@ export function getPropertyAssignmentsInObjectLiteralExpression(
}
return properties;
}

export function findAmbientModuleByDeclarationName(
ambientModules: ts.Symbol[], moduleName: string
): ts.Symbol | undefined {
return ambientModules.find((symbol) => {
if (!symbol.valueDeclaration || !ts.isModuleDeclaration(symbol.valueDeclaration)) {
return false;
}
return symbol.valueDeclaration.name.text === moduleName;
});
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
sap.ui.define([
// This file covers the case where a framework library contains a relative import
// to a library module from which an implicit global is accessed.
"./library"
], function (Library) {
"use strict";

var ColorPickerMode = Library['ColorPickerMode'],

// This is a special case as ColorPickerDisplayMode is defined in its own module but also exported via the library module.
// However, this export from the library module is not reflected in the types / api.json, so it is still reported as implicit global usage.
ColorPickerDisplayMode = Library['ColorPickerDisplayMode'];
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*!
* ${copyright}
*/

/**
* Initialization Code and shared classes of library sap.ui.unified.
*/
sap.ui.define(
[
"sap/ui/core/Lib",
"sap/ui/unified/ColorPickerDisplayMode",
],
function (
Library,
ColorPickerDisplayMode,
) {
"use strict";

/**
* Unified controls intended for both, mobile and desktop scenarios
*
* @namespace
* @alias sap.ui.unified
* @author SAP SE
* @version ${version}
* @since 1.28
* @public
*/
var thisLib = Library.init({
name: "sap.ui.unified",
apiVersion: 2,
version: "${version}",
dependencies: ["sap.ui.core"],
designtime: "sap/ui/unified/designtime/library.designtime",
types: [
"sap.ui.unified.ColorPickerDisplayMode",
],
});

// expose imported enum as property of library namespace, for documentation see ColorPickerDisplayMode.js
thisLib.ColorPickerDisplayMode = ColorPickerDisplayMode;

return thisLib;
}
);
5 changes: 3 additions & 2 deletions test/fixtures/linter/rules/NoGlobals/NoExportedLibValues.js
Original file line number Diff line number Diff line change
@@ -5,10 +5,11 @@ sap.ui.define(

var CalendarDayType = unifiedLibrary.CalendarDayType,
DateRange = unifiedLibrary["DateRange"],
DateRange2 = unifiedLibrary['DateRange'],
DateTypeRange = unifiedLibrary.DateTypeRange,
Copy link
Member

Choose a reason for hiding this comment

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

should the previous line also be duplicated with single quotes, unifiedLibrary['DateRange']?

Copy link
Member

Choose a reason for hiding this comment

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

I changed some cases below to single quotes to have a better coverage of both cases. But yeah, for the single-level access we don't have a single-quote fixture right now, that makes sense.

DOMAttribute = coreLibrary.tmpl.DOMAttribute,
DOMAttribute2 = coreLibrary["tmpl"].DOMAttribute,
DOMAttribute3 = coreLibrary["tmpl"]["DOMAttribute"],
DOMAttribute4 = coreLibrary.tmpl["DOMAttribute"];
DOMAttribute3 = coreLibrary['tmpl']["DOMAttribute"],
DOMAttribute4 = coreLibrary.tmpl['DOMAttribute'];
}
);
11 changes: 11 additions & 0 deletions test/lib/linter/linter.ts
Original file line number Diff line number Diff line change
@@ -232,6 +232,17 @@ test.serial("lint: All files of library with sap.ui.suite namespace", async (t)
t.snapshot(preprocessLintResultsForSnapshot(res));
});

test.serial("lint: All files of library with sap.ui.unified namespace", async (t) => {
const projectPath = path.join(fixturesProjectsPath, "sap.ui.unified");
const {lintProject} = t.context;

const res = await lintProject({
rootDir: projectPath,
}, t.context.sharedLanguageService);

t.snapshot(preprocessLintResultsForSnapshot(res));
});

test.serial("lint: All files of com.ui5.troublesome.app with custom config", async (t) => {
const projectPath = path.join(fixturesProjectsPath, "com.ui5.troublesome.app");
const {lintProject} = t.context;
20 changes: 14 additions & 6 deletions test/lib/linter/rules/snapshots/NoGlobals.ts.md
Original file line number Diff line number Diff line change
@@ -388,7 +388,7 @@ Generated by [AVA](https://avajs.dev).
[
{
coverageInfo: [],
errorCount: 6,
errorCount: 7,
fatalErrorCount: 0,
filePath: 'NoExportedLibValues.js',
messages: [
@@ -401,40 +401,48 @@ Generated by [AVA](https://avajs.dev).
severity: 2,
},
{
column: 35,
column: 32,
line: 8,
message: 'Access of module \'sap/ui/unified/DateRange\' (unifiedLibrary.DateRange) not exported by library \'sap/ui/unified/library\'',
messageDetails: 'Please import the module itself directly instead of accessing it via the library module.',
ruleId: 'no-implicit-globals',
severity: 2,
},
{
column: 35,
line: 9,
message: 'Access of module \'sap/ui/unified/DateTypeRange\' (unifiedLibrary.DateTypeRange) not exported by library \'sap/ui/unified/library\'',
messageDetails: 'Please import the module itself directly instead of accessing it via the library module.',
ruleId: 'no-implicit-globals',
severity: 2,
},
{
column: 36,
line: 9,
line: 10,
message: 'Access of module \'sap/ui/core/tmpl/DOMAttribute\' (coreLibrary.tmpl.DOMAttribute) not exported by library \'sap/ui/core/library\'',
messageDetails: 'Please import the module itself directly instead of accessing it via the library module.',
ruleId: 'no-implicit-globals',
severity: 2,
},
{
column: 40,
line: 10,
line: 11,
message: 'Access of module \'sap/ui/core/tmpl/DOMAttribute\' (coreLibrary.tmpl.DOMAttribute) not exported by library \'sap/ui/core/library\'',
messageDetails: 'Please import the module itself directly instead of accessing it via the library module.',
ruleId: 'no-implicit-globals',
severity: 2,
},
{
column: 40,
line: 11,
line: 12,
message: 'Access of module \'sap/ui/core/tmpl/DOMAttribute\' (coreLibrary.tmpl.DOMAttribute) not exported by library \'sap/ui/core/library\'',
messageDetails: 'Please import the module itself directly instead of accessing it via the library module.',
ruleId: 'no-implicit-globals',
severity: 2,
},
{
column: 37,
line: 12,
line: 13,
message: 'Access of module \'sap/ui/core/tmpl/DOMAttribute\' (coreLibrary.tmpl.DOMAttribute) not exported by library \'sap/ui/core/library\'',
messageDetails: 'Please import the module itself directly instead of accessing it via the library module.',
ruleId: 'no-implicit-globals',
Binary file modified test/lib/linter/rules/snapshots/NoGlobals.ts.snap
Binary file not shown.
23 changes: 23 additions & 0 deletions test/lib/linter/snapshots/linter.ts.md
Original file line number Diff line number Diff line change
@@ -3055,6 +3055,29 @@ Generated by [AVA](https://avajs.dev).
},
]

## lint: All files of library with sap.ui.unified namespace

> Snapshot 1

[
{
coverageInfo: [],
errorCount: 1,
fatalErrorCount: 0,
filePath: 'src/sap/ui/unified/ColorPicker.js',
messages: [
{
column: 36,
line: 12,
message: 'Access of module \'sap/ui/unified/ColorPickerDisplayMode\' (Library.ColorPickerDisplayMode) not exported by library \'sap/ui/unified/library\'',
ruleId: 'no-implicit-globals',
severity: 2,
},
],
warningCount: 0,
},
]

## lint: All files of com.ui5.troublesome.app with custom config

> Snapshot 1
Binary file modified test/lib/linter/snapshots/linter.ts.snap
Binary file not shown.