Skip to content

Commit 9e9f18b

Browse files
RandomByteRandomByte
committed
refactor(autofix): Add validation for SimpleForm#minWidth property fix
Only if we detect that a layout other than ResponsiveLayout is used, we can safely remove the minWidth property. If we detect that no layout is set and therefore the default layout "ResponsiveGridLayout" is used, we can also apply this fix. If a binding is used to set the layout, no autofix should be applied. JIRA: CPOUI5FOUNDATION-994
1 parent 32e0b36 commit 9e9f18b

File tree

10 files changed

+270
-67
lines changed

10 files changed

+270
-67
lines changed

‎src/linter/ui5Types/fix/AccessExpressionGeneratorFix.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ export default class AccessExpressionGeneratorFix extends AccessExpressionBaseFi
106106

107107
// If a generator function is provided, use it to generate the change
108108
const value = this.params.generator(identifiers);
109-
if (!value) {
109+
if (value === undefined) {
110110
return;
111111
}
112112
return {

‎src/linter/ui5Types/fix/CallExpressionGeneratorFix.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,7 @@ export default class CallExpressionGeneratorFix<GeneratorContext extends object>
161161

162162
const value = this.params.generator(
163163
this.generatorContext, identifiers, ...this.generatorArgs);
164-
if (!value) {
164+
if (value === undefined) {
165165
return;
166166
}
167167
return {

‎src/linter/ui5Types/fix/FixFactory.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,9 @@ import getGlobalFixInfo, {GlobalFixInfo} from "./getGlobalFixInfo.js";
1010
import CallExpressionGeneratorFix, {CallExpressionGeneratorFixParams} from "./CallExpressionGeneratorFix.js";
1111
import AccessExpressionGeneratorFix, {AccessExpressionGeneratorFixParams} from "./AccessExpressionGeneratorFix.js";
1212
import PropertyAssignmentFix, {PropertyAssignmentFixParams} from "./PropertyAssignmentFix.js";
13+
import PropertyAssignmentGeneratorFix, {
14+
PropertyAssignmentGeneratorFixParams,
15+
} from "./PropertyAssignmentGeneratorFix.js";
1316

1417
const AUTOFIX_COLLECTIONS = [
1518
"sapUiCoreFixes",
@@ -122,3 +125,9 @@ export function callExpressionGeneratorFix<GeneratorContext extends object = obj
122125
export function propertyAssignmentFix(params: PropertyAssignmentFixParams): () => PropertyAssignmentFix {
123126
return () => new PropertyAssignmentFix(params);
124127
}
128+
129+
export function propertyAssignmentGeneratorFix<GeneratorContext extends object = object>(
130+
params: PropertyAssignmentGeneratorFixParams<GeneratorContext>
131+
): () => PropertyAssignmentGeneratorFix<GeneratorContext> {
132+
return () => new PropertyAssignmentGeneratorFix(params);
133+
}

‎src/linter/ui5Types/fix/PropertyAssignmentBaseFix.ts‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
import ts from "typescript";
2+
import {ChangeSet} from "../../../autofix/autofix.js";
3+
import {PositionInfo} from "../../LinterContext.js";
4+
import {Attribute, Position, SaxEventType} from "sax-wasm";
5+
import XmlEnabledFix from "./XmlEnabledFix.js";
6+
import {FixHelpers} from "./Fix.js";
7+
8+
/**
9+
* Fix a global property access. Requires a module name which will be imported and replaces the defined property access.
10+
* The property access is in the order of the AST, e.g. ["core", "ui", "sap"]
11+
*/
12+
export default abstract class PropertyAssignmentBaseFix extends XmlEnabledFix {
13+
protected sourcePosition: PositionInfo | undefined;
14+
protected startPos: number | undefined;
15+
protected endPos: number | undefined;
16+
17+
constructor() {
18+
super();
19+
}
20+
21+
visitLinterNode(node: ts.Node, sourcePosition: PositionInfo, _helpers: FixHelpers) {
22+
if (!ts.isPropertyAssignment(node) || (!ts.isIdentifier(node.name) && !ts.isStringLiteralLike(node.name))) {
23+
return false;
24+
}
25+
26+
this.sourcePosition = sourcePosition;
27+
return true;
28+
}
29+
30+
getNodeSearchParameters() {
31+
if (this.sourcePosition === undefined) {
32+
throw new Error("Position for search is not defined");
33+
}
34+
return {
35+
nodeTypes: [ts.SyntaxKind.PropertyAssignment],
36+
xmlEventTypes: [SaxEventType.Attribute],
37+
position: this.sourcePosition,
38+
};
39+
}
40+
41+
visitAutofixNode(node: ts.Node, _position: number, _sourceFile: ts.SourceFile) {
42+
if (!ts.isPropertyAssignment(node)) {
43+
return false;
44+
}
45+
this.startPos = node.getStart();
46+
this.endPos = node.getEnd();
47+
return true;
48+
}
49+
50+
visitAutofixXmlNode(node: Attribute, toPosition: (pos: Position) => number) {
51+
this.startPos = toPosition(node.name.start);
52+
this.endPos = toPosition(node.value.end) + 1; // TODO: +1 might be incorrect if no quotes are used
53+
return true;
54+
}
55+
56+
getAffectedSourceCodeRange() {
57+
if (this.startPos === undefined || this.endPos === undefined) {
58+
throw new Error("Start and end position are not defined");
59+
}
60+
return {
61+
start: this.startPos,
62+
end: this.endPos,
63+
};
64+
}
65+
66+
getNewModuleDependencies() {
67+
return undefined;
68+
}
69+
70+
setIdentifierForDependency() {
71+
return;
72+
}
73+
74+
getNewGlobalAccess() {
75+
return undefined;
76+
}
77+
78+
setIdentifierForGlobal() {
79+
return;
80+
}
81+
82+
generateChanges(): ChangeSet | ChangeSet[] | undefined {
83+
throw new Error("Method 'generateChanges' must be implemented in subclasses");
84+
}
85+
}

‎src/linter/ui5Types/fix/PropertyAssignmentFix.ts‎

Lines changed: 10 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,7 @@
11
import ts from "typescript";
22
import {ChangeAction, ChangeSet} from "../../../autofix/autofix.js";
3-
import {PositionInfo} from "../../LinterContext.js";
4-
import {Attribute, Position, SaxEventType} from "sax-wasm";
5-
import XmlEnabledFix from "./XmlEnabledFix.js";
3+
import {Attribute, Position} from "sax-wasm";
4+
import PropertyAssignmentBaseFix from "./PropertyAssignmentBaseFix.js";
65

76
export interface PropertyAssignmentFixParams {
87
/**
@@ -16,86 +15,38 @@ export interface PropertyAssignmentFixParams {
1615
* Fix a global property access. Requires a module name which will be imported and replaces the defined property access.
1716
* The property access is in the order of the AST, e.g. ["core", "ui", "sap"]
1817
*/
19-
export default class PropertyAssignmentFix extends XmlEnabledFix {
20-
private sourcePosition: PositionInfo | undefined;
21-
private startPos: number | undefined;
22-
private endPos: number | undefined;
23-
18+
export default class PropertyAssignmentFix extends PropertyAssignmentBaseFix {
2419
constructor(private params: PropertyAssignmentFixParams) {
2520
super();
2621
}
2722

28-
visitLinterNode(node: ts.Node, sourcePosition: PositionInfo) {
29-
if (!ts.isPropertyAssignment(node) || (!ts.isIdentifier(node.name) && !ts.isStringLiteralLike(node.name))) {
23+
visitAutofixNode(node: ts.Node, position: number, sourceFile: ts.SourceFile) {
24+
if (!super.visitAutofixNode(node, position, sourceFile)) {
3025
return false;
3126
}
32-
33-
this.sourcePosition = sourcePosition;
34-
return true;
35-
}
36-
37-
getNodeSearchParameters() {
38-
if (this.sourcePosition === undefined) {
39-
throw new Error("Position for search is not defined");
40-
}
41-
return {
42-
nodeTypes: [ts.SyntaxKind.PropertyAssignment],
43-
xmlEventTypes: [SaxEventType.Attribute],
44-
position: this.sourcePosition,
45-
};
46-
}
47-
48-
visitAutofixNode(node: ts.Node, position: number, sourceFile: ts.SourceFile) {
4927
if (!ts.isPropertyAssignment(node)) {
5028
return false;
5129
}
5230
if (this.params.property) {
5331
// Only replace the property name, not the whole assignment
5432
node = node.name;
33+
this.startPos = node.getStart();
34+
this.endPos = node.getEnd();
5535
}
56-
this.startPos = node.getStart(sourceFile);
57-
this.endPos = node.getEnd();
5836
return true;
5937
}
6038

6139
visitAutofixXmlNode(node: Attribute, toPosition: (pos: Position) => number) {
62-
this.startPos = toPosition(node.name.start);
40+
if (!super.visitAutofixXmlNode(node, toPosition)) {
41+
return false;
42+
}
6343
if (this.params.property) {
6444
// Only replace the property name, not the whole assignment
6545
this.endPos = toPosition(node.name.end);
66-
} else {
67-
// Replace the whole assignment
68-
this.endPos = toPosition(node.value.end) + 1; // TODO: +1 might be incorrect if no quotes are used
6946
}
7047
return true;
7148
}
7249

73-
getAffectedSourceCodeRange() {
74-
if (this.startPos === undefined || this.endPos === undefined) {
75-
throw new Error("Start and end position are not defined");
76-
}
77-
return {
78-
start: this.startPos,
79-
end: this.endPos,
80-
};
81-
}
82-
83-
getNewModuleDependencies() {
84-
return undefined;
85-
}
86-
87-
setIdentifierForDependency() {
88-
return;
89-
}
90-
91-
getNewGlobalAccess() {
92-
return undefined;
93-
}
94-
95-
setIdentifierForGlobal() {
96-
return;
97-
}
98-
9950
generateChanges(): ChangeSet {
10051
if (this.startPos === undefined || this.endPos === undefined) {
10152
throw new Error("Start and end position are not defined");

‎src/linter/ui5Types/fix/PropertyAssignmentGeneratorFix.ts‎

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
import ts from "typescript";
2+
import {ChangeAction, ChangeSet} from "../../../autofix/autofix.js";
3+
import {PositionInfo} from "../../LinterContext.js";
4+
import {FixHelpers} from "./Fix.js";
5+
import PropertyAssignmentBaseFix from "./PropertyAssignmentBaseFix.js";
6+
import {Attribute, Position} from "sax-wasm";
7+
8+
export interface PropertyAssignmentGeneratorFixParams<GeneratorContext extends object> {
9+
/**
10+
* Validate the property assignment, optionally using the checker provided in the fix helpers.
11+
*
12+
* This hook may also collect information that affects the generator. For that, it can store
13+
* information in the provided context object, which can be retrieved later in the generator function
14+
*/
15+
validatePropertyAssignment?: (
16+
ctx: GeneratorContext, helpers: FixHelpers, propertyAssignment: ts.PropertyAssignment
17+
) => boolean;
18+
19+
/**
20+
* The generator function will be used to determine the value of the replacement, affecting
21+
* the whole call expression
22+
*
23+
* If the return value is undefined, no change will be generated.
24+
*
25+
* @param ctx - The context object that can be used to store information between validation and generation
26+
* @param propertyName -
27+
* The name of the property that is being accessed, as a string representation of the source code,
28+
* @param propertyInitializer -
29+
* The initializer of the property assignment, as a string representation of the source code.
30+
*/
31+
generator: (
32+
ctx: GeneratorContext, propertyName: string, propertyInitializer: string
33+
) => string | undefined;
34+
};
35+
36+
/**
37+
* Fix a global property access. Requires a module name which will be imported and replaces the defined property access.
38+
* The property access is in the order of the AST, e.g. ["core", "ui", "sap"]
39+
*/
40+
export default class PropertyAssignmentGeneratorFix<GeneratorContext extends object> extends PropertyAssignmentBaseFix {
41+
protected generatorContext = {} as GeneratorContext;
42+
protected propertyName: string | undefined;
43+
protected propertyInitializer: string | undefined;
44+
45+
constructor(private params: PropertyAssignmentGeneratorFixParams<GeneratorContext>) {
46+
super();
47+
}
48+
49+
visitLinterNode(node: ts.Node, sourcePosition: PositionInfo, helpers: FixHelpers) {
50+
if (!super.visitLinterNode(node, sourcePosition, helpers)) {
51+
return false;
52+
}
53+
if (!ts.isPropertyAssignment(node) || !ts.isIdentifier(node.name)) {
54+
return false;
55+
}
56+
if (this.params.validatePropertyAssignment) {
57+
if (!this.params.validatePropertyAssignment(this.generatorContext, helpers, node)) {
58+
return false;
59+
}
60+
}
61+
62+
this.sourcePosition = sourcePosition;
63+
return true;
64+
}
65+
66+
visitAutofixNode(node: ts.Node, position: number, sourceFile: ts.SourceFile) {
67+
if (!super.visitAutofixNode(node, position, sourceFile)) {
68+
return false;
69+
}
70+
if (!ts.isPropertyAssignment(node)) {
71+
return false;
72+
}
73+
74+
this.propertyName = node.name.getFullText();
75+
this.propertyInitializer = node.initializer.getFullText();
76+
77+
return true;
78+
}
79+
80+
visitAutofixXmlNode(node: Attribute, toPosition: (pos: Position) => number) {
81+
if (!super.visitAutofixXmlNode(node, toPosition)) {
82+
return false;
83+
}
84+
this.propertyName = node.name.value;
85+
this.propertyInitializer = node.value.value;
86+
return true;
87+
}
88+
89+
generateChanges(): ChangeSet | ChangeSet[] | undefined {
90+
if (this.startPos === undefined || this.endPos === undefined) {
91+
throw new Error("Start and end position are not defined");
92+
}
93+
if (this.propertyName === undefined || this.propertyInitializer === undefined) {
94+
throw new Error("Property name or initializer is not defined");
95+
}
96+
const value = this.params.generator(this.generatorContext, this.propertyName, this.propertyInitializer);
97+
if (value === undefined) {
98+
return;
99+
}
100+
return {
101+
action: ChangeAction.REPLACE,
102+
start: this.startPos,
103+
end: this.endPos,
104+
value,
105+
};
106+
}
107+
}

‎src/linter/ui5Types/fix/collections/sapUiLayoutFixes.ts‎

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,38 @@
1+
import ts from "typescript";
12
import Ui5TypeInfoMatcher from "../../Ui5TypeInfoMatcher.js";
23
import {
34
FixTypeInfoMatcher,
4-
propertyAssignmentFix,
5+
propertyAssignmentGeneratorFix,
56
} from "../FixFactory.js";
7+
import {getPropertyAssignmentInObjectLiteralExpression} from "../../utils/utils.js";
68

79
const t: FixTypeInfoMatcher = new Ui5TypeInfoMatcher("sap.ui.layout");
810
export default t;
911

1012
t.declareModule("sap/ui/layout/form/SimpleForm", [
1113
t.managedObjectSetting("$SimpleFormSettings", [
1214
// Property "minWidth" is deprecated and should be removed
13-
t.metadataProperty("minWidth", propertyAssignmentFix({})),
15+
t.metadataProperty("minWidth", propertyAssignmentGeneratorFix({
16+
validatePropertyAssignment: (context, helpers, node) => {
17+
// Validate that the minWidth property is being used
18+
if (!ts.isObjectLiteralExpression(node.parent)) {
19+
return false;
20+
}
21+
const layoutProp = getPropertyAssignmentInObjectLiteralExpression("layout", node.parent);
22+
if (layoutProp && ts.isStringLiteralLike(layoutProp.initializer)) {
23+
// We can safely remove minWidth if
24+
// the layout property is either NOT SET or
25+
// the layout property is NOT SET to "ResponsiveLayout" AND is not a binding
26+
return layoutProp.initializer.text !== "ResponsiveLayout" &&
27+
!(layoutProp.initializer.text.startsWith("{") && layoutProp.initializer.text.endsWith("}"));
28+
}
29+
30+
return true;
31+
},
32+
generator: () => {
33+
// Remove the minWidth property
34+
return "";
35+
},
36+
})),
1437
]),
1538
]);

‎test/fixtures/autofix/DeprecatedApi.view.xml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,12 @@
1515
<form:SimpleForm minWidth="100px"> <!-- REMOVE: Property "minWidth" is deprecated -->
1616
</form:SimpleForm>
1717

18+
<form:SimpleForm minWidth="100px" layout="ResponsiveLayout"> <!-- KEEP: Property "minWidth" is deprecated but ResponsiveLayout is used, preventing a safe migration -->
19+
</form:SimpleForm>
20+
21+
<form:SimpleForm minWidth="100px" layout="{/layoutBinding}"> <!-- KEEP: Property "minWidth" is deprecated but the layout is determined dynamically, preventing a safe migration -->
22+
</form:SimpleForm>
23+
1824
<table:Table groupBy="some-column"> <!-- Association "groupBy" is deprecated -->
1925
<table:plugins> <!-- Aggregation "plugins" is deprecated -->
2026
<tablePlugins:MultiSelectionPlugin id="multi-selection-plugin" />

0 commit comments

Comments
 (0)