[syntax-errors] Name is parameter and nonlocal

[WhiteFox0-0]

Summary

Part of #17412
Detects semantic syntax error where name is parameter and nonlocal

Test Plan

Added tests in nonlocal_parameter.py

[astral-sh-bot]

Memory usage report

Memory usage unchanged ✅

[astral-sh-bot]

ecosystem-analyzer results

No diagnostic changes detected ✅

Full report with detailed diff (timing results)

[astral-sh-bot]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

[ntBre]

Thank you, this looks good on the Ruff side, but I think we need to check how ty handles some duplicate diagnostics. I'd also prefer if we could use mdtests on the Ruff side too.

[ntBre]

Let's use mdtests for this:

ruff/CONTRIBUTING.md

Lines 233 to 240 in bd2d6f6

	#### Rule testing: fixtures and snapshots
	To test rules, Ruff uses the mdtest framework, initially developed for ty. Mdtests are written as
	Markdown files with Python code and TOML configuration blocks alongside prose
	descriptions. Generally, there will be one directory per linter (e.g. `flake8-bandit` for the `S`
	rules) with nested files for each rule (e.g. `unsafe-markup-use.md` for `S704`). Within these
	files, you can define additional Markdown sections to group related tests and their settings
	together.

We actually already have a file for this in ty:

ruff/crates/ty_python_semantic/resources/mdtest/diagnostics/semantic_syntax_errors.md

Line 535 in 9a908ef

## name cannot refer to a parameter and a global variable

And we could now add one in Ruff, maybe at crates/ruff_linter/resources/mdtest/invalid-syntax/nonlocal-parameter.md.

Speaking of ty, we may also need to check where it implements some of these checks. It already emits two diagnostics on this code:

def f(a):
    nonlocal a

Playground

I think this PR will make it emit a third.

[WhiteFox0-0]

This is where the no binding for nonlocal ``a`` found comes from

ruff/crates/ruff_python_parser/src/semantic_errors.rs

