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

Use Never instead of None for stores #13984

Merged
merged 1 commit into from
Oct 30, 2024
Merged

Use Never instead of None for stores #13984

merged 1 commit into from
Oct 30, 2024

Conversation

charliermarsh
Copy link
Member

Summary

See: #13981 (comment)

@charliermarsh charliermarsh marked this pull request as ready for review October 30, 2024 02:18
@@ -93,7 +92,7 @@ reveal_type(d) # revealed: Unknown
### Starred expression (2)

```py
[a, *b, c] = (1, 2) # error: "Object of type `None` is not iterable"
[a, *b, c] = (1, 2)
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm having trouble tracing this -- where is this diagnostic even coming from today?

Copy link
Member

Choose a reason for hiding this comment

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

What's the issue? Is this failing? I think that's probably coming from infer_starred_expression

Copy link
Member Author

Choose a reason for hiding this comment

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

The diagnostic is no longer showing up, so I had to remove these assertions along with the TODOs to remove them (?).

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think that's correct. The diagnostics shouldn't be present in these cases and I think I left out one TODO in this specific case which might've confused you (?). The reason these diagnostics were showing up in the first place is because red knot tries to infer the starred expression (*b) but that isn't supported yet, before this PR it would've returned None and then it would raise a "not-an-iterable" diagnostic. But, it's now changed to use Never in this PR.

let iterable_ty = self.infer_expression(value);
iterable_ty
.iterate(self.db)
.unwrap_with_diagnostic(value.as_ref().into(), self);

Copy link
Contributor

Choose a reason for hiding this comment

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

This PR shouldn't even affect this case (we should never use the type of a Store-context expression), but it does just because we don't support assigning to Starred yet, and the way the temporary code works out, it does currently try to iterate the inferred type of a Store-context expression. But not throwing the error is better, so if anything switching from None to Never is an improvement here.

Copy link
Contributor

ruff-ecosystem results

Linter (stable)

ℹ️ ecosystem check encountered linter errors. (no lint changes; 1 project error)

pandas-dev/pandas (+0 -0 violations, +0 -0 fixes)


pypa/setuptools (error)

ruff failed
  Cause: Failed to parse /home/runner/work/ruff/ruff/checkouts/pypa:setuptools/ruff.toml
  Cause: TOML parse error at line 1, column 11
  |
1 | include = "pyproject.toml"
  |           ^^^^^^^^^^^^^^^^
invalid type: string "pyproject.toml", expected a sequence

Linter (preview)

ℹ️ ecosystem check encountered linter errors. (no lint changes; 1 project error)

pypa/setuptools (error)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

ruff failed
  Cause: Failed to parse /home/runner/work/ruff/ruff/checkouts/pypa:setuptools/ruff.toml
  Cause: TOML parse error at line 1, column 11
  |
1 | include = "pyproject.toml"
  |           ^^^^^^^^^^^^^^^^
invalid type: string "pyproject.toml", expected a sequence

@@ -93,7 +92,7 @@ reveal_type(d) # revealed: Unknown
### Starred expression (2)

```py
[a, *b, c] = (1, 2) # error: "Object of type `None` is not iterable"
[a, *b, c] = (1, 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

This PR shouldn't even affect this case (we should never use the type of a Store-context expression), but it does just because we don't support assigning to Starred yet, and the way the temporary code works out, it does currently try to iterate the inferred type of a Store-context expression. But not throwing the error is better, so if anything switching from None to Never is an improvement here.

@charliermarsh charliermarsh merged commit b1ce8a3 into main Oct 30, 2024
20 checks passed
@charliermarsh charliermarsh deleted the charlie/never branch October 30, 2024 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants