Skip to content

[red-knot] Add initial set of tests for unreachable code #17159

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

Merged
merged 1 commit into from
Apr 2, 2025

Conversation

sharkdp
Copy link
Contributor

@sharkdp sharkdp commented Apr 2, 2025

Summary

Add an initial set of tests that will eventually document our behavior around unreachable code. In the last section of this suite, I argue why we should never type check unreachable sections and never emit any diagnostics in these sections.

@sharkdp sharkdp added the red-knot Multi-file analysis & type inference label Apr 2, 2025
@sharkdp sharkdp force-pushed the david/unreachable-code-tests branch from aae6786 to ee6e92b Compare April 2, 2025 16:42
Copy link
Contributor

github-actions bot commented Apr 2, 2025

mypy_primer results

No ecosystem changes detected ✅

Comment on lines +241 to +242
inside the unreachable section would not cause problems at runtime. And type checking the
unreachable code under the assumption that it *is* reachable might lead to false positives:
Copy link
Member

Choose a reason for hiding this comment

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

I like that reasoning. I assume that only means we won't emit diagnostics. Would we still infer types for those sections?

It will be interesting to see how we integrate this into the linter... but that's a problem for another day

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can still infer types for unreachable sections. Whether it's useful is less clear; you are likely to see a lot of Never types, since no binding from outside the unreachable code section can reach the unreachable code section.

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.

This all looks right to me.

Comment on lines +241 to +242
inside the unreachable section would not cause problems at runtime. And type checking the
unreachable code under the assumption that it *is* reachable might lead to false positives:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can still infer types for unreachable sections. Whether it's useful is less clear; you are likely to see a lot of Never types, since no binding from outside the unreachable code section can reach the unreachable code section.

@carljm carljm linked an issue Apr 2, 2025 that may be closed by this pull request
@carljm
Copy link
Contributor

carljm commented Apr 2, 2025

