refactor(autofix): Refactor autofix definition, add abstraction before creating changes by RandomByte · Pull Request #662 · UI5/linter

RandomByte · 2025-05-19T07:45:41Z

FixHints are still present but should be completely replaced by the Fix class and its subclasses and the generateChanges module.

Concept

Traverse the Transpiled-AST, detect problems and if possible, attempt to create fixes for them
Traverse Source-AST and
a) Collect all module declarations/requires
b) Provide fixes with corresponding nodes of the Source-AST
For all fixes, request the ranges (start and end positions) of their planned modifications (replace, delete or insert operations)
Compare modification ranges and identify conflicts
a) In case of conflicts, discard the whole fix associated with the conflicting range. Prefer smaller range fixes
For all remaining fixes:
a) Request a list of modules they require, the positions where they want to access them and optionally a preferred name for the identifier (all together referred to as "import requests")
b) Request a list of modules they no longer require, together with the identifier currently used to access those
Sort the import requests into the module declarations/requires collected in step 2 and determine the corresponding identifiers
For all remaining fixes, request the final source code changes (replace, delete or insert), providing them with the identifiers determined in step 6

Fixes, Improvements and Changes

See my review comments below for fixes, improvements and changes

RandomByte · 2025-06-10T14:31:21Z

+    	], function(Input, TableTable) {␊
    		const input = new Input();␊
-    		const Button = new SapMButton2();␊
+    		const Button = new SapMButton1();␊


Improvement: Existing import form surrounding sap.ui.define is now reused.

RandomByte · 2025-06-10T14:32:07Z

    	var isPhone = Device.system.phone;␊
    	var isTablet = Device.system.tablet;␊
-    	var isAndroidPhone = Device.os.android && Device.system.tablet;␊
+    	var isAndroidPhone = Device.os.android && Device.system.phone;␊


Fix: This is the correct replacement for jQuery.device.is.android_phone

RandomByte · 2025-06-10T14:32:56Z

    	var isAndroidTablet = Device.os.android && Device.system.tablet;␊
    	var isIPhone = Device.os.ios && Device.system.phone;␊
-    	var isIPad = Device.os.ios && Device.system.ipad;␊
+    	var isIPad = Device.os.ios && Device.system.tablet;␊


Fix: This is the correct replacement for jQuery.device.is.ipad. There is no Device.system.ipad property: https://sapui5.hana.ondemand.com/sdk/#/api/sap.ui.Device.system%23properties

RandomByte · 2025-06-10T14:33:36Z

    `sap.ui.define(["sap/ui/thirdparty/jquery"], async function (jQuery) {␊
    	// https://github.com/SAP/ui5-linter/issues/590␊
    ␊
-    	var isStandAlone = window.navigator.standalone;␊


Improvement: window or rather globalThis is only used when needed (i.e. if an identifier with name navigator already exists)

RandomByte · 2025-06-10T14:33:49Z

    	var intervalCallId3 = jQuery.sap.intervalCall(1000, window, Array.isArray, ["myParam1"]);␊
    	jQuery.sap.clearIntervalCall(intervalCallId);␊
    ␊
-    	var element = window.document.getElementById("popup");␊


Improvement: See above

RandomByte · 2025-06-10T14:34:31Z

    ␊
    	var person = {firstname: "Peter", lastname: "Miller" };␊
-    	var newObj = Object.create(person || null);␊
+    	var newObj = Object.create(person);␊


Improvement: Autofix can now evaluate the type of identifiers, correctly detecting the object here.

RandomByte · 2025-06-10T14:34:51Z

    	var myDetails = "These are the details";␊
    	var myComponent = "jquery.sap.logger.jsunit";␊
-    	Log.debug(myLogMessage, myDetails, myComponent, function () { });␊
+    	Log.debug(myLogMessage, myDetails, myComponent, function () {});␊


Improvement: Whitespaces in parameters are preserved

RandomByte · 2025-06-10T14:37:46Z

-    	var textUpperCase2 = jQuery.sap.charToUpperCase(xy); // Do not migrate vars. We don't know the type of xy␊
-    	var textUpperCase2 = jQuery.sap.charToUpperCase(xy, 0); // Do not migrate vars. We don't know the type of xy␊
-    	var textUpperCase2 = jQuery.sap.charToUpperCase(xy, 3); // // Do not migrate vars. We don't know the type of xy␊
+    	var textUpperCase2 = capitalize(xy);␊


Improvement: Type and value of identifiers is now checked

RandomByte · 2025-06-10T14:39:24Z

    	ObjectPath.set("name.lastname", "Miller", { name: { firstname: "me" } });␊
    	ObjectPath.set("name.lastname", "Miller", { name: { firstname: "me" } });␊
    	ObjectPath.set("", "Miller", { name: { firstname: "me" } });␊
-    	ObjectPath.set(undefined, "Miller", { name: { firstname: "me" } });␊


⚠️ CHANGE: If the first argument is explicitly set to undefined or null, replace it with an empty string.

I'm not sure why the first iteration kept the undefined (but not null). My understanding is that jQuery.sap.setObject always defaults falsy values to an empty string: https://github.com/SAP/openui5/blob/aad2c54fa60d58f681840a954395a08bfb96cb6c/src/sap.ui.core/src/jquery.sap.global.js#L898

ObjectPath.set(undefined, "Miller", { name: { firstname: "me" } }); will definitely result in an exception due to the missing split function on undefined: https://github.com/SAP/openui5/blob/7ac020cdf6e5fa56a96d17aadeec7a0159beff14/src/sap.ui.core/src/sap/base/util/ObjectPath.js#L37

RandomByte · 2025-06-10T14:51:09Z

    	// 2. Replace the code with the following:␊
    	// -> var store = new Storage(Storage.Type.local, "mystore");␊
-    	var store = new Storage(jQuery.sap.storage.Type.local, "mystore");␊
+    	var store = new Storage(Storage.Type.local, "mystore");␊


Improvement: Simultaneous fixes in a call expression's property access and its arguments are now possible.

RandomByte · 2025-06-10T14:55:08Z


-> AutofixResult: /jQuerySapExtendNoFix.js
-
-    `sap.ui.define(["sap/base/util/merge"], function(merge) {␊


Improvement: Import is not added if fixes are not applied

RandomByte · 2025-06-10T15:02:39Z

    	Theming.setTheme("customTheme"); // Can be autofixed when the 2nd argument is undefined␊
    ␊
    	Core.ready(function() {console.log();});␊
-    	Core.ready(function() {console.log();});␊


Fix: Duplicate fixes resolved

RandomByte · 2025-06-10T16:42:51Z

Test failure is due to code coverage only. Will try to improve it 👍

…e creating changes

…back

d3xter666

LGTM

…l identifier

RandomByte mentioned this pull request May 28, 2025

refactor: Introduce Ui5TypeInfo #672

Merged

RandomByte force-pushed the refactor-autofix branch 6 times, most recently from 9a61882 to 27e5265 Compare June 6, 2025 12:42

d3xter666 mentioned this pull request Jun 9, 2025

feat: Autofix Core API (complex cases) #675

Merged

RandomByte commented Jun 10, 2025

View reviewed changes

RandomByte marked this pull request as ready for review June 10, 2025 16:38

RandomByte requested a review from a team June 10, 2025 16:47

RandomByte added 13 commits June 11, 2025 09:36

refactor(autofix): Refactor autofix definition, add abstraction befor…

8406afc

…e creating changes

refactor(autofix): Apply feedback from discussion round

058948c

refactor(autofix): Rename FixScope

79199c4

refactor(autofix): Move GlobalFix validation into class

ebb740a

refactor(autofix): Cleanup

f213aee

refactor(autofix): Use correct position of module declaration body

c615f6a

refactor(autofix): Fix handling for existing dependencies w/o argument

9061023

refactor(autofix): Do not fix access expressions in assignments

a608e35

refactor(autofix): Re-enable remaining jQuery fixes

ebd6db7

refactor(autofix): Remove obsolete modules, cleanup

421781c

refactor(autofix): Minor cleanup

105ec58

refactor(autofix): Also fix jQuery.sap.setObject with no arguments

c9e71be

refactor(autofix): Fix startsWith string length calc for identifiers

1d0746c

RandomByte force-pushed the refactor-autofix branch from 4f453a9 to 1d0746c Compare June 11, 2025 07:36

refactor(autofix): Ignore module declarations without factory or call…

131293d

…back

d3xter666 previously approved these changes Jun 11, 2025

View reviewed changes

Comment thread src/linter/ui5Types/Ui5TypeInfoMatcher.ts Outdated

Comment thread src/linter/ui5Types/Ui5TypeInfoMatcher.ts Outdated

refactor(autofix): Minor renaming

bcb3164

RandomByte dismissed d3xter666’s stale review via bcb3164 June 11, 2025 10:28

test(autofix): Add test for sap.ui.require without callback

0144fe1

d3xter666 previously approved these changes Jun 11, 2025

View reviewed changes

RandomByte added 2 commits June 11, 2025 12:48

test(autofix): Improve error logging, add test for already used globa…

d6676df

…l identifier

test(Ui5TypeInfoMatcher): Add tests

5c19bfe

RandomByte dismissed d3xter666’s stale review via 5c19bfe June 11, 2025 11:16

style: Fix ESLlint problem

cc0472c

d3xter666 approved these changes Jun 11, 2025

View reviewed changes

RandomByte merged commit 894cb84 into main Jun 11, 2025
22 checks passed

RandomByte deleted the refactor-autofix branch June 11, 2025 13:23


		> AutofixResult: /jQuerySapExtendNoFix.js

		`sap.ui.define(["sap/base/util/merge"], function(merge) {␊

Uh oh!

Conversation

RandomByte commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Concept

Fixes, Improvements and Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RandomByte commented Jun 10, 2025

Uh oh!

d3xter666 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

RandomByte commented May 19, 2025 •

edited

Loading