Lines 325 to 334 in fbe3e25

	if !ctx.in_module_scope() {
	for name in names {
	if !ctx.has_nonlocal_binding(name) {
	Self::add_error(
	ctx,
	SemanticSyntaxErrorKind::NonlocalWithoutBinding(name.to_string()),
	name.range,
	);
	}
	}

this is where i updated

for name in names {
    if ctx.is_bound_parameter(name) {
        Self::add_error(
            ctx,
            SemanticSyntaxErrorKind::NonlocalParameter(name.to_string()),
            name.range,
        );
    }

    if !ctx.has_nonlocal_binding(name) {
        Self::add_error(
            ctx,
            SemanticSyntaxErrorKind::NonlocalWithoutBinding(name.to_string()),
            name.range,
        );
    }
}

And following causes name a is used prior to nonlocal declaration

ruff/crates/ty_python_core/src/builder.rs

Lines 4335 to 4344 in fbe3e25

	if symbol.is_bound() || symbol.is_declared() || symbol.is_used() {
	self.report_semantic_error(SemanticSyntaxError {
	kind: SemanticSyntaxErrorKind::LoadBeforeNonlocalDeclaration {
	name: name.to_string(),
	start: name.range.start(),
	},
	range: name.range,
	python_version: self.python_version(),
	});
	}

After i added !symbol.is_parameter()

if (symbol.is_bound() || symbol.is_declared() || symbol.is_used()) 
    && !symbol.is_parameter()

it throws 2 errors

[invalid-syntax] "name `a` cannot refer to a parameter and a nonlocal variable"
[invalid-syntax] "no binding for nonlocal `a` found"

[WhiteFox0-0]

I have added the mdtest in ty.
I tried adding mdtest in ruff crates/ruff_linter/resources/mdtest/invalid-syntax/nonlocal-parameter.md but got unmatched assertion: snapshot: invalid-syntax maybe i am missing something

[ntBre]

I tried adding mdtest in ruff

Oops, yeah that's my bad. Unfortunately the Ruff mdtest runner filters out syntax errors. I should probably fix that at some point. Thanks for trying!

On the ty side, it looks like the "no binding for nonlocal a found` actually comes from this code:

ruff/crates/ty_python_core/src/builder.rs

Lines 1041 to 1047 in fbe3e25

	if matches!(declaration.kind, GlobalOrNonlocal::Nonlocal) {
	self.report_semantic_error(SemanticSyntaxError {
	kind: SemanticSyntaxErrorKind::NonlocalWithoutBinding(name.to_string()),
	range: declaration.range,
	python_version: self.python_version(),
	});
	}

which we can skip with a patch like this (which also covers the other is_parameter check):

                         .record_expression(name, self.current_scope());
                     let symbol_id = self.add_symbol(name.id.clone());
                     let symbol = self.current_place_table().symbol(symbol_id);
+                    // The semantic checker already reports that parameters cannot be nonlocal.
+                    if symbol.is_parameter() {
+                        continue;
+                    }
                     // Check whether the variable has already been accessed in this scope.
-                    if (symbol.is_bound() || symbol.is_declared() || symbol.is_used())
-                        && !symbol.is_parameter()
-                    {
+                    if symbol.is_bound() || symbol.is_declared() || symbol.is_used() {
                         self.report_semantic_error(SemanticSyntaxError {
                             kind: SemanticSyntaxErrorKind::LoadBeforeNonlocalDeclaration {
                                 name: name.to_string(),

but I think we'll need a ty reviewer to verify whether that's the correct behavior. It seems preferable to me to match CPython and only emit the parameter and nonlocal error:

>>> def f(a):
...     nonlocal a
...
  File "<python-input-0>", line 2
    nonlocal a
    ^^^^^^^^^^
SyntaxError: name 'a' is parameter and nonlocal

but I'm not quite sure if there are other implications of skipping the rest of the loop like in this patch. It at least doesn't seem to break any existing tests when I tried it locally.

[WhiteFox0-0]

CPython and only emit the parameter and nonlocal

I did look at this but i thought that Cpython emits only the first error it encounters rather than continue.
but as far as i know we do check for other encounters and report them too

I looked at the parameter and global implementation for the same it also emits the binding error so fi we are to change that for nonlocal should also look to change it for parameter and global

[ntBre]

Hmm, I guess you're right. The name is used prior to nonlocal declaration overlaps directly with parameter and nonlocal, but you could fix the parameter issue and still have no binding for nonlocal so it seems okay to emit both. Thanks for pushing back!

[WhiteFox0-0]

Yeah that makes sense, so is there anything else that needs to be done

[ntBre]

No I think this looks good then, thank you!

[ntBre]

I tried adding mdtest in ruff

Oops, yeah that's my bad. Unfortunately the Ruff mdtest runner filters out syntax errors. I should probably fix that at some point. Thanks for trying!

On the ty side, it looks like the "no binding for nonlocal a found` actually comes from this code:

ruff/crates/ty_python_core/src/builder.rs

Lines 1041 to 1047 in fbe3e25

	if matches!(declaration.kind, GlobalOrNonlocal::Nonlocal) {
	self.report_semantic_error(SemanticSyntaxError {
	kind: SemanticSyntaxErrorKind::NonlocalWithoutBinding(name.to_string()),
	range: declaration.range,
	python_version: self.python_version(),
	});
	}

which we can skip with a patch like this (which also covers the other is_parameter check):

                         .record_expression(name, self.current_scope());
                     let symbol_id = self.add_symbol(name.id.clone());
                     let symbol = self.current_place_table().symbol(symbol_id);
+                    // The semantic checker already reports that parameters cannot be nonlocal.
+                    if symbol.is_parameter() {
+                        continue;
+                    }
                     // Check whether the variable has already been accessed in this scope.
-                    if (symbol.is_bound() || symbol.is_declared() || symbol.is_used())
-                        && !symbol.is_parameter()
-                    {
+                    if symbol.is_bound() || symbol.is_declared() || symbol.is_used() {
                         self.report_semantic_error(SemanticSyntaxError {
                             kind: SemanticSyntaxErrorKind::LoadBeforeNonlocalDeclaration {
                                 name: name.to_string(),

but I think we'll need a ty reviewer to verify whether that's the correct behavior. It seems preferable to me to match CPython and only emit the parameter and nonlocal error:

>>> def f(a):
...     nonlocal a
...
  File "<python-input-0>", line 2
    nonlocal a
    ^^^^^^^^^^
SyntaxError: name 'a' is parameter and nonlocal

but I'm not quite sure if there are other implications of skipping the rest of the loop like in this patch. It at least doesn't seem to break any existing tests when I tried it locally.

[carljm]

This change does not look desirable. This mdtest is in the diagnostics/ subdirectory, so its entire purpose is to snapshot the diagnostic and ensure it looks the way we want it to. I don't think we should replace snapshot: with simply error: here.

[ntBre]

Oh sorry, that was my bad recommendation then. I thought one snapshot would suffice but didn't notice the directory. I may have put this file in the wrong directory to begin with...

[carljm]

Oh! Sorry, I may not have looked closely enough here. If you created this file and these extra snapshots are redundant, go ahead. (But you may be right that ideally we'd split diagnostics/semantic_syntax_errors.md into two files, where this one is fully focused on how the diagnostics look, and another file handles all the semantic edge cases.)

[ntBre]

Thank you! This looks good to me. I can follow up on the ty mdtest placement if needed.

[ntBre]

No I think this looks good then, thank you!

[astral-sh-bot]

Typing conformance results

No changes detected ✅

Current numbers

The percentage of diagnostics emitted that were expected errors held steady at 97.14%. The percentage of expected errors that received a diagnostic held steady at 93.44%. The number of fully passing files held steady at 107/137.