I suspect that a lot of "don't emit diagnostics in unreachable code" will fall out naturally from the prevalence of Never types, because Never (if we've correctly implemented it) is a very forgiving type: it is a subtype of everything, assignable to everything, has all attributes and methods (all of type Never), etc.

In fact it may be better to let it fall out like this than to implement a blanket policy of "no diagnostics in unreachable code". An argument could be made that in the cases where there is a binding inside the unreachable code section, we should still emit diagnostics on invalid uses of that binding. For example, if x = 3; y = x + "foo" all occurs within unreachable code, there is no risk of false positive by still erroring on the bad binary operation. (The counter-argument would be the one mentioned in this PR: it can't cause issues at runtime. But I find that to be a less-convincing argument than the false-positive one. It can be useful to emit diagnostics for issues that cannot occur at runtime, if they would occur at runtime given some other change. The sys.version_info cases are sort of an example of this.)

Unresolved-reference is an exception here; we definitely have to silence this in unreachable code, as a special case.

@MichaReiser
Copy link
Member

MichaReiser commented Apr 2, 2025

@carljm we may need special handling at least for # type: ignore or we'll get false-positives.

@carljm
Copy link
Contributor

carljm commented Apr 2, 2025

Yeah, "unused type-ignore" is another case, like unresolved-reference, that might need special handling.

@AlexWaygood
Copy link
Member

Unresolved-reference is an exception here

I'm not at all sure we can predict which error codes are going to have false positives in unreachable code. This PR also has examples of unresolved-attribute and unresolved-import false-positive diagnostics occuring in unreachable code; there may be others, too.

@carljm
Copy link
Contributor

carljm commented Apr 2, 2025

That may be true, but I don't think adding unresolved-attribute and unresolved-import to the list is strong evidence for that conclusion. Those are just variants of unresolved-reference; they're all issues about boundness, rather than type violations.

@AlexWaygood
Copy link
Member

AlexWaygood commented Apr 2, 2025

Hmm, I suppose so. Are we sure we won't add more error codes in the future that are similarly issues around boundness, though? It feels somewhat fragile to special-case specific error codes.

Anyway, we can iterate on it!

@sharkdp sharkdp merged commit cb7dae1 into main Apr 2, 2025
22 checks passed
@sharkdp sharkdp deleted the david/unreachable-code-tests branch April 2, 2025 17:39
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.

This is great, thanks!!

dcreager added a commit that referenced this pull request Apr 3, 2025
* origin/main: (35 commits)
  [red-knot] Callable types are disjoint from literals (#17160)
  [red-knot] Fix inference for `pow` between two literal integers (#17161)
  [red-knot] Add GitHub PR annotations when mdtests fail in CI (#17150)
  [red-knot] Fix equivalence of differently ordered unions that contain `Callable` types (#17145)
  [red-knot] Add initial set of tests for unreachable code (#17159)
  [`airflow`] Move `AIR302` to `AIR301` and `AIR303` to `AIR302` (#17151)
  ruff_db: simplify lifetimes on `DiagnosticDisplay`
  [red-knot] Detect division-by-zero in unions and intersections (#17157)
  [`airflow`] Add autofix infrastructure to `AIR302` name checks (#16965)
  [`flake8-bandit`] Mark `str` and `list[str]` literals as trusted input (`S603`) (#17136)
  [`airflow`] Add autofix for `AIR302` attribute checks (#16977)
  [`airflow`] Extend `AIR302` with additional symbols (#17085)
  [`airflow`] Move `AIR301` to `AIR002` (#16978)
  [`airflow`] Add autofix for `AIR302` method checks (#16976)
  ruff_db: switch diagnostic rendering over to `std::fmt::Display`
  [red-knot] Add 'Goto type definition' to the playground (#17055)
  red_knot_ide: update snapshots
  red_knot_python_semantic: remove comment about `TypeCheckDiagnostic`
  ruff_db: delete most of the old diagnostic code
  red_knot: use `Diagnostic` inside of red knot
  ...
dcreager added a commit that referenced this pull request Apr 3, 2025
* origin/main: (82 commits)
  [red-knot] Fix more [redundant-cast] false positives (#17170)
  [red-knot] Three-argument type-calls take 'str' as the first argument (#17168)
  Control flow: `return` and `raise` (#17121)
  Bump 0.11.3 (#17173)
  [red-knot] Improve `Debug` implementation for `semantic_index::SymbolTable` (#17172)
  [red-knot] Fix `str(…)` calls (#17163)
  [red-knot] visibility_constraint analysis for match cases (#17077)
  [red-knot] Fix playground crashes when diagnostics are stale (#17165)
  [red-knot] Callable types are disjoint from literals (#17160)
  [red-knot] Fix inference for `pow` between two literal integers (#17161)
  [red-knot] Add GitHub PR annotations when mdtests fail in CI (#17150)
  [red-knot] Fix equivalence of differently ordered unions that contain `Callable` types (#17145)
  [red-knot] Add initial set of tests for unreachable code (#17159)
  [`airflow`] Move `AIR302` to `AIR301` and `AIR303` to `AIR302` (#17151)
  ruff_db: simplify lifetimes on `DiagnosticDisplay`
  [red-knot] Detect division-by-zero in unions and intersections (#17157)
  [`airflow`] Add autofix infrastructure to `AIR302` name checks (#16965)
  [`flake8-bandit`] Mark `str` and `list[str]` literals as trusted input (`S603`) (#17136)
  [`airflow`] Add autofix for `AIR302` attribute checks (#16977)
  [`airflow`] Extend `AIR302` with additional symbols (#17085)
  ...
maxmynter pushed a commit to maxmynter/ruff that referenced this pull request Apr 3, 2025
…7159)

## Summary

Add an initial set of tests that will eventually document our behavior
around unreachable code. In the last section of this suite, I argue why
we should never type check unreachable sections and never emit any
diagnostics in these sections.
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.

4 participants