fix: no-invalid-properties false positives for var() in functions (#227)

* fix: no-invalid-properties false positives for var() in functions

* use Function visitors to avoid double traversal

Author: thecalamiity

src/rules/no-invalid-properties.js

@@ -131,12 +131,16 @@ export default {
 		const vars = new Map();
 
 		/**
-		 * We need to track this as a stack because we can have nested
-		 * rules that use the `var()` function, and we need to
-		 * ensure that we validate the innermost rule first.
-		 * @type {Array<Map<string,FunctionNodePlain>>}
+		 * @type {Array<{
+		 *   valueParts: string[],
+		 *   functionPartsStack: string[][],
+		 *   valueSegmentLocs: Map<string,CssLocationRange>,
+		 *   skipValidation: boolean,
+		 *   hadVarSubstitution: boolean,
+		 *   resolvedCache:  Map<string,string>
+		 * }>}
 		 */
-		const replacements = [];
+		const declStack = [];
 
 		const [{ allowUnknownVariables }] = context.options;
 
@@ -245,26 +249,153 @@ export default {
 			return null;
 		}
 
+		/**
+		 * Process a var function node and add its resolved value to the value list
+		 * @param {Object} varNode The var() function node
+		 * @param {string[]} valueList Array to collect processed values
+		 * @param {Map<string,CssLocationRange>} valueSegmentLocs Map of rebuilt value segments to their locations
+		 * @param {Map<string, string>} resolvedCache Cache for resolved variable values to prevent redundant lookups
+		 * @returns {boolean} Whether processing was successful
+		 */
+		function processVarFunction(
+			varNode,
+			valueList,
+			valueSegmentLocs,
+			resolvedCache,
+		) {
+			const varValue = vars.get(varNode.children[0].name);
+
+			if (varValue) {
+				const resolvedValue = resolveVariable(
+					varNode.children[0].name,
+					resolvedCache,
+				);
+				if (resolvedValue) {
+					valueList.push(resolvedValue);
+					valueSegmentLocs.set(resolvedValue, varNode.loc);
+					return true;
+				}
+			}
+
+			// If the variable is not found and doesn't have a fallback value, report it
+			if (varNode.children.length === 1) {
+				if (!allowUnknownVariables) {
+					context.report({
+						loc: varNode.children[0].loc,
+						messageId: "unknownVar",
+						data: { var: varNode.children[0].name },
+					});
+					return false;
+				}
+				return true;
+			}
+
+			// Handle fallback values
+			if (varNode.children[2].type !== "Raw") {
+				return true;
+			}
+
+			const fallbackValue = varNode.children[2].value.trim();
+			const resolvedFallbackValue = resolveFallback(
+				fallbackValue,
+				resolvedCache,
+			);
+			if (resolvedFallbackValue) {
+				valueList.push(resolvedFallbackValue);
+				valueSegmentLocs.set(resolvedFallbackValue, varNode.loc);
+				return true;
+			}
+
+			// No valid fallback found
+			if (!allowUnknownVariables) {
+				context.report({
+					loc: varNode.children[0].loc,
+					messageId: "unknownVar",
+					data: { var: varNode.children[0].name },
+				});
+				return false;
+			}
+
+			return true;
+		}
+
 		return {
 			"Rule > Block > Declaration"() {
-				replacements.push(new Map());
+				declStack.push({
+					valueParts: [],
+					functionPartsStack: [],
+					valueSegmentLocs: new Map(),
+					skipValidation: false,
+					hadVarSubstitution: false,
+					/**
+					 * Cache for resolved variable values within this single declaration.
+					 * Prevents re-resolving the same variable and re-walking long `var()` chains.
+					 */
+					resolvedCache: new Map(),
+				});
 			},
 
-			"Function[name=/^var$/i]"(node) {
-				const map = replacements.at(-1);
-				if (!map) {
+			"Rule > Block > Declaration > Value > *:not(Function)"(node) {
+				const state = declStack.at(-1);
+				const text = sourceCode.getText(node).trim();
+				state.valueParts.push(text);
+				state.valueSegmentLocs.set(text, node.loc);
+			},
+
+			Function() {
+				declStack.at(-1).functionPartsStack.push([]);
+			},
+
+			"Function > *:not(Function)"(node) {
+				const state = declStack.at(-1);
+				const parts = state.functionPartsStack.at(-1);
+				const text = sourceCode.getText(node).trim();
+				parts.push(text);
+				state.valueSegmentLocs.set(text, node.loc);
+			},
+
+			"Function:exit"(node) {
+				const state = declStack.at(-1);
+				if (state.skipValidation) {
 					return;
 				}
 
-				/*
-				 * Store the custom property name and the function node
-				 * so can use these to validate the value later.
-				 */
-				const name = node.children[0].name;
-				map.set(name, node);
+				const parts = state.functionPartsStack.pop();
+				let result;
+				if (node.name.toLowerCase() === "var") {
+					const resolvedParts = [];
+					const success = processVarFunction(
+						node,
+						resolvedParts,
+						state.valueSegmentLocs,
+						state.resolvedCache,
+					);
+
+					if (!success) {
+						state.skipValidation = true;
+						return;
+					}
+
+					if (resolvedParts.length === 0) {
+						return;
+					}
+
+					state.hadVarSubstitution = true;
+					result = resolvedParts[0];
+				} else {
+					result = `${node.name}(${parts.join(" ")})`;
+				}
+
+				const parentParts = state.functionPartsStack.at(-1);
+				if (parentParts) {
+					parentParts.push(result);
+				} else {
+					state.valueParts.push(result);
+				}
 			},
 
 			"Rule > Block > Declaration:exit"(node) {
+				const state = declStack.pop();
 				if (node.property.startsWith("--")) {
 					// store the custom property name and value to validate later
 					vars.set(node.property, node.value);
@@ -273,106 +404,13 @@ export default {
 					return;
 				}
 
-				const varsFound = replacements.pop();
+				if (state.skipValidation) {
+					return;
+				}
 
-				/** @type {Map<string,CssLocationRange>} */
-				const valuesWithVarLocs = new Map();
-				const usingVars = varsFound?.size > 0;
 				let value = node.value;
-
-				if (usingVars) {
-					const valueList = [];
-					/**
-					 * Cache for resolved variable values within this single declaration.
-					 * Prevents re-resolving the same variable and re-walking long `var()` chains.
-					 * @type {Map<string,string>}
-					 */
-					const resolvedCache = new Map();
-					const valueNodes = node.value.children;
-
-					// When `var()` is used, we store all the values to `valueList` with the replacement of `var()` with there values or fallback values
-					for (const child of valueNodes) {
-						// If value is a function starts with `var()`
-						if (
-							child.type === "Function" &&
-							child.name.toLowerCase() === "var"
-						) {
-							const varValue = vars.get(child.children[0].name);
-
-							// If the variable is found, use its value, otherwise check for fallback values
-							if (varValue) {
-								const resolvedValue = resolveVariable(
-									child.children[0].name,
-									resolvedCache,
-								);
-								if (resolvedValue !== null) {
-									valueList.push(resolvedValue);
-									valuesWithVarLocs.set(
-										resolvedValue,
-										child.loc,
-									);
-								} else {
-									if (!allowUnknownVariables) {
-										context.report({
-											loc: child.children[0].loc,
-											messageId: "unknownVar",
-											data: {
-												var: child.children[0].name,
-											},
-										});
-										return;
-									}
-								}
-							} else {
-								// If the variable is not found and doesn't have a fallback value, report it
-								if (child.children.length === 1) {
-									if (!allowUnknownVariables) {
-										context.report({
-											loc: child.children[0].loc,
-											messageId: "unknownVar",
-											data: {
-												var: child.children[0].name,
-											},
-										});
-
-										return;
-									}
-								} else {
-									// If it has a fallback value, use that
-									if (child.children[2].type === "Raw") {
-										const raw =
-											child.children[2].value.trim();
-										const resolvedFallbackValue =
-											resolveFallback(raw, resolvedCache);
-										if (resolvedFallbackValue !== null) {
-											valueList.push(
-												resolvedFallbackValue,
-											);
-											valuesWithVarLocs.set(
-												resolvedFallbackValue,
-												child.loc,
-											);
-										} else if (!allowUnknownVariables) {
-											context.report({
-												loc: child.children[0].loc,
-												messageId: "unknownVar",
-												data: {
-													var: child.children[0].name,
-												},
-											});
-											return;
-										}
-									}
-								}
-							}
-						} else {
-							// If the child is not a `var()` function, just add its text to the `valueList`
-							const valueText = sourceCode.getText(child).trim();
-							valueList.push(valueText);
-							valuesWithVarLocs.set(valueText, child.loc);
-						}
-					}
-
+				if (state.hadVarSubstitution) {
+					const valueList = state.valueParts;
 					value =
 						valueList.length > 0
 							? valueList.join(" ")
@@ -385,7 +423,7 @@ export default {
 					// validation failure
 					if (isSyntaxMatchError(error)) {
 						const errorValue =
-							usingVars &&
+							state.hadVarSubstitution &&
 							value.slice(
 								error.mismatchOffset,
 								error.mismatchOffset + error.mismatchLength,
@@ -398,8 +436,8 @@ export default {
 							 * If so, use that location; otherwise, use the error's
 							 * reported location.
 							 */
-							loc: usingVars
-								? (valuesWithVarLocs.get(errorValue) ??
+							loc: state.hadVarSubstitution
+								? (state.valueSegmentLocs.get(errorValue) ??
 									node.value.loc)
 								: error.loc,
 							messageId: "invalidPropertyValue",
@@ -411,12 +449,8 @@ export default {
 								 * only include the part that caused the error.
 								 * Otherwise, use the full value from the error.
 								 */
-								value: usingVars
-									? value.slice(
-											error.mismatchOffset,
-											error.mismatchOffset +
-												error.mismatchLength,
-										)
+								value: state.hadVarSubstitution
+									? errorValue
 									: error.css,
 								expected: error.syntax,
 							},