Skip to content
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

Allow assigning ellipsis literal as parameter default value #14982

Merged
merged 38 commits into from
Jan 5, 2025

Conversation

Glyphack
Copy link
Contributor

@Glyphack Glyphack commented Dec 15, 2024

Resolves #14840

Summary

Usage of ellipsis literal as default argument is allowed in stub files.

Test Plan

Added mdtest for both python files and stub files.

Copy link
Contributor

github-actions bot commented Dec 15, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@AlexWaygood AlexWaygood added the red-knot Multi-file analysis & type inference label Dec 15, 2024
@AlexWaygood
Copy link
Member

The pre-commit failure will go away if you merge in main or rebase :-)

@Glyphack Glyphack force-pushed the ellipsis-default-type branch from 8157331 to a7e7723 Compare December 15, 2024 21:23
@Glyphack
Copy link
Contributor Author

Wow there's a bug in the new github UI for PRs it was not showing the rebase button I thought it's already up to date.

@Glyphack Glyphack marked this pull request as ready for review December 15, 2024 21:26
@AlexWaygood
Copy link
Member

Wow there's a bug in the new github UI for PRs it was not showing the rebase button I thought it's already up to date.

Oh, that only appears for a repository if the maintainers have checked a certain box in the repository settings on GitHub. We haven't enabled it for Astral repositories because the rate of development is quite rapid, and some folks found it annoying to be constantly told that all their PRs were out of date with the main branch 😆

Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!!

@Glyphack
Copy link
Contributor Author

Glyphack commented Dec 16, 2024

Thanks for the review.
I have a question, pyright behavior here is that in all files the assignment works only if the function body is also ellipsis. The link does not specify if it's stub file but pasting this into a python file locally shows the same warnings.
https://pyright-play.net/?strict=true&code=CYUwZgBGAUAeBcECWA7ALhAvBAdHglBALQB8EAcgPYojwBQEjEATiGgK7MoSx12iQYCZOiy4CxMlRqI8OIA
Mypy does the same. Is this just a limitation of them or the specs are not strictly forbidding this?

@Glyphack Glyphack force-pushed the ellipsis-default-type branch from 4d835da to 06e1551 Compare December 16, 2024 21:37
@AlexWaygood
Copy link
Member

AlexWaygood commented Dec 16, 2024

This special case is only allowed in function and method arguments. Based on what I understand from typing.readthedocs.io/en/latest/guides/writing_stubs.html#module-level-attributes and typing.readthedocs.io/en/latest/guides/writing_stubs.html#classes.

Do not unnecessarily use an assignment for module-level attributes.

Ah, I think there's a small misunderstanding here. There are two halves of typing.readthedocs.io:

  • One half of the website (https://typing.readthedocs.io/en/latest/spec/) is a formal typing spec: this is a set of rules that type checkers must follow if they claim to support all features of Python's typing system
  • The other half of the website (https://typing.readthedocs.io/en/latest/guides/) is a set of guides for users on how they should use annotations in their own runtime code and stub files. These are guides to best practices, but they aren't necessarily things that type checkers need to enforce

The two pages you link to there are both in the "second half" of the website: those paragraphs describe best practices for people writing stub files, but these aren't necessarily rules that type checkers have to enforce regarding stub files. The typing spec part of typing.readthedocs.io is here: https://typing.readthedocs.io/en/latest/spec/distributing.html#value-expressions. It states that type checkers should allow ... as a value in many more places, even if the "guides" part of typing.readthedocs.io states that it wouldn't necessarily be best practice for users to do so. Here's what the spec has to say:

In locations where value expressions can appear, such as the right-hand side of assignment statements and function parameter defaults, type checkers should support the following expressions:

  • The ellipsis literal, ..., which can stand in for any value
    -Any value that is a legal parameter for typing.Literal
  • Floating point literals, such as 3.14
  • Complex literals, such as 1 + 2j

And later:

Type checkers should support module-level variable annotations, with and without assignments:

x: int
x: int = 0
x = 0  # type: int
x = ...  # type: int

@AlexWaygood
Copy link
Member

So I think we should allow ... as a value for any annotated symbol in stub files, in any context, not just parameter defaults

@carljm
Copy link
Contributor

carljm commented Dec 16, 2024

Thanks for that clarification, Alex! I was surprised that x: int = ... wouldn't be supported in a stub, and I read the links without noticing they weren't part of the spec.

I feel like the wording of the spec:

In locations where value expressions can appear... type checkers should support the following expressions: ... The ellipsis literal, ..., which can stand in for any value

Suggests a totally different implementation approach here, which is that in a stub file we should simply infer literal ... expressions as type Any, which is assignable to anything. That seems better than trying to handle this at each point where we enforce assignability.

@carljm
Copy link
Contributor

carljm commented Dec 16, 2024

I have a question, pyright behavior here is that in all files the assignment works only if the function body is also ellipsis ... Mypy does the same

That's very interesting, good observation! I don't actually know why that is. It may be in order to support ellipsis defaults in overloads and protocol methods. I'm a little surprised that this appears to be handled by allowing it based on the body being ..., rather than just allowing it contextually for overloads and protocols.

I'm not convinced we want to follow suit here; it seems incorrect to do this based on a body of ..., considering that is a valid body for a regular Python function.

So I would say let's implement this only for stubs for now, and consider the best approach when we add overloads/protocols support.

@AlexWaygood
Copy link
Member

Suggests a totally different implementation approach here, which is that in a stub file we should simply infer literal ... expressions as type Any, which is assignable to anything. That seems better than trying to handle this at each point where we enforce assignability.

That seems okay to me as long as we don't start inferring unions with Any for things imported from stub files... but I don't think we will, because declared types take precedence over inferred types for things imported from other modules, right?

@carljm
Copy link
Contributor

carljm commented Dec 16, 2024

That seems okay to me as long as we don't start inferring unions with Any for things imported from stub files... but I don't think we will, because declared types take precedence over inferred types for things imported from other modules, right?

Yes, that's right.

@Glyphack
Copy link
Contributor Author

Thanks let me do try to change this to infer the type of ellipsis literal as Any instead of adding this special case.

@carljm
Copy link
Contributor

carljm commented Dec 16, 2024

Suggests a totally different implementation approach here, which is that in a stub file we should simply infer literal ... expressions as type Any, which is assignable to anything. That seems better than trying to handle this at each point where we enforce assignability.

Ok, after further discussion (thanks @AlexWaygood!) I think this is not a good idea, and might cause undesirable behavior in examples like this:

class Foo: ...

X: int = ...
Y: Foo = X

Where this should be flagged as an error, even in a type stub, but my suggestion (combined with our current handling of inferred dynamic type) would not consider it an error. (It might be worth including this as a test case.)

So I think the better approach is in fact to special-case handling of literal ... in three different places: in function parameters, where you're doing it now; in infer_annotated_assignment_definition, and in infer_assignment_definition.

In the first two cases, if we see a literal ... as the default-value or RHS, in a stub file, we should just discard that inference and record the declared type as the inferred type. In the third case, there is no declaratiion and we should record Unknown.

@Glyphack Glyphack force-pushed the ellipsis-default-type branch from 06e1551 to 9689544 Compare December 19, 2024 18:06
Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! A few comments.

crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved
annotation_ty,
value_ty,
);
if self.file().is_stub(self.db().upcast()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we've forgotten the condition "and the value expression is a literal ..." here. If that doesn't fail any test, we should definitely add a test with e.g. x: int = 1 in a stub file showing we still infer Literal[1] for x, not Unknown.

Copy link
Contributor Author

@Glyphack Glyphack Dec 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it was not failing. Added a new test for it in mdtest/assignment/annotations.md.

crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved
@Glyphack Glyphack marked this pull request as draft December 20, 2024 09:57
@Glyphack Glyphack force-pushed the ellipsis-default-type branch from acf3072 to d30649c Compare December 30, 2024 11:45
@Glyphack Glyphack force-pushed the ellipsis-default-type branch from d499b43 to 2be530d Compare December 31, 2024 14:57
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks close! A few comments below

Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, modulo a couple small nits

crates/red_knot_python_semantic/src/types/unpacker.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types/unpacker.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you!

@carljm carljm merged commit b264489 into astral-sh:main Jan 5, 2025
21 checks passed
@Glyphack Glyphack deleted the ellipsis-default-type branch January 5, 2025 19:43
@Glyphack
Copy link
Contributor Author

Glyphack commented Jan 5, 2025

Thanks for your time and review.

dcreager added a commit that referenced this pull request Jan 6, 2025
* main: (60 commits)
  [`ruff`] Dataclass enums (`RUF049`) (#15299)
  Better error message when `--config` is given a table key and a non-inline-table value (#15266)
  Update pre-commit dependencies (#15289)
  Don't fix in ecosystem check (#15267)
  Update Rust crate itertools to 0.14.0 (#15287)
  Remove accidental empty block at the bottom of `split-static-string (SIM905)` doc (#15290)
  Update Rust crate clearscreen to v4 (#15288)
  Update Rust crate insta to v1.42.0 (#15286)
  Update NPM Development dependencies (#15285)
  Update dependency uuid to v11.0.4 (#15284)
  Update dependency ruff to v0.8.6 (#15283)
  Update Rust crate syn to v2.0.95 (#15282)
  Update Rust crate matchit to v0.8.6 (#15281)
  Update Rust crate bstr to v1.11.3 (#15280)
  [red-knot] Future-proof `Type::is_disjoint_from()` (#15262)
  [red-knot] Improve `Type::is_disjoint_from()` for `KnownInstanceType`s (#15261)
  [red-knot] Minor simplifications and improvements to constraint narrowing logic (#15270)
  Allow assigning ellipsis literal as parameter default value (#14982)
  [red-knot] fix control flow for assignment expressions in elif tests (#15274)
  [`refurb`] Mark fix as unsafe when the right-hand side is a string (`FURB171`) (#15273)
  ...
dcreager added a commit that referenced this pull request Jan 6, 2025
* main: (29 commits)
  [`ruff`] Dataclass enums (`RUF049`) (#15299)
  Better error message when `--config` is given a table key and a non-inline-table value (#15266)
  Update pre-commit dependencies (#15289)
  Don't fix in ecosystem check (#15267)
  Update Rust crate itertools to 0.14.0 (#15287)
  Remove accidental empty block at the bottom of `split-static-string (SIM905)` doc (#15290)
  Update Rust crate clearscreen to v4 (#15288)
  Update Rust crate insta to v1.42.0 (#15286)
  Update NPM Development dependencies (#15285)
  Update dependency uuid to v11.0.4 (#15284)
  Update dependency ruff to v0.8.6 (#15283)
  Update Rust crate syn to v2.0.95 (#15282)
  Update Rust crate matchit to v0.8.6 (#15281)
  Update Rust crate bstr to v1.11.3 (#15280)
  [red-knot] Future-proof `Type::is_disjoint_from()` (#15262)
  [red-knot] Improve `Type::is_disjoint_from()` for `KnownInstanceType`s (#15261)
  [red-knot] Minor simplifications and improvements to constraint narrowing logic (#15270)
  Allow assigning ellipsis literal as parameter default value (#14982)
  [red-knot] fix control flow for assignment expressions in elif tests (#15274)
  [`refurb`] Mark fix as unsafe when the right-hand side is a string (`FURB171`) (#15273)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[red-knot] ... should be treated as a valid parameter default for any annotation in a stub file
5 participants