[ty] Don't warn yield not in function when yield is in function

[maxmynter]

Closes: astral-sh/ty#299

Summary

Don't warn yield not in function when yield is in function.

This is achieved by iterating through the scope stack to check if it contains a function scope.

The issue was found when creating the semantic syntax md integration tests (#17748 (comment)).

Test Plan

Updated integration tests to reflect this behavior.

---

I don't know if I fully get how issues / PR's work across the boundaries of ruff and ty. Let me know if i'm doing something wrong.

[github-actions]

mypy_primer results

No ecosystem changes detected ✅

[ntBre]

I think we should also return false if we see a ScopeKind::Class:

>>> def f():
...     class C:
...         yield 5
...
  File "<python-input-1>", line 3
    yield 5
    ^^^^^^^
SyntaxError: 'yield' outside function

I'm also realizing that I gave bad advice in my comment on the issue. The in_*_scope methods are supposed to refer to the directly-enclosing scope:

ruff/crates/ruff_python_parser/src/semantic_errors.rs

Lines 1649 to 1663 in 7a48477

	/// Note that the `in_*_scope` methods should refer to the immediately-enclosing scope. For example,
	/// `in_function_scope` should return true for this case:
	///
	/// ```python
	/// def f():
	/// x # here
	/// ```
	///
	/// but not for this case:
	///
	/// ```python
	/// def f():
	/// class C:
	/// x # here
	/// ```

We may just want to reuse in_await_allowed_context for detecting this syntax error and also possibly rename it to something like in_function_context.

That should fix the ruff implementation of the rule too, which I guess I hadn't seen issues with either.

[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.

[maxmynter]

All right, this took longer than I thought for a quite small diff.

• Reverted the change to in_function_scope to only consider the immediate scope.

• I'm on the fence with renaming in_await_allowed_context. Especially considering the contextual comment in the trait definition it seems the current name better conveys what the function does.

• The upward scope stack traversal in in_await_allowed_context needs to be checked against generator and comprehensions to not give false positives/negatives.

• Rename in_sync_comprehension to in_sync_comprehension_scope for consistency with other locator methods in scope.

Questions:

• How important is performance here? Specifically, how expensive is traversing the scope stack? We can avoid the duplicate call of is_await_allowed_context at some cost of readability with more nesting. I've already ordered the early returns in increasing expensiveness.

[maxmynter]

The notebook in the ecosystem changes gives the same Invalid JSON error when seen through the Git web ui.

[ntBre]

This seems a bit strange because in_sync_comprehension_scope and in_await_allowed_context both traverse the parent scopes, but in_generator_scope doesn't. Is there a way to break this check by nesting generators somehow?

I think I would probably lean toward a separate context method anyway, just for clarity. Traversing scopes should be pretty cheap since it's just iterating backwards over a vector for both ruff and ty, but I think this whole check would be simplified if we had an in_yield_allowed_context method or similar. And then we can traverse the stack once anyway.

Ah, actually I think that's the reason for the else-if on kind.is_await. If we don't return from there, we'll run all of these checks again, even though we already know we want a diagnostic for await. So I think I would put this whole section back to something like this:

if kind.is_await { 
    // ...
} else if ctx.in_yield_allowed_context() {
    return
}

[maxmynter]

Makes sense. Adressed in 1d0e43b

I was shying away from adding a new method to the trait because then the implementation in crates/ruff_linter/src/checkers/ast/mod.rs isn't used.

But I now agree it's not that bad.

[ntBre]

We could also add a test case for ruff (is that what you mean by the Checker version not being used?)

[ntBre]

The notebook in the ecosystem changes gives the same Invalid JSON error when seen through the Git web ui.

Yeah no worries about this, I've been seeing it on every PR. If the issue persists we can ignore the file from the ecosystem check.

Edit: looks like they fixed it 6 hours ago. Next time the ecosystem check runs it should update!

[maxmynter]

It seems that backticked parts were sliced out of the commit messages.

They were supposed to be

• Drop _scope from in_sync_comprehension_scope (a62a990)

• (refactor) Add in_yield_allowed_context to SemanticSyntaxContext trait (1d0e43b)

But since we squash, i'll leave them as is.

[ntBre]

Thanks, this looks good to me! The only additional thing would be adding a test in ruff. I think it could have been susceptible to the same, or at least similar, initial issue. But the existing tests should at least exercise the new context method anyway.

It might be good to get a quick review from someone on the ty side too, but this is otherwise good to go.

[maxmynter]

Added tests in 7460049

[maxmynter]

Ping @carljm @dhruvmanila @MichaReiser (because Brent said it would be good if someone from the TY side would review and you're code owners and recent ty committers :) )