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

feat: Detect usage of non exported values by library.js #468

Merged
merged 25 commits into from
Jan 14, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
4de7e91
feat: Add new message type
d3xter666 Jan 9, 2025
595e0d3
feat: Detect exported values by library
d3xter666 Jan 9, 2025
5f1e58a
refactor: Use getter for the name
d3xter666 Jan 9, 2025
57b7956
test: Add test cases
d3xter666 Jan 9, 2025
c51cb02
fix: Breaking check
d3xter666 Jan 9, 2025
5b5f3db
test: Add more test cases
d3xter666 Jan 9, 2025
c11d7a8
refactor: Add support for PropertyAccessExpression-s
d3xter666 Jan 9, 2025
3cb4e5e
test: Update snapshots
d3xter666 Jan 9, 2025
68ee498
test: Fix test cases
d3xter666 Jan 9, 2025
0c2b254
refactor: Improve messaging
d3xter666 Jan 9, 2025
5207d26
fix: Allow ElementAccessExpressions
d3xter666 Jan 10, 2025
ff8910e
test: Add more test cases
d3xter666 Jan 10, 2025
3a5a668
docs: Update comments
d3xter666 Jan 10, 2025
8bb9d55
fix: Merge conflicts
d3xter666 Jan 10, 2025
82a2920
refactor: Remove redundant check
d3xter666 Jan 10, 2025
9a39a75
test: Remove redundant tests
d3xter666 Jan 10, 2025
ccf7380
fix: Properly detect non exported elements
d3xter666 Jan 10, 2025
46c7683
test: Update snapshots with the correct findings
d3xter666 Jan 10, 2025
b5dccb3
refactor: Exported values messaging
d3xter666 Jan 10, 2025
d6e3f0f
test: Update snapshots w/ the new messages
d3xter666 Jan 10, 2025
2cbb8f4
refactor: Restore the non exported negative examples
d3xter666 Jan 10, 2025
ef38758
refactor: Update messages
d3xter666 Jan 10, 2025
c6bde70
fix: Resolve nested namespaces
d3xter666 Jan 13, 2025
734d1fb
test: Update snapshots
d3xter666 Jan 13, 2025
3d050be
refactor: Move sample to the negative test
d3xter666 Jan 13, 2025
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
12 changes: 12 additions & 0 deletions src/linter/messages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const RULES = {
"no-deprecated-library": "no-deprecated-library",
"no-deprecated-theme": "no-deprecated-theme",
"no-globals": "no-globals",
"no-implicit-globals": "no-implicit-globals",
"no-pseudo-modules": "no-pseudo-modules",
"parsing-error": "parsing-error",
"ui5-class-declaration": "ui5-class-declaration",
Expand Down Expand Up @@ -53,6 +54,7 @@ export enum MESSAGE {
NO_DEPRECATED_RENDERER,
NO_DIRECT_DATATYPE_ACCESS,
NO_DIRECT_ENUM_ACCESS,
NO_EXPORTED_VALUES_BY_LIB,
NO_GLOBALS,
NO_ICON_POOL_RENDERER,
NOT_STATIC_CONTROL_RENDERER,
Expand Down Expand Up @@ -570,4 +572,14 @@ export const MESSAGE_INFO = {
details: ({messageDetails}: {messageDetails: string}) => messageDetails,
},

[MESSAGE.NO_EXPORTED_VALUES_BY_LIB]: {
severity: LintMessageSeverity.Error,
ruleId: RULES["no-implicit-globals"],

message: ({module, namespace, libraryName}: {module: string; namespace: string; libraryName: string}) =>
`Access of module '${module}' (${namespace}) not exported by library '${libraryName}'`,
details: () =>
`Please import the module itself directly instead of accessing it via the library module.`,
},

} as const;
72 changes: 72 additions & 0 deletions src/linter/ui5Types/SourceFileLinter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ export default class SourceFileLinter {
node as (ts.PropertyAccessExpression | ts.ElementAccessExpression)); // Check for global
this.analyzePropertyAccessExpressionForDeprecation(
node as (ts.PropertyAccessExpression | ts.ElementAccessExpression)); // Check for deprecation
this.analyzeExportedValuesByLib(node as (ts.PropertyAccessExpression | ts.ElementAccessExpression));
} else if (node.kind === ts.SyntaxKind.ObjectBindingPattern &&
node.parent?.kind === ts.SyntaxKind.VariableDeclaration) {
// e.g. `const { Button } = sap.m;`
Expand Down Expand Up @@ -1185,8 +1186,79 @@ export default class SourceFileLinter {
].includes(nodeType);
}

analyzeExportedValuesByLib(node: ts.PropertyAccessExpression | ts.ElementAccessExpression) {
if (!ts.isElementAccessExpression(node) &&
node.name?.kind !== ts.SyntaxKind.Identifier) {
return;
}

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

return nodeName.replaceAll("\"", "");
};

