C408 fix changes behavior for non-NFKC identifier

[dscorbett]

Description

The fix for unnecessary-collection-call (C408) changes the program’s behavior when a dict call’s keyword argument’s runtime name does not equal the name as it appears in the source code. This happens when the name in the source code isn’t normalized in NFKC.

$ cat >c408.py <<'# EOF'
print(dict(ℼ=3.14))
# EOF

$ python c408.py
{'π': 3.14}

$ ruff --isolated check --select C408 c408.py --unsafe-fixes --fix
Found 1 error (1 fixed, 0 remaining).

$ python c408.py
{'ℼ': 3.14}

[ntBre]

I took a brief look at the code here and didn't immediately see anything weird going on with Unicode stuff. I wonder if something could be going wrong with the libcst codegen around here?

ruff/crates/ruff_linter/src/rules/flake8_comprehensions/fixes.rs

Lines 286 to 291 in 0868e73

	pad_expression(
	tree.codegen_stylist(stylist),
	expr.range(),
	checker.locator(),
	checker.semantic(),
	),

This feels like it could be a more general issue than just this rule.

[jelle-openai]

I believe the issue here is that Python NFKC-normalizes identifiers, but not string literals. In the original dict(ℼ=3.14), ℼ is an identifier so it gets normalized:

>>> unicodedata.normalize('NFKC', 'ℼ')
'π'

But Ruff turns this into {'ℼ': 3.14}, which is not an identifier and doesn't get normalized.

The fix would be to NFKC-normalize any identifiers that the C408 autofix turns into string literals.

This issue could also appear in other fixes if they turn identifiers into string literals, but that's not going to be very common.

Perhaps Ruff should also have a rule against identifiers that are not NFKC-normalized.

[ntBre]

Ah, thanks @jelle-openai! That makes more sense.

[AlexWaygood]

We emulate Python's NFKC normalisation for identifiers in the lexer here:

ruff/crates/ruff_python_parser/src/lexer.rs

Lines 639 to 655 in e84985e

	// Keep track of whether the identifier is ASCII-only or not.
	//
	// This is important because Python applies NFKC normalization to
	// identifiers: https://docs.python.org/3/reference/lexical_analysis.html#identifiers.
	// We need to therefore do the same in our lexer, but applying NFKC normalization
	// unconditionally is extremely expensive. If we know an identifier is ASCII-only,
	// (by far the most common case), we can skip NFKC normalization of the identifier.
	let mut is_ascii = first.is_ascii();
	self.cursor
	.eat_while(|c| is_identifier_continuation(c, &mut is_ascii));
	let text = self.token_text();
	if !is_ascii {
	self.current_value = TokenValue::Name(text.nfkc().collect::<Name>());
	return TokenKind::Name;
	}

We've had issues in the past when fixes use libcst due to the fact that libcst uses a different parser, and does not do the same NFKC normalisation of identifiers. This looks like it stems from the same issue

[AlexWaygood]

Perhaps Ruff should also have a rule against identifiers that are not NFKC-normalized.

It actually surprises me that we don't already! Considering that we already have RUF001, RUF002 and RUF003