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

Fix function-no-unknown false positives for SCSS functions with namespace #6920

Closed
re757575 opened this issue Jun 15, 2023 · 10 comments · Fixed by #6921
Closed

Fix function-no-unknown false positives for SCSS functions with namespace #6920

re757575 opened this issue Jun 15, 2023 · 10 comments · Fixed by #6921
Labels
status: ready to implement is ready to be worked on by someone syntax: scss relates to SCSS and SCSS-like syntax type: bug a problem with a feature or rule

Comments

@re757575
Copy link

re757575 commented Jun 15, 2023

What steps are needed to reproduce the bug?

File

// src/assets/sass/utils/_functions.scss

@use 'sass:math';

$base-font-size: 16;

// px to rem
@function rem($px) {
  @return math.div($px, $base-font-size) + rem;
}
// src/assets/sass/utils/_index.scss

@forward 'functions' as f-*;
// src/App.scss

@use "/src/assets/sass/utils" as u;

#root {
  border: u.f-rem(20) solid red;
}

Run stylelint

Will got Error: Unexpected unknown function "u.f-rem" scss/function-no-unknown

Demo

https://codesandbox.io/p/sandbox/stylelint-test-function-no-unknown-t5ylc3?file=%2Fsrc%2FApp.scss

What Stylelint configuration is needed to reproduce the bug?

{
  "plugins": ["stylelint-scss"],
  "extends": ["stylelint-config-standard-scss"],
  "rules": {
    "scss/function-no-unknown": [
      true,
      {
        "ignoreFunctions": ["/^u\\.f-/"]
      }
    ]
  }
}

How did you run Stylelint?

stylelint '**/*.{css,scss}'

Which version of Stylelint or dependencies are you using?

>= 15.6.2

What did you expect to happen?

The 15.6.1 version does not have this error, and the version after updating to 15.6.2 has an error.
Or how do I need to adjust ignoreFunctions?

What actually happened?

src/App.scss
 4:13  ✖  Unexpected unknown function "u.f-rem"  scss/function-no-unknown

Does the bug relate to non-standard syntax?

SCSS

Proposal to fix the bug

No response

@ybiquitous
Copy link
Member

@re757575 Thanks for the report using the template. And for providing the demo!

But I think the reported rule scss/function-no-unknown comes from stylelint-scss, not stylelint, right?
https://github.com/stylelint-scss/stylelint-scss/tree/master/src/rules/function-no-unknown

@ybiquitous ybiquitous added the status: needs clarification triage needs clarification from author label Jun 16, 2023
@re757575
Copy link
Author

@re757575 Thanks for the report using the template. And for providing the demo!

But I think the reported rule scss/function-no-unknown comes from stylelint-scss, not stylelint, right? https://github.com/stylelint-scss/stylelint-scss/tree/master/src/rules/function-no-unknown

Sorry I didn't notice, but I haven't changed stylelint-scss (5.0.1) version.

Work:

stylelint: 15.6.1
stylelint-scss 5.0.1

Error:

stylelint: 15.6.2
stylelint-scss 5.0.1

@ybiquitous
Copy link
Member

Thank you. I can reproduce it now!

Stylelint 15.6.1 works

demo

image

Stylelint 15.6.2 does not work

demo

image

@ybiquitous
Copy link
Member

Maybe, the behavior change seems related to #6842. 🤔

The scss/function-no-unknown rule internally uses the function-no-unknown rule here:
https://github.com/stylelint-scss/stylelint-scss/blob/88fa72292c1eee313d8ac682eeeef5360ba5726b/src/rules/function-no-unknown/index.js#L81-L86

The function-no-unknown rule has stopped using postcss-value-parser for more accurate parsing since #6842, while the scss/function-no-unknown rule still has been using it. So, I guess we should fix scss/function-no-unknown to resolve this problem.

@ybiquitous ybiquitous added status: needs discussion triage needs further discussion and removed status: needs clarification triage needs clarification from author labels Jun 16, 2023
@ybiquitous
Copy link
Member

Maybe related to also stylelint-scss/stylelint-scss#817

@romainmenke
Copy link
Member

Definitely caused by the change in tokenizer/parser.

I think the <namespace>.<function-name> is the difference.

In the new parser that is handled as <ident><delim><function>
While I suspect it is handled as <function> in the old parser.


This test passes in 15.6.1 and fails in 15.6.2 :

testRule({
	ruleName,
	config: [true, { ignoreFunctions: ['color.adjust'] }],
	customSyntax: 'postcss-scss',

	accept: [
		{
			code: 'a { transform: color.adjust(1px); }',
		},
	],
});

Option 1 :

We already have if (!isStandardSyntaxValue(value)) return; in this rule, but that check doesn't detect name spaced functions, only name spaced variables.

// SCSS namespace (example namespace.$variable)
if (/^.+\.\$/.test(value)) {
	return false;
}

Changing it to also detect name spaced function would make the error go away.

Option 2 :

Intercept the token stream and rewrite <ident><delim><function> to <function>, only when <delim> is a full stop.

This would restore the old behavior but it might cause issues later if standard CSS has a use case for this token sequence.

@ybiquitous
Copy link
Member

@romainmenke Thanks for the investigation! Let's take Option 1 as you suggest. I think a plugin should handle non-standard syntaxes as possible.

@romainmenke
Copy link
Member

I think this should work :

// SCSS namespace (example namespace.$variable)
if (/^.+\.\$/.test(value)) {
	return false;
}

// SCSS namespace (example namespace.function-name())
if (/^.+\.[-\w]+\(/.test(value)) {
	return false;
}

@ybiquitous ybiquitous changed the title function-no-unknown ignoreFunctions not working after 15.6.2 Fix function-no-unknown false positives for SCSS functions with namespace Jun 16, 2023
@ybiquitous ybiquitous added status: ready to implement is ready to be worked on by someone type: bug a problem with a feature or rule syntax: scss relates to SCSS and SCSS-like syntax and removed status: needs discussion triage needs further discussion labels Jun 16, 2023
@ybiquitous
Copy link
Member

@romainmenke Sounds good 👍🏼

I've labeled the issue as ready to implement. Please consider contributing if you have time.

There are steps on how to fix a bug in a rule in the Developer guide.

@romainmenke
Copy link
Member

Just for reference :

/**
 * Convert <ident><full_stop><function> to <function> to support scss name spacing
 *
 * @param {Array<import('@csstools/css-tokenizer').CSSToken>} tokens
 * @returns {Array<import('@csstools/css-tokenizer').CSSToken>}
 */
function normalizeTokenStream(tokens) {
	for (let i = 0; i < tokens.length; i++) {
		const token = tokens[0];

		if (!token || token[0] !== TokenType.Ident) continue;

		const tokenPlus1 = tokens[1];

		if (!tokenPlus1 || tokenPlus1[0] !== TokenType.Delim || tokenPlus1[4].value !== '.') continue;

		const tokenPlus2 = tokens[2];

		if (!tokenPlus2 || tokenPlus2[0] !== TokenType.Function) continue;

		if (!tokenPlus2) continue;

		tokens.splice(i, 3, [
			TokenType.Function,
			token[1] + tokenPlus1[1] + tokenPlus2[1],
			token[2],
			tokenPlus2[3],
			{
				value: token[4].value + tokenPlus1[4].value + tokenPlus2[4].value,
			},
		]);
	}

	return tokens;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready to implement is ready to be worked on by someone syntax: scss relates to SCSS and SCSS-like syntax type: bug a problem with a feature or rule
Development

Successfully merging a pull request may close this issue.

3 participants