let exprNode = node.expression;
const namespace: string[] = [];
while (ts.isPropertyAccessExpression(exprNode) ||
ts.isElementAccessExpression(exprNode)) {
namespace.unshift(extractVarName(exprNode));
exprNode = exprNode.expression;
}
const exprType = this.checker.getTypeAtLocation(exprNode);
const potentialLibImport = exprType.symbol?.getName().replaceAll("\"", "") ?? "";

// 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;
}

const varName = extractVarName(node);
const moduleName = [
potentialLibImport.replace("/library", ""),
...namespace,
varName,
].join("/");

// 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}"`);

let isModuleExported = true;
let libExports = libAmbientModule?.exports ?? new Map();
for (const moduleChunk of [...namespace, varName]) {
if (!libExports.has(moduleChunk)) {
isModuleExported = false;
break;
}

const exportMap = libExports.get(moduleChunk) as ts.Symbol | undefined;
libExports = exportMap?.exports ?? new Map();
}

if (isRegisteredAsUi5Module && !isModuleExported) {
this.#reporter.addMessage(MESSAGE.NO_EXPORTED_VALUES_BY_LIB, {
module: moduleName,
namespace: [
exprNode?.getText(),
...namespace,
varName,
].join("."),
libraryName: potentialLibImport,
},
(ts.isPropertyAccessExpression(node) ? node.name : node.argumentExpression));
}
}

analyzePropertyAccessExpression(node: ts.AccessExpression | ts.CallExpression) {
const exprNode = node.expression;

if (ts.isIdentifier(exprNode)) {
// The expression being an identifier indicates that this is the first access
// in a possible chain. E.g. the "sap" in "sap.ui.getCore()"
Expand Down
14 changes: 14 additions & 0 deletions test/fixtures/linter/rules/NoGlobals/NoExportedLibValues.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
sap.ui.define(
["sap/ui/unified/library", "sap/ui/core/library"],
function (unifiedLibrary, coreLibrary) {
"use strict";

var CalendarDayType = unifiedLibrary.CalendarDayType,
DateRange = unifiedLibrary["DateRange"],
DateTypeRange = unifiedLibrary.DateTypeRange,
DOMAttribute = coreLibrary.tmpl.DOMAttribute,
DOMAttribute2 = coreLibrary["tmpl"].DOMAttribute,
DOMAttribute3 = coreLibrary["tmpl"]["DOMAttribute"],
DOMAttribute4 = coreLibrary.tmpl["DOMAttribute"];
}
);
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
sap.ui.define(
[
"sap/ui/unified/library",
"sap/ui/core/library",
"sap/ui/comp/library",
// This ones are correct- elements, imported as modules
"sap/ui/unified/DateTypeRange",
"sap/ui/unified/CalendarDayType",
Copy link
Member

Choose a reason for hiding this comment

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

This is actually a pseudo module (see finding in test snapshot), and it should rather be accessed via the library module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, and that's why I added it. With the prevous miss, actually the pseudo modules were also reported as non exported values. Let's leave this as a safeguard.

],
function (unifiedLibrary, coreLibrary, compLibrary) {
"use strict";

// These are data types that are actually exported by the library
var SelectOptionSign = compLibrary.smartfilterbar.SelectOptionSign;

var CalendarAppointmentHeight =
unifiedLibrary.CalendarAppointmentHeight,
CalendarAppointmentRoundWidth =
unifiedLibrary.CalendarAppointmentRoundWidth,
ColorPickerMode = unifiedLibrary.ColorPickerMode,
ContentSwitcherAnimation = unifiedLibrary.ContentSwitcherAnimation;
}
);
88 changes: 88 additions & 0 deletions test/lib/linter/rules/snapshots/NoGlobals.ts.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,94 @@ The actual snapshot is saved in `NoGlobals.ts.snap`.

Generated by [AVA](https://avajs.dev).

## General: NoExportedLibValues.js

> Snapshot 1

[
{
coverageInfo: [],
errorCount: 6,
fatalErrorCount: 0,
filePath: 'NoExportedLibValues.js',
messages: [
{
column: 31,
line: 7,
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: 8,
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,
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,
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,
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,
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,
},
],
warningCount: 0,
},
]

## General: NoExportedLibValues_Negative.js
d3xter666 marked this conversation as resolved.
Show resolved Hide resolved

> Snapshot 1

[
{
coverageInfo: [],
errorCount: 1,
fatalErrorCount: 0,
filePath: 'NoExportedLibValues_Negative.js',
messages: [
{
column: 3,
line: 8,
message: 'Deprecated access of enum pseudo module \'sap/ui/unified/CalendarDayType\'',
messageDetails: 'Migrating Access to Pseudo Modules (https://ui5.sap.com/#/topic/00737d6c1b864dc3ab72ef56611491c4)',
ruleId: 'no-pseudo-modules',
severity: 2,
},
],
warningCount: 0,
},
]

## General: NoGlobals.js

> Snapshot 1
Expand Down
Binary file modified test/lib/linter/rules/snapshots/NoGlobals.ts.snap
Binary file not shown.
Loading