[syntax-errors] Name is parameter and global

[11happy]

Summary

This PR implements a new semantic syntax error where name is parameter & global.

Test Plan

I have written inline test as directed in #17412

[github-actions]

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]

I'd prefer not to reformat unrelated sections of code, as I mentioned on your other PR

[11happy]

sure , actually I am not changing these , these are automatically added by pre-commit jobs

[github-actions]

Diagnostic diff on typing conformance tests

No changes detected when running ty on typing conformance tests ✅

[github-actions]

mypy_primer results

No ecosystem changes detected ✅
No memory usage changes detected ✅

[ntBre]

Thank you, this is looking good. We just need to be a bit more careful with our scope handling and revert the unnecessary Violation changes.

[ntBre]

nit: I think a name like is_parameter or is_bound_parameter would be a bit better.

More importantly, and related to my comment on the test case above, I don't think this is quite right. We need to limit our parameter check to the nearest-enclosing function scope. In other words, we need to search through self.semantic.current_scopes more like we do in some of the other context methods:

ruff/crates/ruff_linter/src/checkers/ast/mod.rs

Lines 737 to 749 in bedfc6f

	fn in_async_context(&self) -> bool {
	for scope in self.semantic.current_scopes() {
	match scope.kind {
	ScopeKind::Class(_) | ScopeKind::Lambda(_) => return false,
	ScopeKind::Function(ast::StmtFunctionDef { is_async, .. }) => return *is_async,
	ScopeKind::Generator { .. }
	| ScopeKind::Module
	| ScopeKind::Type
	| ScopeKind::DunderClassCell => {}
	}
	}
	false
	}

until we find the first function scope, and then only check for shadowed bindings within that scope. We can also return early if we find a class scope, for example, because this is also valid, like the nested function case I showed above:

>>> def h(a):
...     class C:
...         global a
...
>>>

I think the checks on the binding below look roughly correct, but we need to be more careful about identifying the scope holding the relevant binding.

[11happy]

done

[ntBre]

Thank you, this is looking good for the tests currently implemented. I think we should a few more test cases, for example:

def f(a):
    a = 1
    global a  # error

This tests the shadowing code. I'm actually not sure I was correct to recommend checking shadowed_bindings. Rereading the docstring, it sounds like this only catches shadowing from different scopes. We might actually need to check for rebinding_scopes. In any case, a test like the one above, and possibly one with multiple rebindings, would be a good idea:

def f(a):
    a = 1
    a = 2
    global a  # error

I see that is_bound_parameter also has special handling for classes, so it seems worth including a test case with an intervening class scope, probably more than one case showing some of the interleavings like function-class-function, class-function-class, etc, or at least the interesting ones. I think something like this would be interesting at a minimum:

def f(a):
    class Inner:
        global a

I also had a couple of minor nits about spaces between functions.

[11happy]

So the shadowed_bindings in SemanticModel and Scope are different, respectively. As per the example given in the docstring of Scope

/// A map from binding ID to binding ID that it shadows.
///
/// For example:
/// ```python
/// def f():
///     x = 1
///     x = 2
/// ```
///
/// In this case, the binding created by `x = 2` shadows the binding created by `x = 1`.
shadowed_bindings: FxHashMap<BindingId, BindingId>,

We can utilize this to trace back whether a global declaration is originally an argument or not — and I’ve done that accordingly. I also think we don’t really need to interact with rebindings_scope here. I manually checked with an example, and it was coming out to be empty in the following case:

def f(a):
    a = 1
    a = 2
    global a

Also, while trying to solve this issue, I read a lot about bindings, shadowed bindings, etc., and I’m genuinely thankful for the opportunity to work on this. The tests are working as expected now

Thank you : )

[11happy]

is the CI/fuzz parser error related to this PR changes ?

[ntBre]

This is what the fuzzer seeds that are failing look like. This is not supposed to be an error:

>>> def f(a):
...     class Inner:
...         global a
...
>>> f
<function f at 0x7fbec085be20>

[11happy]

done , have fixed it & fixed the merge conflict as well (rebased)
Thank you : )

[11happy]

Currently we are doing an early return for class scope , please let me know if there are any more edge cases you would like me to test , would be happy to add them.

Thank you

[ntBre]

Thanks, this is looking good. I had one idea to simplify is_bound_parameter and a suggestion for a follow-up.

[ntBre]

Ah, I overlooked this initially, but the ScopeKinds here actually contain these nodes' definitions. We should be able to extract the parameters from that and avoid this binding stuff entirely. I always get a little nervous when I see a loop, so I looked a bit harder at this today.

    fn is_bound_parameter(&self, name: &str) -> bool {
        for scope in self.semantic.current_scopes() {
            match scope.kind {
                ScopeKind::Class(_) => return false,
                ScopeKind::Function(ast::StmtFunctionDef { parameters, .. })
                | ScopeKind::Lambda(ast::ExprLambda {
                    parameters: Some(parameters),
                    ..
                }) => return parameters.includes(name),
                ScopeKind::Lambda(_)
                | ScopeKind::Generator { .. }
                | ScopeKind::Module
                | ScopeKind::Type
                | ScopeKind::DunderClassCell => {}
            }
        }

        false
    }

I believe it's actually impossible for a lambda to contain a global since it can only contain a single expression and not a statement, so we could probably eliminate the lambda branch here too.

[11happy]

done

[11happy]

Now that I’m looking at this and the previous iterations, I feel like if we’re starting to make the code more complex, something’s probably off with the approach. From now on, I’ll be more careful and focus on keeping things simpler.

Thank you : )

[ntBre]

Thank you!