From f87f784a429f626e67710ab1a8026c0725414a3b Mon Sep 17 00:00:00 2001 From: Masafumi Koba <473530+ybiquitous@users.noreply.github.com> Date: Sun, 28 Apr 2024 08:08:26 +0900 Subject: [PATCH] Fix `shorthand-property-no-redundant-values` false negatives for functions (#7657) Co-authored-by: Marc G. --- .changeset/famous-deers-build.md | 5 ++ .../__tests__/index.mjs | 29 ++++++--- .../index.cjs | 60 +++++++++--------- .../index.mjs | 61 +++++++++---------- lib/utils/typeGuards.cjs | 9 +++ lib/utils/typeGuards.mjs | 8 +++ 6 files changed, 102 insertions(+), 70 deletions(-) create mode 100644 .changeset/famous-deers-build.md diff --git a/.changeset/famous-deers-build.md b/.changeset/famous-deers-build.md new file mode 100644 index 0000000000..260e1856bc --- /dev/null +++ b/.changeset/famous-deers-build.md @@ -0,0 +1,5 @@ +--- +"stylelint": minor +--- + +Fixed: `shorthand-property-no-redundant-values` false negatives for functions diff --git a/lib/rules/shorthand-property-no-redundant-values/__tests__/index.mjs b/lib/rules/shorthand-property-no-redundant-values/__tests__/index.mjs index 04ae23f1f0..75e0fc621f 100644 --- a/lib/rules/shorthand-property-no-redundant-values/__tests__/index.mjs +++ b/lib/rules/shorthand-property-no-redundant-values/__tests__/index.mjs @@ -78,23 +78,19 @@ testRule({ description: 'ignore not shorthandable property', }, { - code: 'a { border-radius: 1px / 1px }', + code: 'a { border-radius: 1px / 1px; }', description: 'ignore ellipse', }, { - code: 'a { margin: calc(1px + 1px) calc(1px + 1px); }', - description: 'ignore calc function', - }, - { - code: 'a { margin: some-function(1px + 1px) some-function(1px + 1px); }', - description: 'ignore all function', + code: 'a { border-radius: 1px 1px / 1px; }', + description: 'ignore ellipse', }, { code: 'a { margin: var(--margin) var(--margin); }', description: 'ignore variables', }, { - code: 'a { border-color: #FFFFFF transparent transparent }', + code: 'a { border-color: #FFFFFF transparent transparent; }', description: 'ignore upper case value', }, { @@ -381,6 +377,23 @@ testRule({ line: 1, column: 5, }, + { + code: 'a { margin: calc(1px + 1px) calc(1px + 1px); }', + fixed: 'a { margin: calc(1px + 1px); }', + message: messages.rejected('calc(1px + 1px) calc(1px + 1px)', 'calc(1px + 1px)'), + line: 1, + column: 5, + }, + { + code: 'a { margin: some-function(1px + 1px) some-function(1px + 1px); }', + fixed: 'a { margin: some-function(1px + 1px); }', + message: messages.rejected( + 'some-function(1px + 1px) some-function(1px + 1px)', + 'some-function(1px + 1px)', + ), + line: 1, + column: 5, + }, ], }); diff --git a/lib/rules/shorthand-property-no-redundant-values/index.cjs b/lib/rules/shorthand-property-no-redundant-values/index.cjs index fc46bc742b..8f57e1d486 100644 --- a/lib/rules/shorthand-property-no-redundant-values/index.cjs +++ b/lib/rules/shorthand-property-no-redundant-values/index.cjs @@ -5,10 +5,12 @@ const valueParser = require('postcss-value-parser'); const isStandardSyntaxDeclaration = require('../../utils/isStandardSyntaxDeclaration.cjs'); const isStandardSyntaxProperty = require('../../utils/isStandardSyntaxProperty.cjs'); +const isStandardSyntaxValue = require('../../utils/isStandardSyntaxValue.cjs'); const report = require('../../utils/report.cjs'); const ruleMessages = require('../../utils/ruleMessages.cjs'); const validateOptions = require('../../utils/validateOptions.cjs'); const vendor = require('../../utils/vendor.cjs'); +const typeGuards = require('../../utils/typeGuards.cjs'); const ruleName = 'shorthand-property-no-redundant-values'; @@ -21,7 +23,7 @@ const meta = { fixable: true, }; -const propertiesWithShorthandNotation = new Set([ +const SUPPORTED_SHORTHANDS = new Set([ 'margin', 'padding', 'border-color', @@ -32,22 +34,12 @@ const propertiesWithShorthandNotation = new Set([ 'inset', ]); -const ignoredCharacters = ['+', '*', '/', '(', ')', '$', '@', '--', 'var(']; - -/** - * @param {string} value - * @returns {boolean} - */ -function hasIgnoredCharacters(value) { - return ignoredCharacters.some((char) => value.includes(char)); -} - /** * @param {string} property * @returns {boolean} */ -function isShorthandProperty(property) { - return propertiesWithShorthandNotation.has(property); +function isSupportedShorthand(property) { + return SUPPORTED_SHORTHANDS.has(property); } /** @@ -125,40 +117,46 @@ const rule = (primary, _secondaryOptions, context) => { } root.walkDecls((decl) => { - if (!isStandardSyntaxDeclaration(decl) || !isStandardSyntaxProperty(decl.prop)) { - return; - } + if (!isStandardSyntaxDeclaration(decl)) return; + + const { prop, value } = decl; - const prop = decl.prop; - const value = decl.value; + if (!isStandardSyntaxProperty(prop)) return; + + if (!isStandardSyntaxValue(value)) return; const normalizedProp = vendor.unprefixed(prop.toLowerCase()); - if (hasIgnoredCharacters(value) || !isShorthandProperty(normalizedProp)) { - return; - } + if (!isSupportedShorthand(normalizedProp)) return; /** @type {string[]} */ const valuesToShorthand = []; - valueParser(value).walk((valueNode) => { - if (valueNode.type !== 'word') { + for (const valueNode of valueParser(value).nodes) { + if (typeGuards.isValueDiv(valueNode)) { + return; + } + + // `var()` should be ignored. E.g. + // + // --padding: 20px 5px; + // padding: 0 var(--padding) 0; + // + if (typeGuards.isValueFunction(valueNode) && valueNode.value.toLowerCase() === 'var') { return; } - valuesToShorthand.push(valueParser.stringify(valueNode)); - }); + if (typeGuards.isValueWord(valueNode) || typeGuards.isValueFunction(valueNode)) { + valuesToShorthand.push(valueParser.stringify(valueNode)); + } + } if (valuesToShorthand.length <= 1 || valuesToShorthand.length > 4) { return; } - const shortestForm = canCondense( - valuesToShorthand[0] || '', - valuesToShorthand[1] || '', - valuesToShorthand[2] || '', - valuesToShorthand[3] || '', - ); + const [top, right, bottom, left] = valuesToShorthand; + const shortestForm = canCondense(top ?? '', right ?? '', bottom ?? '', left ?? ''); const shortestFormString = shortestForm.filter(Boolean).join(' '); const valuesFormString = valuesToShorthand.join(' '); diff --git a/lib/rules/shorthand-property-no-redundant-values/index.mjs b/lib/rules/shorthand-property-no-redundant-values/index.mjs index 55d2c18212..2a50fc2878 100644 --- a/lib/rules/shorthand-property-no-redundant-values/index.mjs +++ b/lib/rules/shorthand-property-no-redundant-values/index.mjs @@ -2,11 +2,14 @@ import valueParser from 'postcss-value-parser'; import isStandardSyntaxDeclaration from '../../utils/isStandardSyntaxDeclaration.mjs'; import isStandardSyntaxProperty from '../../utils/isStandardSyntaxProperty.mjs'; +import isStandardSyntaxValue from '../../utils/isStandardSyntaxValue.mjs'; import report from '../../utils/report.mjs'; import ruleMessages from '../../utils/ruleMessages.mjs'; import validateOptions from '../../utils/validateOptions.mjs'; import vendor from '../../utils/vendor.mjs'; +import { isValueDiv, isValueFunction, isValueWord } from '../../utils/typeGuards.mjs'; + const ruleName = 'shorthand-property-no-redundant-values'; const messages = ruleMessages(ruleName, { @@ -18,7 +21,7 @@ const meta = { fixable: true, }; -const propertiesWithShorthandNotation = new Set([ +const SUPPORTED_SHORTHANDS = new Set([ 'margin', 'padding', 'border-color', @@ -29,22 +32,12 @@ const propertiesWithShorthandNotation = new Set([ 'inset', ]); -const ignoredCharacters = ['+', '*', '/', '(', ')', '$', '@', '--', 'var(']; - -/** - * @param {string} value - * @returns {boolean} - */ -function hasIgnoredCharacters(value) { - return ignoredCharacters.some((char) => value.includes(char)); -} - /** * @param {string} property * @returns {boolean} */ -function isShorthandProperty(property) { - return propertiesWithShorthandNotation.has(property); +function isSupportedShorthand(property) { + return SUPPORTED_SHORTHANDS.has(property); } /** @@ -122,40 +115,46 @@ const rule = (primary, _secondaryOptions, context) => { } root.walkDecls((decl) => { - if (!isStandardSyntaxDeclaration(decl) || !isStandardSyntaxProperty(decl.prop)) { - return; - } + if (!isStandardSyntaxDeclaration(decl)) return; - const prop = decl.prop; - const value = decl.value; + const { prop, value } = decl; + + if (!isStandardSyntaxProperty(prop)) return; + + if (!isStandardSyntaxValue(value)) return; const normalizedProp = vendor.unprefixed(prop.toLowerCase()); - if (hasIgnoredCharacters(value) || !isShorthandProperty(normalizedProp)) { - return; - } + if (!isSupportedShorthand(normalizedProp)) return; /** @type {string[]} */ const valuesToShorthand = []; - valueParser(value).walk((valueNode) => { - if (valueNode.type !== 'word') { + for (const valueNode of valueParser(value).nodes) { + if (isValueDiv(valueNode)) { return; } - valuesToShorthand.push(valueParser.stringify(valueNode)); - }); + // `var()` should be ignored. E.g. + // + // --padding: 20px 5px; + // padding: 0 var(--padding) 0; + // + if (isValueFunction(valueNode) && valueNode.value.toLowerCase() === 'var') { + return; + } + + if (isValueWord(valueNode) || isValueFunction(valueNode)) { + valuesToShorthand.push(valueParser.stringify(valueNode)); + } + } if (valuesToShorthand.length <= 1 || valuesToShorthand.length > 4) { return; } - const shortestForm = canCondense( - valuesToShorthand[0] || '', - valuesToShorthand[1] || '', - valuesToShorthand[2] || '', - valuesToShorthand[3] || '', - ); + const [top, right, bottom, left] = valuesToShorthand; + const shortestForm = canCondense(top ?? '', right ?? '', bottom ?? '', left ?? ''); const shortestFormString = shortestForm.filter(Boolean).join(' '); const valuesFormString = valuesToShorthand.join(' '); diff --git a/lib/utils/typeGuards.cjs b/lib/utils/typeGuards.cjs index 6272694be2..efd1c9c056 100644 --- a/lib/utils/typeGuards.cjs +++ b/lib/utils/typeGuards.cjs @@ -53,6 +53,14 @@ function isDocument(node) { return node.type === 'document'; } +/** + * @param {import('postcss-value-parser').Node} node + * @returns {node is import('postcss-value-parser').DivNode} + */ +function isValueDiv(node) { + return node.type === 'div'; +} + /** * @param {import('postcss-value-parser').Node} node * @returns {node is import('postcss-value-parser').FunctionNode} @@ -92,6 +100,7 @@ exports.isDeclaration = isDeclaration; exports.isDocument = isDocument; exports.isRoot = isRoot; exports.isRule = isRule; +exports.isValueDiv = isValueDiv; exports.isValueFunction = isValueFunction; exports.isValueSpace = isValueSpace; exports.isValueWord = isValueWord; diff --git a/lib/utils/typeGuards.mjs b/lib/utils/typeGuards.mjs index 04bc5d2768..2117ddb85b 100644 --- a/lib/utils/typeGuards.mjs +++ b/lib/utils/typeGuards.mjs @@ -49,6 +49,14 @@ export function isDocument(node) { return node.type === 'document'; } +/** + * @param {import('postcss-value-parser').Node} node + * @returns {node is import('postcss-value-parser').DivNode} + */ +export function isValueDiv(node) { + return node.type === 'div'; +} + /** * @param {import('postcss-value-parser').Node} node * @returns {node is import('postcss-value-parser').FunctionNode}