-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[syntax-errors] Allow yield in base classes and annotations
#17206
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| # parse_options: {"target-version": "3.14"} | ||
| def f() -> (y := 3): ... | ||
| def g(arg: (x := 1)): ... | ||
| def outer(): | ||
| def i(x: (yield 1)): ... | ||
| def k() -> (yield 1): ... | ||
| def m(x: (yield from 1)): ... | ||
| def o() -> (yield from 1): ... |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,4 @@ | ||
| class F(y := list): ... | ||
| def f(): | ||
| class G((yield 1)): ... | ||
| class H((yield from 1)): ... |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,7 @@ | ||
| def f() -> (y := 3): ... | ||
| def g(arg: (x := 1)): ... | ||
| def outer(): | ||
| def i(x: (yield 1)): ... | ||
| def k() -> (yield 1): ... | ||
| def m(x: (yield from 1)): ... | ||
| def o() -> (yield from 1): ... |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -128,18 +128,29 @@ impl SemanticSyntaxChecker { | |
| // test_ok valid_annotation_function | ||
| // def f() -> (y := 3): ... | ||
| // def g(arg: (x := 1)): ... | ||
| // def outer(): | ||
| // def i(x: (yield 1)): ... | ||
| // def k() -> (yield 1): ... | ||
| // def m(x: (yield from 1)): ... | ||
| // def o() -> (yield from 1): ... | ||
|
|
||
| // test_err invalid_annotation_function_py314 | ||
| // # parse_options: {"target-version": "3.14"} | ||
| // def f() -> (y := 3): ... | ||
| // def g(arg: (x := 1)): ... | ||
| // def outer(): | ||
| // def i(x: (yield 1)): ... | ||
| // def k() -> (yield 1): ... | ||
| // def m(x: (yield from 1)): ... | ||
| // def o() -> (yield from 1): ... | ||
|
|
||
| // test_err invalid_annotation_function | ||
| // def f[T]() -> (y := 3): ... | ||
| // def g[T](arg: (x := 1)): ... | ||
| // def h[T](x: (yield 1)): ... | ||
| // def i(x: (yield 1)): ... | ||
| // def j[T]() -> (yield 1): ... | ||
| // def k() -> (yield 1): ... | ||
| // def l[T](x: (yield from 1)): ... | ||
| // def m(x: (yield from 1)): ... | ||
| // def n[T]() -> (yield from 1): ... | ||
| // def o() -> (yield from 1): ... | ||
| // def p[T: (yield 1)](): ... # yield in TypeVar bound | ||
| // def q[T = (yield 1)](): ... # yield in TypeVar default | ||
| // def r[*Ts = (yield 1)](): ... # yield in TypeVarTuple default | ||
|
|
@@ -148,19 +159,20 @@ impl SemanticSyntaxChecker { | |
| // def u[T = (x := 1)](): ... # named expr in TypeVar default | ||
| // def v[*Ts = (x := 1)](): ... # named expr in TypeVarTuple default | ||
| // def w[**Ts = (x := 1)](): ... # named expr in ParamSpec default | ||
| let is_generic = type_params.is_some(); | ||
| let mut visitor = InvalidExpressionVisitor { | ||
| allow_named_expr: !is_generic, | ||
| position: InvalidExpressionPosition::TypeAnnotation, | ||
| ctx, | ||
| }; | ||
| if let Some(type_params) = type_params { | ||
| visitor.visit_type_params(type_params); | ||
| } | ||
| if is_generic { | ||
| // the __future__ annotation error takes precedence over the generic error | ||
| if ctx.future_annotations_or_stub() || ctx.python_version() > PythonVersion::PY313 { | ||
| visitor.position = InvalidExpressionPosition::TypeAnnotation; | ||
| } else if type_params.is_some() { | ||
| visitor.position = InvalidExpressionPosition::GenericDefinition; | ||
| } else { | ||
| visitor.position = InvalidExpressionPosition::TypeAnnotation; | ||
| return; | ||
| } | ||
| for param in parameters | ||
| .iter() | ||
|
|
@@ -173,36 +185,29 @@ impl SemanticSyntaxChecker { | |
| } | ||
| } | ||
| Stmt::ClassDef(ast::StmtClassDef { | ||
| type_params, | ||
| type_params: Some(type_params), | ||
| arguments, | ||
| .. | ||
| }) => { | ||
| // test_ok valid_annotation_class | ||
| // class F(y := list): ... | ||
| // def f(): | ||
| // class G((yield 1)): ... | ||
| // class H((yield from 1)): ... | ||
|
|
||
| // test_err invalid_annotation_class | ||
| // class F[T](y := list): ... | ||
| // class G((yield 1)): ... | ||
| // class H((yield from 1)): ... | ||
| // class I[T]((yield 1)): ... | ||
| // class J[T]((yield from 1)): ... | ||
| // class K[T: (yield 1)]: ... # yield in TypeVar | ||
| // class L[T: (x := 1)]: ... # named expr in TypeVar | ||
| let is_generic = type_params.is_some(); | ||
| let mut visitor = InvalidExpressionVisitor { | ||
| allow_named_expr: !is_generic, | ||
| position: InvalidExpressionPosition::TypeAnnotation, | ||
| ctx, | ||
| }; | ||
| if let Some(type_params) = type_params { | ||
| visitor.visit_type_params(type_params); | ||
| } | ||
| if is_generic { | ||
| visitor.position = InvalidExpressionPosition::GenericDefinition; | ||
| } else { | ||
| visitor.position = InvalidExpressionPosition::BaseClass; | ||
| } | ||
| visitor.visit_type_params(type_params); | ||
| if let Some(arguments) = arguments { | ||
| visitor.position = InvalidExpressionPosition::GenericDefinition; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need the same future check here to determine the position or are functions different?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think base classes still allow this, at least with future annotations: >>> from __future__ import annotations
>>> class F(y := list): ...
... def f[T]():
... class G((yield 1)): ...
... class H((yield from 1)): ...
...Although I'm now realizing that this applies to other annotations not considered in this PR: >>> def f():
... x: (yield 1)
...
>>> from __future__ import annotations
>>> def f():
... x: (yield 1)
...
File "<python-input-2>", line 2
x: (yield 1)
^^^^^^^
SyntaxError: yield expression cannot be used within an annotationAnother addition to #11934... |
||
| visitor.visit_arguments(arguments); | ||
| } | ||
| } | ||
|
|
@@ -217,7 +222,6 @@ impl SemanticSyntaxChecker { | |
| // type Y = (yield 1) # yield in value | ||
| // type Y = (x := 1) # named expr in value | ||
| let mut visitor = InvalidExpressionVisitor { | ||
| allow_named_expr: false, | ||
| position: InvalidExpressionPosition::TypeAlias, | ||
| ctx, | ||
| }; | ||
|
|
@@ -625,12 +629,6 @@ impl Display for SemanticSyntaxError { | |
| write!(f, "cannot delete `__debug__` on Python {python_version} (syntax was removed in 3.9)") | ||
| } | ||
| }, | ||
| SemanticSyntaxErrorKind::InvalidExpression( | ||
| kind, | ||
| InvalidExpressionPosition::BaseClass, | ||
| ) => { | ||
| write!(f, "{kind} cannot be used as a base class") | ||
| } | ||
| SemanticSyntaxErrorKind::InvalidExpression(kind, position) => { | ||
| write!(f, "{kind} cannot be used within a {position}") | ||
| } | ||
|
|
@@ -858,7 +856,6 @@ pub enum InvalidExpressionPosition { | |
| TypeVarTupleDefault, | ||
| ParamSpecDefault, | ||
| TypeAnnotation, | ||
| BaseClass, | ||
| GenericDefinition, | ||
| TypeAlias, | ||
| } | ||
|
|
@@ -872,7 +869,6 @@ impl Display for InvalidExpressionPosition { | |
| InvalidExpressionPosition::ParamSpecDefault => "ParamSpec default", | ||
| InvalidExpressionPosition::TypeAnnotation => "type annotation", | ||
| InvalidExpressionPosition::GenericDefinition => "generic definition", | ||
| InvalidExpressionPosition::BaseClass => "base class", | ||
| InvalidExpressionPosition::TypeAlias => "type alias", | ||
| }) | ||
| } | ||
|
|
@@ -1086,16 +1082,6 @@ impl<'a, Ctx: SemanticSyntaxContext> MatchPatternVisitor<'a, Ctx> { | |
| } | ||
|
|
||
| struct InvalidExpressionVisitor<'a, Ctx> { | ||
| /// Allow named expressions (`x := ...`) to appear in annotations. | ||
| /// | ||
| /// These are allowed in non-generic functions, for example: | ||
| /// | ||
| /// ```python | ||
| /// def foo(arg: (x := int)): ... # ok | ||
| /// def foo[T](arg: (x := int)): ... # syntax error | ||
| /// ``` | ||
| allow_named_expr: bool, | ||
|
|
||
| /// Context used for emitting errors. | ||
| ctx: &'a Ctx, | ||
|
|
||
|
|
@@ -1108,7 +1094,7 @@ where | |
| { | ||
| fn visit_expr(&mut self, expr: &Expr) { | ||
| match expr { | ||
| Expr::Named(ast::ExprNamed { range, .. }) if !self.allow_named_expr => { | ||
| Expr::Named(ast::ExprNamed { range, .. }) => { | ||
| SemanticSyntaxChecker::add_error( | ||
| self.ctx, | ||
| SemanticSyntaxErrorKind::InvalidExpression( | ||
|
|
@@ -1166,6 +1152,9 @@ pub trait SemanticSyntaxContext { | |
| /// Returns `true` if a module's docstring boundary has been passed. | ||
| fn seen_docstring_boundary(&self) -> bool; | ||
|
|
||
| /// Returns `true` if `__future__`-style type annotations are enabled. | ||
| fn future_annotations_or_stub(&self) -> bool; | ||
|
|
||
| /// The target Python version for detecting backwards-incompatible syntax changes. | ||
| fn python_version(&self) -> PythonVersion; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These ones should perhaps stay as errors. They are a syntax error in Python 3.14 (and under
from __future__ import annotations). They're legal otherwise in older versions, but yielding in an annotation is a very questionable idea and it will be a SyntaxError in the future, so it would make sense for Ruff to error about it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, interesting. We can check the version before emitting these errors, but I don't think we have any others that depend on
from __future__ import annotationsyet. It does seem easiest just to make this an error on any version in that case. Thanks!And it looks like my error message was very similar to CPython in that case too:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference: https://peps.python.org/pep-0649/#changes-to-allowable-annotations-syntax
Up to you how to handle this in Ruff. You could raise a SyntaxError on all versions for simplicity (pro: helps prepare people for the error, simpler to implement; con: not strictly accurate on 3.13 and earlier, might have false positives in the unlikely case people are actually using
yieldetc. in annotations). You could also add a new error code specifically for this case, so people can ignore it if they want. Or you can just leave it as in this PR for now, and make it a SyntaxError for >3.14 only once you add 3.14 support.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh is this the same for named expressions too? I had already skipped those in the initial PR because they were allowed in 3.13:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and
await. (The comment above was written before I saw your reply.)