mirrors/ruff
0
0 Fork 0
mirror of https://github.com/astral-sh/ruff.git synced 2026-10-28 10:45:37 +08:00
Code Activity

[ty] Add rules to detect always-truthy and always-falsy conditions (#28034)

Browse Source 
## Summary

Builds on #28032.

Add two rules for boolean tests whose truthiness is statically known:

- `redundant-condition`, enabled by default at warning severity, catches
cases such as an uncalled function or method, an unawaited coroutine, a
generator tested for emptiness, or a tuple whose length makes its
truthiness constant.
- `redundant-condition-strict`, disabled by default, covers
boolean/integer conditions, conditions whose outcome is fixed only by
short-circuit evaluation, and conditions containing walrus expressions.
These can expose real bugs, but are also much more likely to be
intentional.

The checks reuse ty's truthiness inference and apply to `if`/`elif`,
`while`, assertions, match guards, conditional expressions,
comprehension filters, and unary `not`. The operands of `and`/`or` are
checked when the expression is used as a condition. The main design
question is which statically redundant tests are likely to represent
mistakes. The implementation deliberately prefers missing some redundant
conditions to warning about common defensive or compatibility patterns.

## Avoiding false positives

### Separate likely mistakes from more opinionated checks

Types assignable to `int`, including `bool`, use the strict rule. So do
conditions whose outcome is fixed by short-circuit evaluation even
though their inferred value type has ambiguous truthiness, and tests
containing walrus expressions. Other values with fixed truthiness use
the default rule.

This keeps checks of ordinary boolean/integer flags quiet by default,
because these are extremely common and it is difficult to distinguish
deliberately constant flags from mistakes:

```python
DEBUG = False

if DEBUG:  # Only reported by redundant-condition-strict.
    print("debugging")
```

The strict rule is a sibling of the default rule: it handles a separate
set of expressions rather than reporting the same expressions more
aggressively.

### Distinguish a condition's outcome from its value type

Short-circuit evaluation can establish a fixed outcome even when the
expression's value type does not:

```python
def check(value: object):
    if value and False:  # Always false; reported by redundant-condition-strict.
        print("unreachable")
```

The condition fails whether `value` is truthy or falsy. However, saving
`value and False` to a variable and testing that variable later is
different: the saved value could be the original object, whose
`__bool__` result may change between calls. The redundant-condition
checks use short-circuit truthiness APIs introduced in
https://github.com/astral-sh/ruff/pull/28082 for direct conditions, and
value-type truthiness for tests of computed values. This also handles
chained comparisons whose intermediate results are objects with mutable
truthiness.

### Recognize explicitly constant tests

Both rules exempt direct boolean, integer and `None` literals in the
AST, such as `if False:`, `if 0:`, `while True:`, `while 1:`, and
`assert None`. These spellings are strong evidence that the constant
truthiness is deliberate. A name _inferred_ as `Literal[False]` is
different: its value may not be apparent where it is tested, so it
remains eligible for the strict rule.

### Treat walrus expressions as evidence of intentional side effects

An always-truthy operand can be useful because evaluating it assigns a
value:

```python
if should_continue() and (message := "continuing") and is_ready():
    print(message)
```

The middle operand uses the strict rule, so it does not warn by default.

We does not attempt to classify arbitrary side effects. For example, `if
items.append(value) or fallback:` can still trigger the default rule
because `append` returns `None`.

An `and`/`or` expression used to compute a value (rather than one that
is used directly in an `if` test or similar), such as `result =
items.append(value) or fallback`, is not flagged by either rule.

### Follow the origins of environment-dependent conditions

Both rules exempt compatibility checks involving `sys.version_info`,
`sys.platform`, `os.name`, and `TYPE_CHECKING`. A condition being
constant for the configured environment does not make it redundant
across the environments supported by the project.

The exemption also follows names and attributes through their
definitions, including aliases and imports across modules:

```python
import sys

IS_WINDOWS = sys.platform == "win32"
LINE_PREFIX = "\n" if IS_WINDOWS else ""

if LINE_PREFIX:  # Exempt even though its truthiness is known on this platform.
    print(LINE_PREFIX)
```

The traversal follows assignment values, augmented assignments, walrus
bindings, loop and comprehension iterables, context-manager expressions,
and the subjects of captured `match` patterns.

This lookup is deliberately not flow-sensitive: if any resolved
definition depends on one of these symbols, the use is exempt. This is
so that we err on the side of avoiding false positives.

The shared definition-resolution machinery from #28032 allows this
traversal during inference without requesting completed inference of the
same scope. Name lookups are cached per scope and name; attribute
lookups are cached per receiver type and attribute name. Definition
traversal has its own Salsa query boundary and handles cyclic aliases,
so repeated conditions can share the provenance analysis.

### Preserve defensive assertions and exhaustive dispatch

Defensive boolean/integer assertions such as `assert isinstance(value,
int)` remain useful at runtime even when `value` is annotated as `int`,
since the runtime does not enforce type annotations. These are therefore
exempt from both rules.

Non-boolean mistakes such as `assert function` (where the user meant
`assert function()`) are still reported by the default rule. A complete
assertion whose value type has ambiguous truthiness but whose outcome is
fixed by short-circuit evaluation remains eligible for the strict rule.

For `if` and `elif` statements, the strict rule also recognizes
defensive exits. An always-false condition is exempt when its body ends
in a `raise`, an assertion that might fail, a call returning `Never`, or
a return assignable to `NotImplementedType`:

```python
def increment(value: int) -> int:
    if not isinstance(value, int):
        raise TypeError("expected an int")
    return value + 1
```

For an always-true condition, the same heuristic examines an immediately
following `else` suite, or the remainder of the surrounding suite when
there are no later clauses. This accommodates exhaustive dispatch
followed by `assert_never`, and early-return dispatch followed by a
defensive exception.

These defensive-exit exemptions apply to boolean/integer conditions and
conditions requiring short-circuit analysis. An uncalled function is
still suspicious even when an otherwise unreachable branch raises.

### Check compound conditions once

In `result = flag and function`, Python does not test the truthiness of
the final operand: it simply returns that value if evaluation reaches
it. The rule therefore does not flag `function`, even though it is an
always-truthy object in a boolean expression. In `if flag and
predicate:`, the surrounding `if` does test the truthiness of
`predicate`, so the rule reports the uncalled function even though the
complete condition has ambiguous truthiness.

Unlike `and`/`or` expressions used to compute values, `not` explicitly
tests its operand's truthiness even when it used to computer a value, so
`not` expressions are checked even outside the context of
`if`/`while`/`elif`/`assert` tests.

For compound conditions, the redundant-condition checks visit operands
before deciding whether to report the complete expression. Reporting an
uncalled function in `if not function:` suppresses a second duplicate
diagnostic on the `not function` expression as a whole. Boolean/integer
operands and operands requiring short-circuit analysis are instead
suppressed so that the complete condition can be reported once. This
applies consistently to statement conditions, conditional expressions,
and comprehension filters.

Boolean tests inside value expressions, such as `if consume(not
function):`, are checked independently. Tests in nested scopes are
checked when those scopes are inferred.

Neither rule runs in stub files or files excluded from checking.
Diagnostic and provenance analysis is skipped entirely when both rules
are disabled.

## Diagnostics and fixes

Special-case diagnostics are provided to help explain some of the more
common errors we see in the ecosystem: an uncalled function or method
being always truthy, a fixed-length tuple being always truthy or always
falsy, a `TypedDict` always being truthy due to the presence of
`Required` fields, a `@final` class without `__bool__` or `__len__`
being always truthy, or an unevaluated generator being always truthy.

For simple function and method references, the diagnostic carries a call
fix. A parameterless signature gets an unsafe `()` fix; signatures with
parameters get a display-only `(...)` suggestion. Generator diagnostics
suggest `any()` and carry a display-only wrapping fix when the standard
`any` builtin is known not to be shadowed, including by a custom
`__builtins__.pyi`.

These fixes are deliberately not classified as safe: calling a function
or consuming a generator can change runtime behavior, and the intended
correction may be different.

### Default-rule diagnostics

<details>
<summary>Uncalled function</summary>

![Uncalled
function](https://github.com/user-attachments/assets/9538e4a9-b716-4fff-a4c7-24fc6deb7994)

</details>

<details>
<summary>Uncalled method</summary>

![Uncalled
method](https://github.com/user-attachments/assets/3c327ada-eec0-4bcc-9588-e5df50b3455b)

</details>

<details>
<summary>Generator: suggest <code>any()</code></summary>

![Generator: suggest
any()](https://github.com/user-attachments/assets/83d500fb-d69a-4753-94d6-4008379af8d8)

</details>

<details>
<summary>Tuple with a fixed length</summary>

![Tuple with a fixed
length](https://github.com/user-attachments/assets/e37ade52-22a6-4946-aa29-71f5328bfa79)

</details>

<details>
<summary>Tuple with a minimum length</summary>

![Tuple with a minimum
length](https://github.com/user-attachments/assets/6fc71bd5-6a52-417f-9a9b-db4e760752c3)

</details>

<details>
<summary>Empty tuple</summary>

![Empty
tuple](https://github.com/user-attachments/assets/6c8e8242-fd8b-410d-b56b-e6a49737ea0f)

</details>

<details>
<summary>Nonempty string literals</summary>

![Nonempty string
literals](https://github.com/user-attachments/assets/139f9f53-5128-4d85-8901-01937f312986)

</details>

<details>
<summary>Empty string</summary>

![Empty
string](https://github.com/user-attachments/assets/261fc16d-dc67-44b8-bf51-58f2fcb12e2f)

</details>

<details>
<summary><code>None</code></summary>


![None](https://github.com/user-attachments/assets/c04b0308-b294-4125-bba8-3603fd5bda02)

</details>

<details>
<summary><code>TypedDict</code>: required fields and their
declarations</summary>

![TypedDict: required fields and their
declarations](https://github.com/user-attachments/assets/46367c59-665f-4b40-893b-227add4ca3df)

</details>

<details>
<summary>Final class without <code>__bool__</code> or
<code>__len__</code></summary>

![Final class without __bool__ or
__len__](https://github.com/user-attachments/assets/4e8e92e7-0645-485c-ab30-4d0807943005)

</details>

<details>
<summary>Enum: implicit finality</summary>

![Enum: implicit
finality](https://github.com/user-attachments/assets/6596a69b-a122-40bb-80fc-b011c054e15d)

</details>

### Strict-rule diagnostics

These examples enable `redundant-condition-strict` explicitly.

<details>
<summary>Indexing <code>bytes</code> returns <code>int</code></summary>

![Indexing bytes returns
int](https://github.com/user-attachments/assets/e6f33bc9-c998-4be5-b1ca-36c32b073d99)

</details>

<details>
<summary>Comparing <code>str</code> with <code>bytes</code></summary>

![Comparing str with
bytes](https://github.com/user-attachments/assets/48e15215-d6ae-4779-ab44-244c21dabf55)

</details>

<details>
<summary>Length comparison: explain the fixed tuple length</summary>

![Length comparison: explain the fixed tuple
length](https://github.com/user-attachments/assets/556ef251-ac2f-4048-b7f4-b545a8b87a69)

</details>

<details>
<summary>Comparison: omit an obvious literal's type annotation</summary>

![Comparison: omit an obvious literal's type
annotation](https://github.com/user-attachments/assets/95a0e73f-38f5-457b-9726-8169016b991d)

</details>

## Implementation

The semantic index records the outermost boolean tests in each scope, so
one check owns the diagnostics for a complete condition and its
subexpressions. A shared checker separates classification, context
exemptions, and duplicate suppression from diagnostic rendering. The
main implementation is in
`types/infer/builder/redundant_conditions/mod.rs`; its `diagnostic.rs`
submodule constructs messages and fixes, and `exemptions.rs` handles
environment provenance and defensive exits.
This commit is contained in:
Alex Waygood authored and GitHub committed 2026-09-05 20:45:59 +01:00
1 parent c6c7f93a57
commit 689a33fd36
37 files changed
+5364 -479
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

.github/ty-ecosystem.toml
+1
View File
@@ -15,3 +15,4 @@ unsound-assignment = "warn"
unsound-return-statement = "warn"
unsound-yield = "warn"
unsupported-dynamic-base = "warn"
redundant-condition-strict = "warn"
crates/ruff_diagnostics/src/fix.rs
+12
View File
@@ -104,6 +104,18 @@ impl Fix {
}
}
// Create a new [`Fix`] from multiple [`Edit`] elements, but which should only [display](Applicability::DisplayOnly)
// and not apply.
pub fn display_only_edits(edit: Edit, rest: impl IntoIterator<Item = Edit>) -> Self {
let mut edits: Vec<Edit> = std::iter::once(edit).chain(rest).collect();
edits.sort_by_key(|edit| (edit.start(), edit.end()));
Self {
edits,
applicability: Applicability::DisplayOnly,
isolation_level: IsolationLevel::default(),
}
}
/// Create a new [`Fix`] with the specified [`Applicability`] to apply an [`Edit`] element.
pub fn applicable_edit(edit: Edit, applicability: Applicability) -> Self {
Self {
crates/ty/docs/rules.md
Generated
+704 -122
View File
File diff suppressed because it is too large. Load diff
crates/ty_python_core/src/builder.rs
+105 -66
View File
@@ -12,7 +12,9 @@ use ruff_db::source::{SourceText, source_text};
use ruff_index::IndexVec;
use ruff_python_ast::name::Name;
use ruff_python_ast::visitor::{Visitor, walk_expr, walk_keyword, walk_pattern, walk_stmt};
use ruff_python_ast::{self as ast, AtomicNodeIndex, NodeIndex, PySourceType, PythonVersion};
use ruff_python_ast::{
self as ast, AtomicNodeIndex, HasNodeIndex, NodeIndex, PySourceType, PythonVersion,
};
use ruff_python_parser::semantic_errors::{
LazyImportContext, SemanticSyntaxChecker, SemanticSyntaxContext, SemanticSyntaxError,
SemanticSyntaxErrorKind, YieldOutsideFunctionKind,
@@ -38,7 +40,7 @@ use crate::definition::{
MatchPatternDefinitionNodeRef, NestedBindingExecution, NestedBindingsDefinitionKind,
ParameterDefinitionNodeRef, StarImportDefinitionNodeRef, WithItemDefinitionNodeRef,
};
use crate::expression::{Expression, ExpressionKind};
use crate::expression::{Expression, ExpressionContext, ExpressionKind};
use crate::frozen::{FrozenMap, FrozenSet};
use crate::member::MemberExprBuilder;
use crate::place::{
@@ -49,7 +51,8 @@ use crate::predicate::{
CallableAndCallExpr, ClassPatternKeywordPredicateKind, ClassPatternPredicateKind,
MappingPatternEntryPredicateKind, MappingPatternPredicateKind, PatternPredicate,
PatternPredicateKind, Predicate, PredicateNode, PredicateOrLiteral, ScopedPredicateId,
SequencePatternPredicateKind, StarImportPlaceholderPredicate, SubjectElementPatternPredicate,
SequencePatternPredicateKind, StarImportPlaceholderPredicate, StatementCall,
SubjectElementPatternPredicate,
};
use crate::re_exports::exported_names;
use crate::reachability_constraints::{
@@ -229,39 +232,6 @@ impl ConditionFlowSnapshot {
}
}
/// Whether evaluation produces a result object or chooses a control-flow path.
///
/// In `Value` context, the enclosing code receives the expression's result object. For example,
/// `result = x and y` produces `x` if `x` is falsy, or `y` otherwise. This also applies to expressions
/// that return `bool`: the comparison in `result = x > 0` has value context.
///
/// In `Condition` context, the enclosing code only needs to know which branch to take. For example,
/// CPython evaluates `if x and y` by testing `x` and, only if `x` is truthy, testing `y`. If `x` tests
/// falsy, that one truthiness check is enough to skip the body: `x` is not tested again as the
/// result of `x and y`.
///
/// This distinction matters when an operand's `__bool__` can change between calls:
///
/// ```python
/// if x and False: # A falsy x skips the body; a truthy x reaches False.
/// ... # Unreachable in either case.
/// saved = x and False # Can produce x after checking that it is falsy.
/// if saved: # Can call x.__bool__ again, which may now return True.
/// ... # Reachable.
/// ```
///
/// The context propagates through `and`, `or`, `not`, and the branches of conditional expressions.
/// Condition context does not propagate through calls or assignment expressions: in
/// `if f(x and False)`, the call's result controls the branch, but its argument is evaluated in
/// value context.
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
enum ExpressionContext {
/// Produce the expression's result object for the enclosing code to use.
Value,
/// Choose the truthy or falsy control-flow path without preserving the result object.
Condition,
}
pub(super) struct SemanticIndexBuilder<'db, 'ast> {
// Builder state
db: &'db dyn Db,
@@ -280,6 +250,11 @@ pub(super) struct SemanticIndexBuilder<'db, 'ast> {
/// The name of the first function parameter of the innermost function that we're currently visiting.
current_first_parameter_name: Option<&'ast str>,
/// The scope of a boolean test whose subexpressions we are currently visiting.
///
/// See [`Self::visit_boolean_test`] for details on how this field is used.
active_boolean_test_scope: Option<FileScopeId>,
/// Per-scope exception contexts for nested `try` and `with` statements.
exception_context_stack_manager: ExceptionContextStackManager,
@@ -354,6 +329,7 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> {
current_statements: Vec::new(),
current_match_case: None,
current_first_parameter_name: None,
active_boolean_test_scope: None,
exception_context_stack_manager: ExceptionContextStackManager::default(),
has_future_annotations: false,
@@ -2263,6 +2239,82 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> {
}
}
fn visit_condition(&mut self, test: &'ast ast::Expr) {
self.visit_boolean_test(test, ExpressionContext::Condition);
}
/// Visit a boolean test.
///
/// A boolean test is an expression that Python tests for truthiness, such as an `if`
/// condition or the operand of a `not` expression. ty's `redundant-condition` and
/// `redundant-condition-strict` rules can warn when such a test is always true or
/// always false. The rules try hard to avoid emitting duplicate diagnostics on the
/// same boolean test, however: in this case, a naive implementation would emit two
/// diagnostics on the `if` test, since there is both an `if` condition that is always
/// falsy and a `not` operand that is always truthy:
///
/// ```py
/// def func(): ...
///
/// if not func: # one diagnostic, or two?
/// pass
/// ```
///
/// To determine whether diagnostics on boolean-test subexpressions need to be suppressed,
/// the rules need to be able to reliably know whether a boolean test is nested within
/// another outer boolean test. The [`super::UseDefMap`] answers these queries by storing
/// [`NodeIndex`]es for each outermost boolean test in any given scope.
///
/// [`Self::visit_boolean_test`] is responsible for instructing the [`UseDefMapBuilder`]
/// to record a new outermost boolean test. The method is called whenever we visit a boolean
/// test such as an `if` condition, `while` condition, `assert` condition or `not` operand,
/// and it consults the [`Self::active_boolean_test_scope`] field to inform its decision:
///
/// - If the field is `None`, it knows that the expression currently being visited is not
/// nested inside another outermost boolean test, so the expression currently being
/// visited should be recorded
/// - If the field is `Some(scope)`, the scope must be compared against the scope currently
/// being visited. If it's a new scope, it should be recorded; if it's the same scope, it
/// should not.
///
/// As further examples, in the following snippet, `ready` is a function object, which is
/// always truthy. Each use of `not ready` should therefore produce a warning suggesting a
/// call to `ready`:
///
/// ```python
/// def ready() -> bool:
/// return True
///
/// def consume(value: object) -> bool:
/// return bool(value)
///
/// if consume(not ready):
/// print("Condition passed")
///
/// if consume(lambda: not ready):
/// print("Condition passed")
/// ```
///
/// For the first `if`, we record `consume(not ready)`. Its diagnostic check will also
/// examine the test of `ready`, so we do not record `ready` separately. While visiting the
/// call's arguments, this field keeps the call's scope ID even though the arguments are
/// evaluated with `ExpressionContext::Value`.
///
/// For the second `if`, the `lambda` body is checked independently in its own scope. Storing
/// the scope ID lets `visit_boolean_test` recognize that the test of `ready` inside the
/// `lambda` needs its own entry. This field is temporary visitor state; only the recorded
/// expression node IDs are retained in the semantic index.
fn visit_boolean_test(&mut self, test: &'ast ast::Expr, context: ExpressionContext) {
let scope = self.current_scope();
let previous = self.active_boolean_test_scope.replace(scope);
if previous != Some(scope) {
self.current_use_def_map_mut()
.record_boolean_test_root(test.node_index().load());
}
self.visit_expr_with_context(test, context);
self.active_boolean_test_scope = previous;
}
/// Adds a new predicate to the list of all predicates, but does not record it. Returns the
/// predicate ID for later recording using
/// [`SemanticIndexBuilder::record_narrowing_constraint_id_for_places`].
@@ -3077,7 +3129,7 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> {
/// print(last)
/// ```
fn visit_comprehension_filter(&mut self, if_expr: &'ast ast::Expr) -> FlowSnapshot {
self.visit_expr_with_context(if_expr, ExpressionContext::Condition);
self.visit_condition(if_expr);
let condition_flow_snapshot = self.flow_snapshot_for_condition(if_expr);
let filtered_out = if let Some(snapshots) = condition_flow_snapshot.into_branches() {
self.flow_restore(snapshots.truthy);
@@ -3545,14 +3597,11 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> {
self.record_exception_checkpoint();
}
ast::Expr::UnaryOp(unary) => {
self.visit_expr_with_context(
&unary.operand,
if unary.op == ast::UnaryOp::Not {
context
} else {
ExpressionContext::Value
},
);
if unary.op == ast::UnaryOp::Not {
self.visit_boolean_test(&unary.operand, context);
} else {
self.visit_expr_with_context(&unary.operand, ExpressionContext::Value);
}
self.record_exception_checkpoint_if(
unary.op != ast::UnaryOp::Not
|| !Self::condition_evaluation_is_known_safe(&unary.operand),
@@ -3602,7 +3651,7 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> {
let ast::ExprIf {
body, test, orelse, ..
} = node;
self.visit_expr_with_context(test, ExpressionContext::Condition);
self.visit_condition(test);
let condition_flow_snapshot = self.flow_snapshot_for_condition(test);
let falsy = if let Some(snapshots) = condition_flow_snapshot.into_branches() {
self.flow_restore(snapshots.truthy);
@@ -4193,7 +4242,7 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> {
// `msg` branch back into the following flow, since there is no way of getting out
// of that branch. Code after the assertion starts from the condition's truthy flow.
self.visit_expr_with_context(test, ExpressionContext::Condition);
self.visit_condition(test);
let condition_flow_snapshot = self.flow_snapshot_for_condition(test);
let predicate = self.build_predicate(test, ExpressionContext::Condition);
@@ -4366,7 +4415,7 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> {
}
}
ast::Stmt::If(node) => {
self.visit_expr_with_context(&node.test, ExpressionContext::Condition);
self.visit_condition(&node.test);
let condition_flow_snapshot = self.flow_snapshot_for_condition(&node.test);
let mut falsy = if let Some(snapshots) = condition_flow_snapshot.into_branches() {
self.flow_restore(snapshots.truthy);
@@ -4420,7 +4469,7 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> {
self.record_negated_reachability_constraint(last_reachability_constraint);
let next_falsy = if let Some(elif_test) = clause_test {
self.visit_expr_with_context(elif_test, ExpressionContext::Condition);
self.visit_condition(elif_test);
// A test expression is evaluated whether the branch is taken or not
let condition_flow_snapshot = self.flow_snapshot_for_condition(elif_test);
let next_falsy =
@@ -4503,7 +4552,7 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> {
// Visit the test expression after creating loop headers, so that loop-back values
// are visible.
self.visit_expr_with_context(test, ExpressionContext::Condition);
self.visit_condition(test);
let condition_flow_snapshot = self.flow_snapshot_for_condition(test);
// Take the pre_loop snapshot from the post-test fallback flow before restoring the
@@ -4881,7 +4930,7 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> {
// while the next case is reached through `!P || (P && !G)`. Save `P && !G`
// separately so it can be merged with the pattern-failure state after the body.
let match_success_guard_failure = case.guard.as_ref().map(|guard| {
self.visit_expr_with_context(guard, ExpressionContext::Condition);
self.visit_condition(guard);
let condition_flow_snapshot = self.flow_snapshot_for_condition(guard);
let falsy = if let Some(snapshots) = condition_flow_snapshot.into_branches()
{
@@ -5391,20 +5440,10 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> {
// it significantly degrades performance. We thus cut scope here and add these
// constraints only at statement-level function calls, like `sys.exit()`, and not
// within sub-expressions like `3 + sys.exit()` etc.
let call_info = match value.as_ref() {
ast::Expr::Call(ast::ExprCall { func, .. }) => {
Some((func.as_ref(), value.as_ref(), false))
}
ast::Expr::Await(ast::ExprAwait { value: inner, .. }) => match inner.as_ref() {
ast::Expr::Call(ast::ExprCall { func, .. }) => {
Some((func.as_ref(), value.as_ref(), true))
}
_ => None,
},
_ => None,
};
if let Some((func, expr, is_await)) = call_info {
if let Some(StatementCall { call, is_await }) =
StatementCall::from_expression(value)
{
let func = call.func.as_ref();
// Avoid creating reachability nodes for calls on unannotated collection
// literals. Without this short-circuit, performing reachability analysis
// can lead to quadratic blowup of cycle dependencies during full-scope
@@ -5427,7 +5466,7 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> {
{
let callable =
self.add_standalone_expression_impl(func, ExpressionKind::Callee, None);
let call_expr = self.add_standalone_expression(expr);
let call_expr = self.add_standalone_expression(value);
let predicate = Predicate {
node: PredicateNode::IsNonTerminalCall(CallableAndCallExpr {
crates/ty_python_core/src/expression.rs
+79
View File
@@ -7,6 +7,85 @@ use ruff_db::files::File;
use ruff_python_ast as ast;
use salsa;
/// Whether evaluation produces a result object or chooses a control-flow path.
///
/// The semantic-index builder uses this context when creating reachability predicates. The
/// redundant-condition checker uses it to choose between short-circuit analysis and testing
/// the resulting value type.
///
/// In `Value` context, the enclosing code receives the expression's result object. For example,
/// `result = x and y` produces `x` if `x` is falsy, or `y` otherwise. This also applies to expressions
/// that return `bool`: the comparison in `result = x > 0` has value context.
///
/// In `Condition` context, the enclosing code only needs to know which branch to take. For example,
/// CPython evaluates `if x and y` by testing `x` and, only if `x` is truthy, testing `y`. If `x` tests
/// falsy, that one truthiness check is enough to skip the body: `x` is not tested again as the
/// result of `x and y`.
///
/// This distinction matters when an operand's `__bool__` can change between calls:
///
/// ```python
/// if x and False: # A falsy x skips the body; a truthy x reaches False.
/// ... # Unreachable in either case.
/// saved = x and False # Can produce x after checking that it is falsy.
/// if saved: # Can call x.__bool__ again, which may now return True.
/// ... # Reachable.
/// ```
///
/// Chained comparisons have the same distinction. Consider a class whose comparison method has
/// an `object` return type:
///
/// ```py
/// from typing_extensions import reveal_type
///
///
/// class Comparable:
/// def __lt__(self, other: int) -> object: ...
///
///
/// def check(value: Comparable):
/// reveal_type(value < 1 < 0) # revealed: ~AlwaysTruthy
///
/// if value < 1 < 0: # error: [redundant-condition-strict] "always false"
/// pass
/// ```
///
/// Outside the context of an `if` test, the revealed type of the condition here is `~AlwaysTruthy`:
/// in other words, ty knows that this expression is not *always true*, but cannot guarantee that it is
/// definitely *always false*. It could be an object that is sometimes true and sometimes false -- for
/// example, a `list` (which is falsy when it is empty, and truthy otherwise).
///
/// Nonetheless, when `value < 1 < 0` is used directly as a condition, ty knows that the condition will
/// always be falsy and the `if` branch will never be taken. Python tests the truthiness of the object
/// returned by `Comparable.__lt__` once: if it is falsy, the condition fails immediately. If it is
/// truthy, Python evaluates `1 < 0`, which is false. There is no second truthiness test of the object
/// returned by `__lt__`.
///
/// If the chained comparison is saved as a variable first, its value can be the object returned by
/// `__lt__`, if that object was falsy when first tested. The `if result` statement then tests that
/// object's truthiness again. A user-defined `__bool__` method can return a different result on that
/// second call, so ty cannot guarantee that the saved value is still falsy, and no diagnostic is
/// emitted:
///
/// ```py
/// def check_saved(value: Comparable):
/// result = value < 1 < 0
/// if result: # no diagnostic
/// pass
/// ```
///
/// The context propagates through `and`, `or`, `not`, and the branches of conditional expressions.
/// Condition context does not propagate through calls or assignment expressions: in
/// `if f(x and False)`, the call's result controls the branch, but its argument is evaluated in
/// value context.
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
pub enum ExpressionContext {
/// Produce the expression's result object for the enclosing code to use.
Value,
/// Choose the truthy or falsy control-flow path without preserving the result object.
Condition,
}
/// The context used to infer an independently tracked expression.
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, get_size2::GetSize)]
pub enum ExpressionKind {
crates/ty_python_core/src/lib.rs
+35 -1
View File
@@ -9,7 +9,7 @@ use std::sync::Arc;
use ruff_db::parsed::parsed_module;
use ruff_index::{FrozenIndexVec, IndexSlice};
use ruff_python_ast::NodeIndex;
use ruff_python_ast::{HasNodeIndex, NodeIndex};
use ruff_python_parser::semantic_errors::SemanticSyntaxError;
use ruff_text_size::TextRange;
use rustc_hash::{FxHashMap, FxHashSet};
@@ -553,6 +553,36 @@ impl<'db> SemanticIndex<'db> {
})
}
/// Return `true` if `expression` is an outermost "boolean test".
///
/// A boolean test is an expression that Python tests for truthiness, such as an `if`
/// condition or the operand of a `not` expression. ty's `redundant-condition` and
/// `redundant-condition-strict` rules can warn when such a test is always true or
/// always false. The rules try hard to avoid emitting duplicate diagnostics on the
/// same boolean test, however: in this case, a naive implementation would emit two
/// diagnostics on the `if` test, since there is both an `if` condition that is always
/// falsy and a `not` operand that is always truthy:
///
/// ```py
/// def func(): ...
///
/// if not func: # one diagnostic, or two?
/// pass
/// ```
///
/// This method returns `true` when passed the expression `not func` in the above
/// example, but `false` for `func`, which is part of a nested boolean test.
///
/// See `SemanticIndexBuilder::visit_boolean_test` for details on how we compute
/// and store the required data for answering this query during semantic indexing.
pub fn is_boolean_test_root(&self, expression: &ast::Expr) -> bool {
self.try_expression_scope_id(expression)
.is_some_and(|scope| {
self.use_def_map(scope)
.is_boolean_test_root(expression.node_index().load())
})
}
/// Returns an iterator over the descendent scopes of `scope`.
fn descendent_scopes(&self, scope: FileScopeId) -> DescendantsIter<'_> {
DescendantsIter::new(&self.scopes, scope)
@@ -954,6 +984,10 @@ impl Truthiness {
!self.is_always_false()
}
pub const fn may_be_false(self) -> bool {
!self.is_always_true()
}
pub const fn is_always_true(self) -> bool {
matches!(self, Truthiness::AlwaysTrue)
}
crates/ty_python_core/src/predicate.rs
+39 -1
View File
@@ -11,7 +11,7 @@ use crate::Program;
use ruff_db::PythonFile;
use ruff_db::files::File;
use ruff_index::{FrozenIndexVec, Idx, IndexVec};
use ruff_python_ast::{Singleton, name::Name};
use ruff_python_ast::{self as ast, Singleton, name::Name};
use crate::ProgramFile;
use crate::ast_ids::ExpressionNodeKey;
@@ -112,6 +112,44 @@ pub struct CallableAndCallExpr<'db> {
pub is_await: bool,
}
/// A call whose completion determines whether an expression statement can continue.
#[derive(Debug)]
pub struct StatementCall<'ast> {
pub call: &'ast ast::ExprCall,
pub is_await: bool,
}
impl<'ast> StatementCall<'ast> {
/// Extracts a direct or awaited call for checking whether an expression statement can return.
///
/// Callers pass the value of an expression statement, such as `sys.exit()` or `await stop()`.
/// The semantic-index builder uses the returned call to record a reachability constraint:
/// if the call returns `Never`, subsequent statements are unreachable. The
/// `redundant-condition-strict` rule uses the same extraction when recognizing defensive exits.
///
/// The `is_await` flag matters for async functions returning `Never`: calling one creates a
/// coroutine, while awaiting it prevents execution from continuing.
///
/// Calls inside larger expressions, such as `1 + sys.exit()`, are deliberately excluded.
/// Tracking their termination would require many more reachability constraints and degrade
/// analysis performance.
pub fn from_expression(expression: &'ast ast::Expr) -> Option<Self> {
match expression {
ast::Expr::Call(call) => Some(Self {
call,
is_await: false,
}),
ast::Expr::Await(ast::ExprAwait { value, .. }) => {
value.as_call_expr().map(|call| Self {
call,
is_await: true,
})
}
_ => None,
}
}
}
#[derive(Clone, Copy, Debug, Hash, PartialEq, Eq, get_size2::GetSize, salsa::SalsaValue)]
pub enum PredicateNode<'db> {
/// The truthiness of an expression's resulting value.
crates/ty_python_core/src/use_def.rs
+46 -1
View File
@@ -245,6 +245,7 @@ use std::rc::Rc;
use std::sync::{Arc, LazyLock};
use ruff_index::{FrozenIndexVec, Idx, IndexVec, newtype_index};
use ruff_python_ast::NodeIndex;
use ruff_text_size::TextRange;
use rustc_hash::{FxBuildHasher, FxHashMap, FxHashSet, FxHasher};
use smallvec::SmallVec;
@@ -664,6 +665,16 @@ struct UseDefMapExtra {
/// Completed loop headers in this scope.
loop_headers: FrozenIndexVec<LoopHeaderId, LoopHeader>,
/// Node IDs of "boolean tests".
///
/// See [`super::SemanticIndexBuilder::visit_boolean_test`] for details on what a boolean
/// test is, and how this field is used during type inference.
///
/// The stored IDs exclude any entries that are nested inside another such test in this
/// scope. They are guaranteed to be in sorted order, so that they can be queried using a
/// binary search.
boolean_test_roots: Box<[NodeIndex]>,
}
static EMPTY_CONSTRAINT_TABLES: LazyLock<ConstraintTables<'static>> =
@@ -1022,6 +1033,22 @@ impl<'db> UseDefMap<'db> {
block.in_type_checking_block && entry_range.contains_range(range)
})
}
/// Return `true` if `node` is one of the tests recorded in
/// [`UseDefMapExtra::boolean_test_roots`].
///
/// See [`super::SemanticIndexBuilder::visit_boolean_test`] for details on what this is used for.
pub(crate) fn is_boolean_test_root(&self, node: NodeIndex) -> bool {
self.extra.as_ref().is_some_and(|extra| {
debug_assert!(
extra.boolean_test_roots.is_sorted(),
"`boolean_test_roots` must be in sorted order \
to use a binary search in `is_boolean_test_root`"
);
extra.boolean_test_roots.binary_search(&node).is_ok()
})
}
pub fn end_of_scope_bindings(
&self,
place: ScopedPlaceId,
@@ -1848,6 +1875,9 @@ pub(super) struct UseDefMapBuilder<'db> {
/// keyed by their text range.
range_reachability: Vec<(TextRange, RangeInfo)>,
/// Node IDs collected for [`UseDefMapExtra::boolean_test_roots`], before sorting.
boolean_test_roots: Vec<NodeIndex>,
/// Identifies the current control-flow path for exception checkpoints.
///
/// Unlike `reachability`, this excludes per-call gates so repeated calls with unchanged
@@ -1902,6 +1932,7 @@ impl<'db> UseDefMapBuilder<'db> {
multi_bindings_by_use: FxHashMap::default(),
reachability: ScopedReachabilityConstraintId::ALWAYS_TRUE,
range_reachability: Vec::new(),
boolean_test_roots: Vec::new(),
checkpoint_flow: ScopedReachabilityConstraintId::ALWAYS_TRUE,
checkpoint_state: ExceptionCheckpointState::default(),
definitions_by_definition: FxHashMap::default(),
@@ -2625,6 +2656,14 @@ impl<'db> UseDefMapBuilder<'db> {
self.range_reachability.push((range, this_range_info));
}
/// Record a "boolean test" expression that Python tests for truthiness.
///
/// See [`super::SemanticIndexBuilder::visit_boolean_test`] for details on what a boolean
/// test is, and how the recorded "boolean test roots" are used during type inference.
pub(super) fn record_boolean_test_root(&mut self, node: NodeIndex) {
self.boolean_test_roots.push(node);
}
pub(super) fn snapshot_enclosing_state(
&mut self,
enclosing_place: ScopedPlaceId,
@@ -2953,10 +2992,15 @@ impl<'db> UseDefMapBuilder<'db> {
Self::zip_place_states(end_of_scope_members, reachable_definitions_by_member);
let multi_bindings_by_use = MultiBindingsByUse::from_map(self.multi_bindings_by_use);
let loop_headers = self.loop_headers;
let mut boolean_test_roots = self.boolean_test_roots;
// In `body if test else other`, we visit `test` before `body`, but node indices follow
// source order. Sort the recorded IDs so root membership can use binary search.
boolean_test_roots.sort_unstable();
let extra = (!bindings_by_use.is_empty()
|| !member_states.is_empty()
|| !enclosing_snapshots.is_empty()
|| !loop_headers.is_empty())
|| !loop_headers.is_empty()
|| !boolean_test_roots.is_empty())
.then(|| {
Box::new(UseDefMapExtra {
bindings_by_use: bindings_by_use.into(),
@@ -2964,6 +3008,7 @@ impl<'db> UseDefMapBuilder<'db> {
member_states,
enclosing_snapshots: enclosing_snapshots.into(),
loop_headers: loop_headers.into(),
boolean_test_roots: boolean_test_roots.into_boxed_slice(),
})
});
let predicates = self.predicates.build();
crates/ty_python_semantic/resources/lint_docs/redundant-condition-strict.md
+380
View File
@@ -0,0 +1,380 @@
## What it does
Detects boolean conditions where the condition can be statically inferred to be always true or
always false.
This rule is disabled by default. It exclusively covers cases that its sibling (enabled-by-default)
rule `redundant-condition` does not cover. These cases often flag real bugs in user code, but also
have a significantly higher rate of unavoidable false positives than other cases.
This rule reports redundant conditions that meet any of these criteria:
- The boolean test is inferred as evaluating to `True` itself, `False` itself, or an exact integer
such as `1` or `0`.
- Short-circuit evaluation means the condition can be guaranteed to be always truthy or always falsy
despite fixed truthiness not being guaranteed by the inferred type of the expression's value
(see "Short-circuiting boolean conditions" below for an example).
- The condition uses a walrus operator (`:=`). The assignment's side effect may be intentional, even
when its result has fixed truthiness.
## Why is this bad?
A boolean condition that is always true or always false usually indicates a mistake in your code,
and can often lead to incorrect behavior. If an `if` condition is inferred as always false,
moreover, ty will infer all code within that `if` branch as being unreachable, and will not report
any diagnostics on code in that region.
## Examples
A common error in Python code is to make the mistake of thinking that indexing into a `bytes` object
will get you an object of type `bytes`. But `bytes` work differently to `str`s in Python -- although
a string is a sequence of strings, a bytestring is a sequence of `int`s, so indexing into a `bytes`
object gives you an `int`. This rule can catch that error by alerting you to the fact that checking
whether a `bytes` object is unequal to an `int` will always evaluate to `True`:
```py
def validate_record(data: bytes) -> None:
if data[0] != b"\x1e": # error: [redundant-condition-strict]
raise ValueError("Invalid record separator")
```
Another common mistake is to assume that annotating `**kwargs` with `dict[str, str]` describes the
dictionary containing the keyword arguments. In fact, a `**kwargs` annotation describes each
individual keyword argument, so this annotation says that every value is itself a dictionary.
Comparing one of those values with a string will therefore always evaluate to `False`:
```py
def trace(**kwargs: dict[str, str]) -> None:
if kwargs.get("operation") == "task": # error: [redundant-condition-strict]
print("Tracing task")
```
## Short-circuiting boolean conditions
In some situations, ty can know that a condition will always be true, or it can know that a
condition will always be false, even when this is not guaranteed by the inferred type of that
condition. This is because of the way that Python short-circuits evaluation of conditions in the
context of `if` tests, `while` tests and `assert` statements.
Consider a class whose comparison method has an `object` return type:
```py
from typing_extensions import reveal_type
class Comparable:
def __lt__(self, other: int) -> object: ...
def check(value: Comparable):
reveal_type(value < 1 < 0) # revealed: ~AlwaysTruthy
if value < 1 < 0: # error: [redundant-condition-strict] "always false"
pass
```
Outside the context of an `if` test, the revealed type of the condition here is `~AlwaysTruthy`: in
other words, ty knows that this expression is not *always true*, but cannot guarantee that it is
definitely *always false*. It could be an object that is sometimes true and sometimes false -- for
example, a `list` (which is falsy when it is empty, and truthy otherwise).
Nonetheless, when `value < 1 < 0` is used directly as a condition, ty knows that the condition will
always be falsy and the `if` branch will never be taken. Python tests the truthiness of the object
returned by `Comparable.__lt__` once: if it is falsy, the condition fails immediately. If it is
truthy, Python evaluates `1 < 0`, which is false. There is no second truthiness test of the object
returned by `__lt__`.
If the chained comparison is saved as a variable first, its value can be the object returned by
`__lt__`, if that object was falsy when first tested. The `if result` statement then tests that
object's truthiness again. A user-defined `__bool__` method can return a different result on that
second call, so ty cannot guarantee that the saved value is still falsy, and no diagnostic is
emitted:
```py
def check_saved(value: Comparable):
result = value < 1 < 0
if result: # no diagnostic
pass
```
## Exemptions
Like `redundant-condition`, this rule checks subexpressions of an `and` or `or` expression only when
the outer expression is used as a condition. This is to avoid emitting false-positive diagnostics on
code like the following, where the `and` expression is clearly not redundant despite the fact that
both `CONSTANT_1` and `CONSTANT_2` are always truthy:
```py
from typing import Final
CONSTANT_1: Final = 1
CONSTANT_2: Final = 2
def do_something(coinflip: bool):
# could also be written as `constant_to_use = CONSTANT_1 if coinflip else CONSTANT_2`,
# but use of an `and` expression for this is common in older codebases.
constant_to_use = coinflip and CONSTANT_1 or CONSTANT_2
# do something with `constant_to_use` now...
...
```
Unlike `and` and `or`, however, `not` explicitly converts its operand to a boolean, so the rule
checks `not` expressions in every context.
Another exemption applied by this rule concerns `assert`-statement tests. A common pattern in Python
code is to use defensive `assert`s to enforce behaviour at runtime, even when the asserted condition
can be inferred statically to be always true. For example:
```py
def add_one(x: int) -> int:
assert isinstance(x, int) # no diagnostic
return x + 1
```
This kind of defensive behaviour is often reasonable, since the author of a library cannot guarantee
that end users of the library will run a type checker on code calling into the library, meaning that
it's entirely possible at runtime for an object passed into the `x`a parameter above to be a `str`
(for example) even though the parameter annotation states that only `int`s can ever be passed in.
This rule therefore also exempts all assertion tests or subexpressions that evaluate to a subtype of
`int` or `bool`:
`redundant-condition-strict` can still trigger on `assert` statements in some contexts, however. For
example, `redundant-condition-strict` will be emitted on the below example, where the left-hand side
of the `and` expression is always true and not a subtype of `bool` or `int`, but where the condition
is nonetheless excluded from the enabled-by-default `redundant-condition` rule due to the use of the
walrus operator:
```py
def func() -> bool:
return True
def test_func():
assert (result := func) and result != func() # error: [redundant-condition-strict]
```
For similar reasons to the `assert` exemptions, this rule also exempts always-false `if` or `elif`
conditions when their bodies end in a defensive check: a `raise`, an assertion that could fail, a
call returning `Never`, an `await` to a call returning `Never`, or `return NotImplemented`:
```py
import sys
def add_two(x: int) -> int:
if not isinstance(x, int): # no diagnostic
raise TypeError("need an int!!")
return x + 2
def add_three(x: int) -> int:
if not isinstance(x, int): # no diagnostic
assert False, "unreachable"
return x + 3
def add_four(x: int) -> int:
if not isinstance(x, int): # no diagnostic
sys.exit(1)
return x + 4
class Foo:
def __init__(self, data: int):
self.data = data
def __add__(self, other: "Foo"):
if not isinstance(other, Foo): # no diagnostic
return NotImplemented
return Foo(self.data + other.data)
```
And an exemption is applied for always-true `if` or `elif` statements that are followed by branches
which contain defensive checks:
```py
from typing_extensions import assert_never
def parse_data(data: int | str):
if isinstance(data, int):
print("got an int")
elif isinstance(data, str): # Always true, but no diagnostic, since
# the `else` branch following this branch is always terminal.
# (`assert_never` returns `Never`, indicating that it always raises an exception)
print("got a str")
else:
assert_never(data)
def parse_data_early_return(data: int | str):
if isinstance(data, int):
print("got an int")
return
# Always true, but no diagnostic, since
# the suite following this branch is always terminal
# (every control-flow path following this `if` statement ends in a `raise` statement)
if isinstance(data, str):
print("got a str")
return
raise AssertionError("unexpected data")
```
Any conditions defined in relation to `sys.version_info`, `sys.platform`, `os.name` or
`typing.TYPE_CHECKING` are also exempted. The rule recursively follows the definitions of names and
attributes across module boundaries to determine if a name or attribute was indirectly defined in
relation to one of these highly special-cased symbols:
```toml
[environment]
python-version = "3.14"
python-platform = "linux"
```
```py
import os
import sys
from typing import TYPE_CHECKING
if sys.version_info >= (3, 14): # inferred as always true here, but no diagnostic
pass
if sys.platform == "win32": # inferred as always false here, but no diagnostic
pass
LINE_ENDING = "\n" if os.name == "posix" else "\r\n"
if LINE_ENDING == "\n": # inferred as always true here, but no diagnostic
pass
if TYPE_CHECKING: # inferred as always true, but no diagnostic
pass
```
Conditions involving these constants, or conditions involving values defined in relation to these
constants, can often be inferred as always-true or always-false by ty. Indeed, these conditions
usually *will* be always true or always false across a single invocation run of a Python programme.
Nonetheless, Python code is often written so that it can work on multiple different Python versions
and/or multiple different operating systems, and a condition that is always true on one operating
system might very well be always false on another operating system (for example). Flagging these
conditions as being always true or always false would only add noise: the aim of the rule is to flag
conditions that are *unintentionally* always true or always false.
Lastly, some conditions involving literal integers and booleans in the AST are also exempted:
there's no reason why you'd use a condition like this unless it was intentional.
```py
if True: # inferred as always true (obviously), but no diagnostic
pass
if 0:
pass # inferred as always false, but no diagnostic
```
## Known issues and workarounds
This rule can often trigger on code that is not incorrect, but could be written in a clearer way.
For example, the rule will flag this code:
```py
from enum import Enum
class YesOrNo(Enum):
YES = 1
NO = 0
def say_yes_or_no(what_to_say: YesOrNo):
if what_to_say == YesOrNo.YES:
print("yes")
elif what_to_say == YesOrNo.NO: # error: [redundant-condition-strict]
print("no")
```
This snippet could be written more clearly as this, which would not trigger the rule owing to the
exemptions described in the section above:
```py
def say_yes_or_no(what_to_say: YesOrNo):
if what_to_say == YesOrNo.YES:
print("yes")
else:
assert what_to_say == YesOrNo.NO
print("no")
```
or the snippet could also be rewritten as this, which would also be fine according to the rule's
heuristics:
```py
from typing_extensions import assert_never
def say_yes_or_no(what_to_say: YesOrNo):
if what_to_say == YesOrNo.YES:
print("yes")
elif what_to_say == YesOrNo.NO:
print("no")
else:
assert_never(what_to_say)
```
In a similar vein, this rule can often flag `and` or `or` expressions that have operands which are
deliberately always truthy or deliberately always falsy, because the purpose of the operand is to
have some side effect occur. For example:
```py
import random
from typing import Literal
def want_to_go_fishing() -> bool:
return random.choice([True, False])
def weather_report() -> Literal["rainy", "sunny", "cloudy"]:
return random.choice(["rainy", "sunny", "cloudy"])
def have_fishing_supplies() -> bool:
return random.choice([True, False])
def main():
if (
want_to_go_fishing()
and (weather := weather_report()) # error: [redundant-condition-strict]
and have_fishing_supplies()
):
print(f"The weather is {weather}, let's go fishing")
```
The middle operand in the above `and` expression is always truthy. This might be deliberate, but
even if it is, the function would arguably be clearer if it were written like this instead:
```py
def main():
if want_to_go_fishing():
weather = weather_report()
if have_fishing_supplies():
print(f"The weather is {weather}, let's go fishing")
```
Lastly, the rule cannot reliably distinguish in all cases comparisons that are intentionally always
true/false from those that are unintentionally always true/false. The rule takes care to avoid
flagging code that uses `if TYPE_CHECKING`, `if sys.version_info < (X, Y)`, `if sys.platform == ...`
and `if os.name == ...`. But it cannot reliably determine that code like this was written the way it
was meant to be:
```py
DEBUGGING = 0
if DEBUGGING: # error: [redundant-condition-strict]
print("Doing debugging stuff...")
```
crates/ty_python_semantic/resources/lint_docs/redundant-condition.md
+169
View File
@@ -0,0 +1,169 @@
## What it does
Detects boolean conditions where the condition can be statically inferred to be always true or
always false due to the inferred type of the condition.
This rule is enabled by default, and is deliberately not comprehensive. In order to avoid false
positives, it excludes conditions that meet any of these criteria:
- The boolean test is inferred as evaluating to `True` itself, `False` itself, or an exact integer
such as `1` or `0`.
- The boolean test can be inferred as always evaluating to `True` and `False`, but this inference is
due to boolean-test short-circuiting in `if` conditions, `while` conditions or `assert` tests
rather than the inferred type of the boolean test.
- The condition uses a walrus operator (`:=`). The assignment's side effect may be intentional, even
when its result has fixed truthiness.
## Why is this bad?
A boolean condition that is always true or always false usually indicates a mistake in your code,
and can often lead to incorrect behavior. If an `if` condition is inferred as always false,
moreover, ty will infer all code within that `if` branch as being unreachable, and will not report
any diagnostics on code in that region.
## Examples
A common error that triggers this rule is to forget to call a function, for example:
```py
import random
def should_do_action() -> bool:
return random.choice([True, False])
# oops! You forgot the parentheses here... this should have been `if should_do_action()`.
# Because it's not, this will always be `True`:
if should_do_action: # error: [redundant-condition]
print("Doing stuff...")
```
Another common mistake is to forget to `await` a coroutine:
```py
import random
async def should_do_async_action():
return random.choice([True, False])
async def main():
# oops! Forgot the await here... this should have been `if await should_do_async_action()`.
# Because it's not, this will always be `True`:
if should_do_async_action(): # error: [redundant-condition]
print("Doing stuff async...")
```
Or to forget that `tuple[X]` means "A tuple with exactly one element" rather than "a tuple with an
arbitrary number of elements" (for which you'd use `tuple[X, ...]`):
```py
# you almost certainly meant to write `tuple[str, ...]` here rather than `tuple[str]`...
def consume_tuples(x: tuple[str]):
# ...and that means that this later condition is inferred as always being True by ty:
if x: # error: [redundant-condition]
print("Got a non-empty tuple")
```
Some Pythonistas fall into the trap of thinking that a generator expression will be falsy if it has
zero elements inside it -- but generator expressions are lazy, and so they're always truthy unless
you collect them into a tuple:
```py
def test_my_data(data: list[int]):
# this will always be `True`, because the asserted object is a `types.GeneratorType` instance,
# not a `tuple`! `assert any(item for item in data if item > 42)`
# is probably what you meant instead.
assert (item for item in data if item > 42) # error: [redundant-condition]
```
## Boolean operators used to compute values
The rule checks `and` and `or` operands when the expression is used as a condition: in an `if`,
`elif`, `while`, or `assert` test, a conditional expression, a comprehension filter, a match guard,
or as the operand of `not`. It does not flag `and` or `or` expressions used to compute values --
even if an operand in an `and` or `or` expression is always truthy, it doesn't necessarily make the
expression redundant:
```py
def f(): ...
def g(): ...
def test(coinflip: bool):
# could also be written as `func = f if coinflip else g`,
# but use of an `and` expression for this is common in older codebases.
func = coinflip and f or g
# `func` will be the `f` function if `coinflip` is `True`,
# and the `g` function otherwise
func()
```
This also allows calls that are deliberately always falsy but are used for their side effects:
```py
from unittest.mock import patch
def ask_to_continue() -> bool:
return input("Continue? ") == "yes"
def test_ask_to_continue():
prompts = []
with patch(
"builtins.input",
side_effect=lambda prompt: prompts.append(prompt) or "yes",
):
assert ask_to_continue()
assert prompts == ["Continue? "]
```
By contrast, `not` always produces a boolean, so we will still emit a diagnostic on the following
example -- negating the truthiness of a function object is pointless, since a function object is
always truthy:
```py
def f(): ...
value = not f # error: [redundant-condition]
```
## Known issues and workarounds
This rule can sometimes trigger on code that is not incorrect, but could be written in a clearer
way. For example, the rule will flag this code:
```py
def find_duplicate_coordinates(coordinates: list[tuple[int, int]]):
seen: set[tuple[int, int]] = set()
# error: [redundant-condition] "Expression `seen.add(coord)` is always falsy (has type `None`)"
duplicates = {coord for coord in coordinates if coord in seen or seen.add(coord)}
print(f"Duplicates are {duplicates}")
```
The error here is triggered due to `seen.add(coord)` being used in a boolean expression, despite the
fact that `set.add()` always returns `None`. Here this is deliberate: `set.add()` is being used for
its side effect.
To workaround this issue, the above code could be rewritten like this, which may also be easier for
some readers to understand:
```py
def find_duplicate_coordinates(coordinates: list[tuple[int, int]]):
seen: set[tuple[int, int]] = set()
duplicates: set[tuple[int, int]] = set()
for coord in coordinates:
if coord in seen:
duplicates.add(coord)
else:
seen.add(coord)
print(f"Duplicates are {duplicates}")
```
crates/ty_python_semantic/resources/mdtest/cycle/basic.md
+9 -9
View File
@@ -7,7 +7,7 @@ inferring the lambda's return type, which depends on that same assignment.
```py
(f := lambda: f)
while lambda: f:
while lambda: f: # error: [redundant-condition] "always truthy"
pass
f = 0
```
@@ -18,7 +18,7 @@ The same cycle can arise when a conditional filters the bindings visible to a re
```py
f = lambda: f
if not (lambda: f):
if not (lambda: f): # error: [redundant-condition] "always truthy"
f = 0
```
@@ -264,7 +264,7 @@ an earlier assertion and prevent inference from converging. This is a regression
```py
f = lambda: f
assert f
assert f # error: [redundant-condition] "always truthy"
@property
def f(x=lambda: f): ...
@@ -274,7 +274,7 @@ The same cycle must converge when the parameter and return type are annotated:
```py
g = lambda: g
assert g
assert g # error: [redundant-condition] "always truthy"
@property
def g(x: object = lambda: g) -> None: ...
@@ -293,7 +293,7 @@ def decorator(value: int) -> int:
return value
f = lambda: f
assert f
assert f # error: [redundant-condition] "always truthy"
# error: [invalid-argument-type] "Expected `int`, found `def f(x=...) -> Unknown`"
@decorator
@@ -306,7 +306,7 @@ Constructing a property explicitly has the same behavior as decorator syntax:
```py
f = lambda: f
assert f
assert f # error: [redundant-condition] "always truthy"
def getter(x=lambda: f): ...
@@ -326,7 +326,7 @@ def decorator(fn: Callable[[Any], Any]) -> Callable[[Any], Any]:
return fn
f = lambda: f
assert f
assert f # error: [redundant-condition] "always truthy"
@decorator
def f(x=lambda: f): ...
@@ -349,7 +349,7 @@ def decorator[**P](fn: Callable[P, None]) -> Callable[[], None]:
return lambda: None
f = lambda: f
assert f
assert f # error: [redundant-condition] "always truthy"
@decorator
def f(x=lambda: f) -> None: ...
@@ -369,7 +369,7 @@ python-version = "3.12"
```py
f = lambda: f
assert f
assert f # error: [redundant-condition] "always truthy"
@property
def f[T](value: T, callback=lambda: f) -> T:
crates/ty_python_semantic/resources/mdtest/decorators.md
+1 -1
View File
@@ -937,7 +937,7 @@ def dynamic(function: object):
return function
f = lambda: f
assert f
assert f # error: [redundant-condition] "always truthy"
# snapshot: dynamic-function-decorator-return
@dynamic
crates/ty_python_semantic/resources/mdtest/enums.md
+1
View File
@@ -3485,6 +3485,7 @@ def color_value_without_red_and_with_restricted_typevar(
def color_truthy_without_red(color: Color) -> int:
if color is Color.RED:
raise ValueError()
# error: [redundant-condition] "always truthy"
if color:
return 1
crates/ty_python_semantic/resources/mdtest/expression/if.md
+3
View File
@@ -11,12 +11,15 @@ def _(flag: bool):
```py
reveal_type(1 if True else 2) # revealed: Literal[1]
# error: [redundant-condition] "always truthy"
reveal_type(1 if "not empty" else 2) # revealed: Literal[1]
# error: [redundant-condition] "always truthy"
reveal_type(1 if (1,) else 2) # revealed: Literal[1]
reveal_type(1 if 1 else 2) # revealed: Literal[1]
reveal_type(1 if False else 2) # revealed: Literal[2]
reveal_type(1 if None else 2) # revealed: Literal[2]
# error: [redundant-condition] "always falsy"
reveal_type(1 if "" else 2) # revealed: Literal[2]
reveal_type(1 if 0 else 2) # revealed: Literal[2]
```
crates/ty_python_semantic/resources/mdtest/liskov.md
+1
View File
@@ -2547,6 +2547,7 @@ have bigger problems:
from __future__ import annotations
class MaybeEqWhile:
# error: [redundant-condition] "always truthy"
while ...:
def __eq__(self, other: MaybeEqWhile) -> bool:
return True
crates/ty_python_semantic/resources/mdtest/named_tuple.md
+3 -3
View File
@@ -209,9 +209,9 @@ a = A(x=B(x=C(x=A(x=None))))
reveal_type(a.x) # revealed: B | None
if a.x:
reveal_type(a.x and a.x.x) # revealed: C
reveal_type(a.x and a.x.x.x) # revealed: A
reveal_type(a.x and a.x.x.x.x) # revealed: B | None
reveal_type(a.x.x) # revealed: C
reveal_type(a.x.x.x) # revealed: A
reveal_type(a.x.x.x.x) # revealed: B | None
A(x=42) # error: [invalid-argument-type]
crates/ty_python_semantic/resources/mdtest/narrow/match.md
+5
View File
@@ -3962,8 +3962,10 @@ class C:
def _(x: Literal["foo", b"bar"] | int):
match x:
# error: [redundant-condition] "always truthy"
case "foo" if reveal_type(x): # revealed: Literal["foo"]
pass
# error: [redundant-condition] "always truthy"
case b"bar" if reveal_type(x): # revealed: Literal[b"bar"]
pass
case 42 if reveal_type(x): # revealed: Literal[42]
@@ -4062,8 +4064,10 @@ from typing import Literal
def _(x: Literal["foo", b"bar"] | int):
match x:
# error: [redundant-condition] "always truthy"
case "foo" | 42 if reveal_type(x): # revealed: Literal["foo", 42]
pass
# error: [redundant-condition] "always truthy"
case b"bar" if reveal_type(x): # revealed: Literal[b"bar"]
pass
case _ if reveal_type(x): # revealed: int & ~Literal[42]
@@ -4102,6 +4106,7 @@ match x:
pass
case False if x and reveal_type(x): # revealed: Never
pass
# error: [redundant-condition] "always truthy"
case "foo" if (x := "bar") and reveal_type(x): # revealed: Literal["bar"]
pass
crates/ty_python_semantic/resources/mdtest/narrow/truthiness.md
+14 -4
View File
@@ -6,6 +6,7 @@ A generator object is truthy even when it yields no values.
```py
def narrow(value: int | None) -> None:
# error: [redundant-condition] "always truthy"
if (value for _ in ()):
if value is None:
return
@@ -32,24 +33,28 @@ def _(x: X):
reveal_type(x) # revealed: Literal[-1, True, "foo", b"bar"]
def _(x: X):
# error: [redundant-condition] "always truthy"
if x and not x:
reveal_type(x) # revealed: Never
else:
reveal_type(x) # revealed: Literal[0, -1, "", "foo", b"", b"bar"] | bool | None | tuple[()]
def _(x: X):
# error: [redundant-condition] "Variable `x` is always truthy (has type `Literal[-1, True, "foo", b"bar"]`)"
if not (x and not x):
reveal_type(x) # revealed: Literal[0, -1, "", "foo", b"", b"bar"] | bool | None | tuple[()]
else:
reveal_type(x) # revealed: Never
def _(x: X):
# error: [redundant-condition] "always falsy"
if x or not x:
reveal_type(x) # revealed: Literal[-1, 0, "foo", "", b"bar", b""] | bool | None | tuple[()]
else:
reveal_type(x) # revealed: Never
def _(x: X):
# error: [redundant-condition] "Variable `x` is always falsy (has type `Literal[0, False, "", b""] | None | tuple[()]`)"
if not (x or not x):
reveal_type(x) # revealed: Never
else:
@@ -152,7 +157,7 @@ def bar(world: str, *args, **kwargs) -> float:
x = foo if flag() else bar
if x:
if x: # error: [redundant-condition] "always truthy"
reveal_type(x) # revealed: (def foo(hello: int) -> bytes) | (def bar(world: str, *args, **kwargs) -> float)
else:
reveal_type(x) # revealed: Never
@@ -227,14 +232,14 @@ class F:
t = T()
if t:
if t: # error: [redundant-condition] "always truthy"
reveal_type(t) # revealed: T
else:
reveal_type(t) # revealed: Never
f = F()
if f:
if f: # error: [redundant-condition] "always falsy"
reveal_type(f) # revealed: Never
else:
reveal_type(f) # revealed: F
@@ -276,11 +281,14 @@ if isinstance(x, str) and not isinstance(x, B):
## Narrowing Multiple Variables
A contradictory condition makes its body unreachable, narrowing every variable to `Never` inside
that body. We report the complete condition when no individual operand is reported.
```py
from typing import Literal
def f(x: Literal[0, 1], y: Literal["", "hello"]):
if x and y and not x and not y:
if x and y and not x and not y: # error: [redundant-condition] "always falsy"
reveal_type(x) # revealed: Never
reveal_type(y) # revealed: Never
else:
@@ -288,6 +296,7 @@ def f(x: Literal[0, 1], y: Literal["", "hello"]):
reveal_type(x) # revealed: Literal[0, 1]
reveal_type(y) # revealed: Literal["", "hello"]
# error: [redundant-condition] "Nonempty string `y` is always truthy (has type `Literal["hello"]`)"
if (x or not x) and (y and not y):
reveal_type(x) # revealed: Never
reveal_type(y) # revealed: Never
@@ -554,6 +563,7 @@ def f(arg1: Empty | None, arg2: NonEmpty | None, arg3: HasNotRequired1 | None, a
if arg4:
reveal_type(arg4) # revealed: HasNotRequired2 & ~AlwaysFalsy
# error: [redundant-condition] "always truthy"
if arg5:
reveal_type(arg5) # revealed: AlsoNonEmpty
```
crates/ty_python_semantic/resources/mdtest/protocols.md
+1 -1
View File
@@ -767,7 +767,7 @@ static_assert(not is_assignable_to(FooSubclassOfAny, HasX))
class FooWithY(Foo):
y: int
assert is_subtype_of(FooWithY, HasXY)
static_assert(is_subtype_of(FooWithY, HasXY))
static_assert(is_assignable_to(FooWithY, HasXY))
class Bar:
crates/ty_python_semantic/resources/mdtest/redundant_condition.md
+945 -175
View File
File diff suppressed because it is too large. Load diff
crates/ty_python_semantic/resources/mdtest/statically_known_branches.md
+1
View File
@@ -181,6 +181,7 @@ class AlwaysTrue:
```py
from module import AlwaysTrue
# error: [redundant-condition] "always truthy"
if AlwaysTrue():
yes = True
else:
crates/ty_python_semantic/resources/mdtest/unary/not.md
+33 -3
View File
@@ -4,18 +4,23 @@
```py
reveal_type(not None) # revealed: Literal[True]
reveal_type(not not None) # revealed: Literal[False]
```
## Function
```py
from typing_extensions import reveal_type
def f():
return 1
# error: [redundant-condition] "always truthy"
reveal_type(not f) # revealed: Literal[False]
# TODO Unknown should not be part of the type of typing.reveal_type
# reveal_type(not reveal_type) revealed: Literal[False]
# error: [redundant-condition] "always truthy"
reveal_type(not reveal_type) # revealed: Literal[False]
```
## Module
@@ -24,7 +29,10 @@ reveal_type(not f) # revealed: Literal[False]
import b
import warnings
# error: [redundant-condition] "always truthy"
reveal_type(not b) # revealed: Literal[False]
# error: [redundant-condition] "always truthy"
reveal_type(not warnings) # revealed: Literal[False]
```
@@ -61,11 +69,15 @@ def _(flag: bool):
r = ""
s = 0
t = ""
# error: [redundant-condition] "always truthy"
reveal_type(not p) # revealed: Literal[False]
reveal_type(not q) # revealed: bool
reveal_type(not r) # revealed: bool
reveal_type(not s) # revealed: bool
# error: [redundant-condition] "always falsy"
reveal_type(not t) # revealed: Literal[True]
```
@@ -97,29 +109,43 @@ reveal_type(not x) # revealed: Literal[True]
## String literal
```py
# error: [redundant-condition] "always truthy"
reveal_type(not "hello") # revealed: Literal[False]
# error: [redundant-condition] "always falsy"
reveal_type(not "") # revealed: Literal[True]
# error: [redundant-condition] "always truthy"
reveal_type(not "0") # revealed: Literal[False]
# error: [redundant-condition] "always truthy"
reveal_type(not "hello" + "world") # revealed: Literal[False]
```
## Bytes literal
```py
# error: [redundant-condition] "always truthy"
reveal_type(not b"hello") # revealed: Literal[False]
# error: [redundant-condition] "always falsy"
reveal_type(not b"") # revealed: Literal[True]
# error: [redundant-condition] "always truthy"
reveal_type(not b"0") # revealed: Literal[False]
# error: [redundant-condition] "always truthy"
reveal_type(not b"hello" + b"world") # revealed: Literal[False]
```
## Tuple
```py
# error: [redundant-condition] "Expression `(1,)` is always truthy (has type `tuple[Literal[1]]`)"
reveal_type(not (1,)) # revealed: Literal[False]
# error: [redundant-condition] "Expression `(1, 2)` is always truthy (has type `tuple[Literal[1], Literal[2]]`)"
reveal_type(not (1, 2)) # revealed: Literal[False]
# error: [redundant-condition] "Expression `(1, 2, 3)` is always truthy (has type `tuple[Literal[1], Literal[2], Literal[3]]`)"
reveal_type(not (1, 2, 3)) # revealed: Literal[False]
# error: [redundant-condition] "Expression `()` is always falsy (has type `tuple[()]`)"
reveal_type(not ()) # revealed: Literal[True]
# error: [redundant-condition] "always truthy"
reveal_type(not ("hello",)) # revealed: Literal[False]
# error: [redundant-condition] "always truthy"
reveal_type(not (1, "hello")) # revealed: Literal[False]
```
@@ -142,6 +168,7 @@ class AlwaysTrue:
def __bool__(self) -> Literal[True]:
return True
# error: [redundant-condition] "always truthy"
# revealed: Literal[False]
reveal_type(not AlwaysTrue())
@@ -149,6 +176,7 @@ class AlwaysFalse:
def __bool__(self) -> Literal[False]:
return False
# error: [redundant-condition] "always falsy"
# revealed: Literal[True]
reveal_type(not AlwaysFalse())
@@ -181,6 +209,7 @@ class WithBothLenAndBool1:
def __len__(self) -> Literal[2]:
return 2
# error: [redundant-condition] "always falsy"
# revealed: Literal[True]
reveal_type(not WithBothLenAndBool1())
@@ -191,6 +220,7 @@ class WithBothLenAndBool2:
def __len__(self) -> Literal[0]:
return 0
# error: [redundant-condition]
# revealed: Literal[False]
reveal_type(not WithBothLenAndBool2())
crates/ty_python_semantic/src/db.rs
+11
View File
@@ -234,6 +234,7 @@ pub(crate) mod tests {
files: Vec<(&'a str, &'a str)>,
/// Whether module resolution should include packages from the synthetic virtual environment.
third_party_packages: bool,
rule_selection: Option<RuleSelection>,
}
impl<'a> TestDbBuilder<'a> {
@@ -244,6 +245,7 @@ pub(crate) mod tests {
src_roots: vec![SystemPathBuf::from("/src")],
files: vec![],
third_party_packages: false,
rule_selection: None,
}
}
@@ -262,6 +264,11 @@ pub(crate) mod tests {
self
}
pub(crate) fn with_rule_selection(mut self, selection: RuleSelection) -> Self {
self.rule_selection = Some(selection);
self
}
pub(crate) fn with_file(
mut self,
path: &'a (impl AsRef<SystemPath> + ?Sized),
@@ -283,6 +290,10 @@ pub(crate) mod tests {
pub(crate) fn build(self) -> anyhow::Result<TestDb> {
let mut db = TestDb::new();
if let Some(selection) = self.rule_selection {
db.rule_selection = Arc::new(selection);
}
for src_root in &self.src_roots {
db.memory_file_system().create_directory_all(src_root)?;
}
crates/ty_python_semantic/src/reachability.rs
+19 -10
View File
@@ -1763,6 +1763,20 @@ fn analyze_non_terminal_call<'db>(
// add them on all statement-level function calls.
let ty = infer_same_file_expression_type(db, callable, TypeContext::default());
is_non_terminal_call(db, &env, ty, is_await, || {
infer_same_file_expression_type(db, call_expr, TypeContext::default())
})
}
/// Shares terminal-call classification between scope reachability and defensive-check exemptions.
/// The result type is only needed when overload selection, generics, or awaiting can affect it.
pub(crate) fn is_non_terminal_call<'db>(
db: &'db dyn Db,
env: &ProgramEnvironment<'db>,
ty: Type<'db>,
is_await: bool,
call_type: impl FnOnce() -> Type<'db>,
) -> Truthiness {
// Short-circuit for well-known types that are known not to return `Never` when called. Without
// the short-circuit, we've seen that threads keep blocking each other because they all try to
// acquire Salsa's `CallableType` lock that ensures each type is only interned once. The lock is
@@ -1773,7 +1787,7 @@ fn analyze_non_terminal_call<'db>(
}
let overloads_iterator = if let Some(callable) = ty
.try_upcast_to_callable(db, &env)
.try_upcast_to_callable(db, env)
.and_then(CallableTypes::exactly_one)
{
callable.signatures(db).overloads.iter()
@@ -1786,23 +1800,18 @@ fn analyze_non_terminal_call<'db>(
let mut any_overload_is_generic = false;
for overload in overloads_iterator {
let returns_never = overload.return_ty.is_equivalent_to(db, &env, Type::Never);
let returns_never = overload.return_ty.is_equivalent_to(db, env, Type::Never);
no_overloads_return_never &= !returns_never;
all_overloads_return_never &= returns_never;
any_overload_is_generic |= overload.return_ty.has_typevar(db, &env);
any_overload_is_generic |= overload.return_ty.has_typevar(db, env);
}
if no_overloads_return_never && !any_overload_is_generic && !is_await {
Truthiness::AlwaysTrue
} else if all_overloads_return_never {
} else if all_overloads_return_never || call_type().is_equivalent_to(db, env, Type::Never) {
Truthiness::AlwaysFalse
} else {
let call_expr_ty = infer_same_file_expression_type(db, call_expr, TypeContext::default());
if call_expr_ty.is_equivalent_to(db, &env, Type::Never) {
Truthiness::AlwaysFalse
} else {
Truthiness::AlwaysTrue
}
Truthiness::AlwaysTrue
}
}
crates/ty_python_semantic/src/types.rs
+5
View File
@@ -2798,6 +2798,11 @@ impl<'db> Type<'db> {
}
}
fn is_int_literal(&self) -> bool {
self.as_literal_value()
.is_some_and(LiteralValueType::is_int)
}
fn as_int_literal(self) -> Option<i64> {
match self {
Type::LiteralValue(literal) => literal.as_int(),
crates/ty_python_semantic/src/types/call/bind.rs
+4 -1
View File
@@ -8579,7 +8579,10 @@ impl<'db> CallableDescription<'db> {
}
}
fn defining_class(db: &'db dyn Db, callable_type: Type<'db>) -> Option<ClassLiteral<'db>> {
pub(crate) fn defining_class(
db: &'db dyn Db,
callable_type: Type<'db>,
) -> Option<ClassLiteral<'db>> {
let function = match callable_type {
Type::FunctionLiteral(function) => function,
Type::BoundMethod(method) => method.function(db),
crates/ty_python_semantic/src/types/class/static_literal.rs
+38
View File
@@ -448,6 +448,44 @@ impl<'db> StaticClassLiteral<'db> {
inherited_legacy_generic_context_inner(db, self)
}
/// Iterate through the decorators on this class, returning the span of the first one
/// that matches the given predicate.
fn find_decorator_span(
self,
db: &'db dyn Db,
predicate: impl Fn(Type<'db>) -> bool,
) -> Option<Span> {
if !self.has_decorators(db) {
return None;
}
let definition = self.definition(db);
let file = definition.file(db);
self.node(db, &parsed_module(db, definition.python_file(db)).load(db))
.decorator_list
.iter()
.find(|decorator| {
predicate(definition_expression_type(
db,
definition,
&decorator.expression,
))
})
.map(|decorator| Span::from(file).with_range(decorator.range))
}
/// Iterate through the decorators on this class, returning the span of the first one
/// that matches the given [`KnownFunction`].
pub(crate) fn find_known_decorator_span(
self,
db: &'db dyn Db,
needle: KnownFunction,
) -> Option<Span> {
self.find_decorator_span(db, |ty| {
ty.as_function_literal()
.is_some_and(|f| f.is_known(db, needle))
})
}
/// Returns all of the typevars that are referenced in this class's base class list.
/// (This is used to ensure that classes do not reference typevars from enclosing
/// generic contexts.)
crates/ty_python_semantic/src/types/diagnostic.rs
+20
View File
@@ -186,6 +186,8 @@ pub(crate) fn register_lints(registry: &mut LintRegistryBuilder) {
registry.register_lint(&INVALID_FROZEN_DATACLASS_SUBCLASS);
registry.register_lint(&INVALID_TOTAL_ORDERING);
registry.register_lint(&INVALID_LEGACY_POSITIONAL_PARAMETER);
registry.register_lint(&REDUNDANT_CONDITION);
registry.register_lint(&REDUNDANT_CONDITION_STRICT);
// String annotations
registry.register_lint(&ESCAPE_CHARACTER_IN_FORWARD_ANNOTATION);
@@ -1369,6 +1371,24 @@ declare_lint! {
}
}
declare_lint! {
#[doc = include_str!("../../resources/lint_docs/redundant-condition.md")]
pub(crate) static REDUNDANT_CONDITION = {
summary: "detects conditions that are always truthy or always falsey",
status: LintStatus::stable("0.0.79"),
default_level: Level::Warn,
}
}
declare_lint! {
#[doc = include_str!("../../resources/lint_docs/redundant-condition-strict.md")]
pub(crate) static REDUNDANT_CONDITION_STRICT = {
summary: "detects conditions that are always truthy or always falsey (strict)",
status: LintStatus::stable("0.0.79"),
default_level: Level::Ignore,
}
}
/// A collection of type check diagnostics.
#[derive(Default, Eq, PartialEq, get_size2::GetSize)]
pub struct TypeCheckDiagnostics {
crates/ty_python_semantic/src/types/infer.rs
+122 -25
View File
@@ -1314,6 +1314,10 @@ struct DeferredAndUndecorated<'db> {
#[derive(Debug, Eq, PartialEq, get_size2::GetSize, Default, salsa::SalsaValue)]
struct OtherDefinitionInferenceExtra<'db> {
/// Condition truthiness retained for checks of enclosing conditions containing walrus expressions.
/// See [`ExpressionInferenceExtra::comparison_truthiness`] for the distinction from value types.
comparison_truthiness: FrozenMap<ExpressionNodeKey, Truthiness>,
/// String annotations found in this region
string_annotations: FrozenSet<ExpressionNodeKey>,
@@ -1409,6 +1413,13 @@ impl<'db> DefinitionInferenceExtra<'db> {
_ => None,
}
}
fn comparison_truthiness(&self) -> Option<&FrozenMap<ExpressionNodeKey, Truthiness>> {
match self {
Self::Other(extra) => Some(&extra.comparison_truthiness),
_ => None,
}
}
}
impl<'db> DefinitionInference<'db> {
@@ -1501,6 +1512,10 @@ impl<'db> DefinitionInference<'db> {
definition: Definition<'db>,
) -> DefinitionInference<'db> {
let env = ProgramEnvironment::from_definition(definition);
if cycle.iteration() > crate::TAINTED_CYCLES {
self.widen_comparison_truthiness(db, &env, previous_inference);
}
for (expr, ty) in &mut self.expressions {
let previous_ty = previous_inference.expression_type(*expr);
*ty = ty.cycle_normalized(db, &env, previous_ty, cycle);
@@ -1548,6 +1563,35 @@ impl<'db> DefinitionInference<'db> {
self
}
fn widen_comparison_truthiness(
&mut self,
db: &'db dyn Db,
env: &ProgramEnvironment<'db>,
previous: &Self,
) {
let comparison_truthiness = widen_comparison_truthiness(
self.extra
.as_deref()
.and_then(DefinitionInferenceExtra::comparison_truthiness),
previous
.extra
.as_deref()
.and_then(DefinitionInferenceExtra::comparison_truthiness),
|expression| self.expression_type(expression).bool(db, env),
|expression| previous.expression_type(expression).bool(db, env),
);
if comparison_truthiness.iter().next().is_some() {
let mut extra = self
.extra
.take()
.map_or_else(OtherDefinitionInferenceExtra::default, |extra| {
extra.into_other()
});
extra.comparison_truthiness = comparison_truthiness;
self.extra = Some(Box::new(DefinitionInferenceExtra::Other(Box::new(extra))));
}
}
pub(crate) fn expression_type(&self, expression: impl Into<ExpressionNodeKey>) -> Type<'db> {
self.try_expression_type(expression)
.unwrap_or_else(Type::unknown)
@@ -1709,6 +1753,43 @@ impl<'db> DefinitionInference<'db> {
}
}
/// Widen comparison truthiness for expression, statement, and definition regions using the same rules.
///
/// Sparse overrides can appear or disappear as operand types change. Compare the effective
/// truthiness in both iterations, including previous-only overrides, so widening cannot make
/// a condition alternate between definite outcomes. The fallbacks use each iteration's value
/// types before those types are themselves widened.
fn widen_comparison_truthiness(
current: Option<&FrozenMap<ExpressionNodeKey, Truthiness>>,
previous: Option<&FrozenMap<ExpressionNodeKey, Truthiness>>,
current_fallback: impl Fn(ExpressionNodeKey) -> Truthiness,
previous_fallback: impl Fn(ExpressionNodeKey) -> Truthiness,
) -> FrozenMap<ExpressionNodeKey, Truthiness> {
current
.into_iter()
.chain(previous)
.flatten()
.map(|(expression, _)| {
let truthiness = current
.and_then(|overrides| overrides.get(expression))
.copied()
.unwrap_or_else(|| current_fallback(*expression));
let previous_truthiness = previous
.and_then(|overrides| overrides.get(expression))
.copied()
.unwrap_or_else(|| previous_fallback(*expression));
(
*expression,
if truthiness == previous_truthiness {
truthiness
} else {
Truthiness::Ambiguous
},
)
})
.collect()
}
/// The inferred types for an expression region.
#[derive(Debug, Eq, PartialEq, get_size2::GetSize, salsa::SalsaValue)]
pub(crate) struct ExpressionInference<'db> {
@@ -1838,37 +1919,23 @@ impl<'db> ExpressionInference<'db> {
self
}
/// Sparse overrides can appear or disappear as operand types change. Compare the effective
/// truthiness in both iterations, including previous-only overrides, so widening cannot make
/// a condition alternate between definite outcomes.
fn widen_comparison_truthiness(
&mut self,
db: &'db dyn Db,
env: &ProgramEnvironment<'db>,
previous: &Self,
) {
let comparison_truthiness: FrozenMap<_, _> = self
.extra
.iter()
.chain(previous.extra.iter())
.flat_map(|extra| &extra.comparison_truthiness)
.map(|(expression, _)| {
let truthiness = self
.comparison_truthiness(*expression)
.unwrap_or_else(|| self.expression_type(*expression).bool(db, env));
let previous_truthiness = previous
.comparison_truthiness(*expression)
.unwrap_or_else(|| previous.expression_type(*expression).bool(db, env));
(
*expression,
if truthiness == previous_truthiness {
truthiness
} else {
Truthiness::Ambiguous
},
)
})
.collect();
let comparison_truthiness = widen_comparison_truthiness(
self.extra
.as_deref()
.map(|extra| &extra.comparison_truthiness),
previous
.extra
.as_deref()
.map(|extra| &extra.comparison_truthiness),
|expression| self.expression_type(expression).bool(db, env),
|expression| previous.expression_type(expression).bool(db, env),
);
if comparison_truthiness.iter().next().is_some() {
self.extra.get_or_insert_default().comparison_truthiness = comparison_truthiness;
}
@@ -1972,6 +2039,10 @@ pub(crate) struct StatementInferenceInner<'db> {
#[derive(Debug, Eq, PartialEq, get_size2::GetSize, Default, salsa::SalsaValue)]
struct StatementInferenceInnerExtra<'db> {
/// Condition truthiness retained for checks performed by the enclosing suite.
/// See [`ExpressionInferenceExtra::comparison_truthiness`] for the distinction from value types.
comparison_truthiness: FrozenMap<ExpressionNodeKey, Truthiness>,
/// String annotations found in this region
string_annotations: FrozenSet<ExpressionNodeKey>,
@@ -2028,6 +2099,10 @@ impl<'db> StatementInferenceInner<'db> {
previous_inference: &StatementInferenceInner<'db>,
cycle: &salsa::Cycle,
) -> StatementInferenceInner<'db> {
if cycle.iteration() > crate::TAINTED_CYCLES {
self.widen_comparison_truthiness(db, env, previous_inference);
}
for (expr, ty) in &mut self.expressions {
let previous_ty = previous_inference.expression_type(*expr);
*ty = ty.cycle_normalized(db, env, previous_ty, cycle);
@@ -2072,6 +2147,28 @@ impl<'db> StatementInferenceInner<'db> {
self
}
fn widen_comparison_truthiness(
&mut self,
db: &'db dyn Db,
env: &ProgramEnvironment<'db>,
previous: &Self,
) {
let comparison_truthiness = widen_comparison_truthiness(
self.extra
.as_deref()
.map(|extra| &extra.comparison_truthiness),
previous
.extra
.as_deref()
.map(|extra| &extra.comparison_truthiness),
|expression| self.expression_type(expression).bool(db, env),
|expression| previous.expression_type(expression).bool(db, env),
);
if comparison_truthiness.iter().next().is_some() {
self.extra.get_or_insert_default().comparison_truthiness = comparison_truthiness;
}
}
fn expression_type(&self, expression: impl Into<ExpressionNodeKey>) -> Type<'db> {
self.try_expression_type(expression)
.unwrap_or_else(Type::unknown)
crates/ty_python_semantic/src/types/infer/builder.rs
+30 -12
View File
@@ -171,6 +171,7 @@ mod named_tuple;
mod new_class;
mod paramspec_validation;
mod post_inference;
mod redundant_conditions;
mod subscript;
mod type_call;
mod type_expression;
@@ -624,6 +625,8 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
DefinitionInferenceExtra::Undecorated(_)
| DefinitionInferenceExtra::DiscardsDictKeyAssignments => {}
DefinitionInferenceExtra::Other(extra) => {
self.comparison_truthiness
.extend(extra.comparison_truthiness.iter().copied());
self.called_functions
.extend(extra.called_functions.iter().copied());
self.extend_cycle_recovery(extra.cycle_recovery);
@@ -673,6 +676,8 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
}
if let Some(extra) = &inference.extra {
self.comparison_truthiness
.extend(extra.comparison_truthiness.iter().copied());
self.called_functions
.extend(extra.called_functions.iter().copied());
self.return_types_and_ranges
@@ -2146,6 +2151,8 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
}
}
}
self.check_suite_for_redundant_conditions(suite);
}
fn infer_statement(&mut self, statement: &ast::Stmt) {
@@ -2726,6 +2733,8 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
if let Err(err) = guard_ty.try_bool(db, self.program_environment()) {
err.report_diagnostic(&self.context, guard);
}
self.check_condition_redundancy(guard, guard_ty);
}
self.infer_body(body);
@@ -8406,6 +8415,8 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
if let Err(err) = test_ty.try_bool(db, env) {
err.report_diagnostic(&self.context, expr);
}
self.check_condition_redundancy(expr, test_ty);
}
}
@@ -8563,6 +8574,9 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
err.fallback_truthiness()
}
};
self.check_condition_redundancy(test, test_ty);
match test_truthiness {
Truthiness::AlwaysTrue => body_ty,
Truthiness::AlwaysFalse => orelse_ty,
@@ -11254,16 +11268,16 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
))
}
(ast::UnaryOp::Not, ty) => Type::from_truthiness(
db,
env,
ty.try_bool(db, env)
.unwrap_or_else(|err| {
err.report_diagnostic(&self.context, unary);
err.fallback_truthiness()
})
.negate(),
),
(ast::UnaryOp::Not, ty) => {
let original_truthiness = ty.try_bool(db, env).unwrap_or_else(|err| {
err.report_diagnostic(&self.context, unary);
err.fallback_truthiness()
});
self.check_negation_redundancy(unary, ty, original_truthiness);
Type::from_truthiness(db, env, original_truthiness.negate())
}
// Handle constrained TypeVars specially: check each constraint individually.
//
// TODO: We expect to replace this with more general support once we migrate to the new
@@ -11739,7 +11753,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
let Self {
context,
expressions,
comparison_truthiness: _,
comparison_truthiness,
qualifiers,
type_expression_flags,
mut collection_use_constraints,
@@ -11772,6 +11786,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
let diagnostics = context.finish();
let extra = (!diagnostics.is_empty()
|| !comparison_truthiness.is_empty()
|| !string_annotations.is_empty()
|| cycle_recovery.is_some()
|| !expected_types.is_empty()
@@ -11785,6 +11800,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
collection_use_constraints.shrink_to_fit();
return_types_and_ranges.shrink_to_fit();
Box::new(StatementInferenceInnerExtra {
comparison_truthiness: FrozenMap::from(comparison_truthiness),
string_annotations: FrozenSet::from(string_annotations),
expected_types: FrozenMap::from(expected_types),
called_functions: called_functions
@@ -11904,7 +11920,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
let Self {
context,
expressions,
comparison_truthiness: _,
comparison_truthiness,
qualifiers,
type_expression_flags,
mut collection_use_constraints,
@@ -11935,6 +11951,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
let diagnostics = context.finish();
let non_undecorated_extra_field_count = usize::from(!string_annotations.is_empty())
+ usize::from(!comparison_truthiness.is_empty())
+ usize::from(!expected_types.is_empty())
+ usize::from(!collection_use_constraints.is_empty())
+ usize::from(!called_functions.is_empty())
@@ -11988,6 +12005,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
(_, undecorated_type) => {
collection_use_constraints.shrink_to_fit();
let extra = OtherDefinitionInferenceExtra {
comparison_truthiness: FrozenMap::from(comparison_truthiness),
string_annotations: FrozenSet::from(string_annotations),
expected_types: FrozenMap::from(expected_types),
collection_use_constraints,
crates/ty_python_semantic/src/types/infer/builder/redundant_conditions/diagnostic.rs
+605
View File
@@ -0,0 +1,605 @@
//! Diagnostic messages and fixes for conditions selected by the redundant-condition checker.
use std::borrow::Cow;
use ruff_db::{
diagnostic::{Annotation, Span, SubDiagnostic, SubDiagnosticSeverity},
parsed::parsed_module,
source::source_text,
};
use ruff_diagnostics::{Applicability, Edit, Fix};
use ruff_python_ast as ast;
use ruff_source_file::LineRanges;
use ruff_text_size::Ranged;
use ty_python_core::Truthiness;
use crate::{
SemanticModel,
types::{
KnownClass, LintDiagnosticGuard, LintDiagnosticGuardBuilder, MemberLookupPolicy, Type,
call::bind::CallableDescription, enum_metadata, function::KnownFunction,
infer::TypeInferenceBuilder, signatures::CallableSignature, tuple::TupleLength,
},
};
use super::RedundantCondition;
impl<'db> TypeInferenceBuilder<'db, '_> {
pub(super) fn report_redundant_condition<'ctx>(
&'ctx self,
condition: &RedundantCondition<'_, 'db>,
) -> Option<LintDiagnosticGuard<'ctx, 'ctx>> {
#[derive(Debug)]
enum FunctionInfo<'db> {
Function(&'db CallableSignature<'db>, &'db str),
Method(&'db CallableSignature<'db>, Cow<'db, str>),
Lambda(&'db CallableSignature<'db>),
}
impl<'db> FunctionInfo<'db> {
fn kind(&self) -> &'static str {
match self {
FunctionInfo::Function(..) | FunctionInfo::Lambda(..) => "function",
FunctionInfo::Method(_, _) => "method",
}
}
fn signature(&self) -> &'db CallableSignature<'db> {
match self {
FunctionInfo::Function(signature, _) => signature,
FunctionInfo::Method(signature, _) => signature,
FunctionInfo::Lambda(signature) => signature,
}
}
}
impl std::fmt::Display for FunctionInfo<'_> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
FunctionInfo::Function(_, name) => write!(f, "Function `{name}`"),
FunctionInfo::Method(_, name) => write!(f, "Method `{name}`"),
FunctionInfo::Lambda(_) => write!(f, "Function object"),
}
}
}
let RedundantCondition {
expression: test,
value_type: test_type,
is_truthy,
kind,
} = condition;
let rule = kind.rule();
let db = self.db();
let env = self.program_environment();
// Quoting a nested test identifies which part of the enclosing condition is redundant.
let should_quote_test_expression = || {
!matches!(test, ast::Expr::Name(_) | ast::Expr::Attribute(_))
|| !self.index.is_boolean_test_root(test)
};
let annotate_inferred_type = |diagnostic: &mut LintDiagnosticGuard| {
diagnostic.set_primary_annotation_message(format_args!(
"Inferred type is `{}`",
test_type.display(db, env)
));
};
let describe_condition = |diagnostic: &mut LintDiagnosticGuard| {
let source = source_text(db, self.file());
if source.contains_line_break(test.range()) {
diagnostic.set_concise_message(format_args!("Condition is always {is_truthy}"));
} else {
diagnostic.set_concise_message(format_args!(
"Condition `{}` is always {is_truthy}",
&source[test.range()]
));
}
if let ast::Expr::Compare(ast::ExprCompare {
left,
ops,
comparators,
..
}) = test
&& ops.len() == 1
&& let [single_comparator] = &**comparators
{
if let (Type::LiteralValue(left_type), Type::LiteralValue(right_type)) = (
self.expression_type(left),
self.expression_type(single_comparator),
) && ((left_type.is_string() && (right_type.is_bytes() || right_type.is_int()))
|| ((left_type.is_bytes() || left_type.is_int()) && right_type.is_string()))
{
// For the specific case of a string-literal type compared with a bytes-literal type,
// cite their nominal-instance supertypes rather than their `Literal` types,
// since their `Literal` types look quite similar in their display representations.
for node in [left, single_comparator] {
if let Some(class) = self.expression_type(node).nominal_class(db, env) {
diagnostic.annotate(
self.context
.secondary(node)
.message(format_args!("Instance of `{}`", class.name(db))),
);
}
}
} else {
for node in [left, single_comparator] {
match node {
ast::Expr::NoneLiteral(_)
| ast::Expr::BooleanLiteral(_)
| ast::Expr::NumberLiteral(_) => {}
ast::Expr::UnaryOp(ast::ExprUnaryOp {
op: ast::UnaryOp::USub | ast::UnaryOp::UAdd,
operand,
..
}) if matches!(&**operand, ast::Expr::NumberLiteral(_)) => {}
_ => {
diagnostic.annotate(self.context.secondary(node).message(
format_args!(
"Has type `{}`",
self.expression_type(node).display(db, env)
),
));
}
}
}
}
} else {
annotate_inferred_type(diagnostic);
}
};
// Short-circuit evaluation can determine a condition's truthiness even when its
// value type does not. In that case, describe the condition rather than the type.
let describe_as_condition =
test_type.is_subtype_of(db, env, KnownClass::Bool.to_instance(db, env))
|| test_type.bool(db, env) != Truthiness::from(*is_truthy);
let builder = self.context.report_lint(rule, test)?;
let diagnostic = if *is_truthy {
let add_always_truthy_concise_message = |diagnostic: &mut LintDiagnosticGuard| {
if should_quote_test_expression()
&& let source = source_text(db, self.file())
&& !source.contains_line_break(test.range())
{
diagnostic.set_concise_message(format_args!(
"{} `{}` is always truthy (has type `{}`)",
if matches!(test, ast::Expr::Name(_)) {
"Variable"
} else {
"Expression"
},
&source[test.range()],
test_type.display(db, env)
));
} else {
diagnostic.set_concise_message(format_args!(
"Object of type `{}` is always truthy",
test_type.display(db, env)
));
}
};
let function_info = match test_type {
Type::FunctionLiteral(function) => Some(FunctionInfo::Function(
function.signature(db),
function.name(db),
)),
Type::BoundMethod(method) => {
let function = method.function(db);
Some(FunctionInfo::Method(
method.bound_signatures(db),
CallableDescription::defining_class(db, *test_type)
.map(|class| {
Cow::Owned(format!("{}.{}", class.name(db), function.name(db)))
})
.unwrap_or(Cow::Borrowed(&**function.name(db))),
))
}
Type::Callable(callable) if callable.is_function_like(db) => {
Some(FunctionInfo::Lambda(callable.signatures(db)))
}
_ => None,
};
if let Some(function) = function_info {
let mut diagnostic =
builder.into_diagnostic(format_args!("{function} is always truthy"));
// Add a suggestion and fix that they might have meant to call this function.
//
// It's true that calling the function might not actually fix this diagnostic
// if the function returns something that is always truthy. They still probably
// meant to call the function, though, so it's still a useful suggestion/fix!
diagnostic.set_primary_annotation_message(format_args!(
"Did you mean to call this {}?",
function.kind()
));
if matches!(test, ast::Expr::Name(_) | ast::Expr::Attribute(_)) {
let (call, applicability) = if function.signature().has_parameters() {
("(...)", Applicability::DisplayOnly)
} else {
("()", Applicability::Unsafe)
};
let call_edit = Edit::insertion(call.to_string(), test.end());
diagnostic.set_fix(Fix::applicable_edit(call_edit, applicability));
}
diagnostic
} else if let Some(tuple_spec) = test_type.tuple_instance_spec(db, env)
&& tuple_spec.len().minimum() > 0
{
// This error message might not be 100% accurate for a tuple subclass
// that overrides `__len__` or `__bool__` in a way that's inconsistent
// with the tuple's inherited tuple spec, but you just shouldn't do that anyway.
let length = tuple_spec.len();
let message = match length {
TupleLength::Fixed(size) => {
format!("A {size}-element tuple is always truthy")
}
TupleLength::Variable(min, _) => format!(
"A tuple with >={min} element{maybe_s} is always truthy",
maybe_s = if min == 1 { "" } else { "s" }
),
};
let mut diagnostic = builder.into_diagnostic(&message);
// If the tuple has a small number of fixed elements,
// describe the whole type of the tuple in the concise message.
// Otherwise, avoid printing the full tuple type in the concise message here
// (since it can be very long in some cases)
if tuple_spec.fixed_elements().len() <= 8 {
add_always_truthy_concise_message(&mut diagnostic);
} else {
diagnostic.set_concise_message(&message);
}
annotate_inferred_type(&mut diagnostic);
diagnostic
} else if let Type::TypedDict(typed_dict) = test_type
&& let Some(field) = typed_dict
.items(db)
.iter()
.find_map(|(_, field)| field.is_required().then_some(field))
{
let num_required_keys = typed_dict
.items(db)
.iter()
.filter(|(_, field)| field.is_required())
.count();
let maybe_s = if num_required_keys == 1 { "" } else { "s" };
let mut diagnostic = builder.into_diagnostic(format_args!(
"A TypedDict with {num_required_keys} required field{maybe_s} is always truthy"
));
if let Some(class) = typed_dict.defining_class() {
diagnostic.set_concise_message(format_args!(
"TypedDict `{}` with {num_required_keys} required field{maybe_s} is always truthy",
class.name(db)
));
} else {
diagnostic.set_concise_message(format_args!(
"A TypedDict with {num_required_keys} required field{maybe_s} is always truthy"
));
}
annotate_inferred_type(&mut diagnostic);
if let Some(defining_class) = typed_dict.defining_class()
&& let Some(typed_dict_definition) = defining_class.definition(db)
&& let Some(field_definition) = field.first_declaration()
{
let typed_dict_module =
parsed_module(db, typed_dict_definition.python_file(db)).load(db);
let field_module = parsed_module(db, field_definition.python_file(db)).load(db);
diagnostic.annotate(
Annotation::secondary(Span::from(
typed_dict_definition.focus_range(db, &typed_dict_module),
))
.message(format_args!("`{}` defined here", defining_class.name(db))),
);
diagnostic.annotate(
Annotation::secondary(Span::from(
field_definition.full_range(db, &field_module),
))
.message(if num_required_keys == 1 {
"Required field declared here"
} else {
"First required field defined here"
}),
);
}
diagnostic
} else if test_type.as_nominal_instance().is_some_and(|instance| {
instance
.class(db, env)
.is_known(db, KnownClass::GeneratorType)
}) {
let mut diagnostic = builder.into_diagnostic("A generator is always truthy");
add_always_truthy_concise_message(&mut diagnostic);
annotate_inferred_type(&mut diagnostic);
diagnostic.help("Did you mean to use `any()`?");
if SemanticModel::new(db, self.program_file())
.definitely_has_builtin_binding("any", ast::AnyNodeRef::from(*test))
{
// display-only edits rather than unsafe edits
// because we don't know what the user *really* wanted here!
// Collecting the result into a `tuple` is also a very plausible thing
// they might have wanted to do (a lot of folks think that generator expressions
// are actually "tuple comprehensions").
diagnostic.set_fix(Fix::display_only_edits(
Edit::insertion("any(".to_string(), test.start()),
[Edit::insertion(")".to_string(), test.end())],
));
}
diagnostic
} else if test_type.is_string_literal()
|| test_type
.as_union()
.is_some_and(|union| union.elements(db).iter().all(Type::is_string_literal))
{
let mut diagnostic = builder.into_diagnostic("A nonempty string is always truthy");
let source = source_text(db, self.file());
if source.contains_line_break(test.range()) {
diagnostic.set_concise_message(format_args!(
"Nonempty string of type `{}` is always truthy",
test_type.display(db, env)
));
} else if test.is_string_literal_expr() {
diagnostic.set_concise_message(format_args!(
"String literal {} is always truthy",
&source[test.range()]
));
} else {
diagnostic.set_concise_message(format_args!(
"{} `{}` is always truthy (has type `{}`)",
if matches!(test, ast::Expr::Name(_) | ast::Expr::Attribute(_)) {
"Nonempty string"
} else {
"Expression"
},
&source[test.range()],
test_type.display(db, env)
));
}
annotate_inferred_type(&mut diagnostic);
diagnostic
} else if describe_as_condition
&& let Some((subexpr, subexpr_type, subexpr_length)) =
self.length_test_against_type_with_known_length(test)
{
self.report_redundant_length_comparison(
test,
subexpr,
subexpr_type,
subexpr_length,
builder,
)
} else if describe_as_condition {
let message = "Condition is always true";
let mut diagnostic = builder.into_diagnostic(message);
describe_condition(&mut diagnostic);
diagnostic
} else {
let mut diagnostic = builder.into_diagnostic("Condition is always truthy");
add_always_truthy_concise_message(&mut diagnostic);
annotate_inferred_type(&mut diagnostic);
if let Type::NominalInstance(instance) = test_type {
let class = instance.class(db, env);
if class.is_final(db)
&& !class.is_known(db, KnownClass::CoroutineType)
&& ["__bool__", "__len__"].into_iter().all(|name| {
test_type
.member_lookup_with_policy(
db,
env,
name,
MemberLookupPolicy::NO_INSTANCE_FALLBACK,
)
.is_undefined()
})
{
let class_name = class.name(db);
let mut sub = SubDiagnostic::new(
SubDiagnosticSeverity::Info,
format_args!(
"`{class_name}` instances are always truthy because \
`{class_name}` cannot be subclassed and does not define \
`__bool__` or `__len__`",
),
);
let class_literal = class.class_literal(db);
let header_range = class_literal.header_range(db);
let final_decorator_range = class_literal
.as_static()
.and_then(|static_class| {
static_class.find_known_decorator_span(db, KnownFunction::Final)
})
.and_then(|span| span.range());
let range = final_decorator_range
.map(|decorator_range| header_range.cover(decorator_range))
.unwrap_or(header_range);
sub.annotate(
Annotation::primary(
Span::from(class_literal.file(db)).with_range(range),
)
.message(format_args!("`{class_name}` defined here")),
);
diagnostic.sub(sub);
if final_decorator_range.is_none()
&& enum_metadata(db, class_literal)
.is_some_and(|metadata| !metadata.members.is_empty())
{
diagnostic.info(format_args!(
"`{class_name}` cannot be subclassed \
because it is an `Enum` subclass and defines enum members"
));
}
}
}
diagnostic
}
} else {
let add_always_falsy_concise_message = |diagnostic: &mut LintDiagnosticGuard| {
if should_quote_test_expression()
&& let source = source_text(db, self.file())
&& !source.contains_line_break(test.range())
{
diagnostic.set_concise_message(format_args!(
"{} `{}` is always falsy (has type `{}`)",
if matches!(test, ast::Expr::Name(_)) {
"Variable"
} else {
"Expression"
},
&source[test.range()],
test_type.display(db, env)
));
} else if test_type.is_none(db) {
diagnostic.set_concise_message("`None` is always falsy");
} else {
diagnostic.set_concise_message(format_args!(
"Object of type `{}` is always falsy",
test_type.display(db, env)
));
}
};
if test_type.is_none(db) {
let mut diagnostic = builder.into_diagnostic("`None` is always falsy");
add_always_falsy_concise_message(&mut diagnostic);
diagnostic
} else if let Some(tuple) = test_type.tuple_instance_spec(db, env)
&& tuple.len() == TupleLength::Fixed(0)
{
// This error message might not be 100% accurate for a tuple subclass
// that overrides `__len__` or `__bool__` in a way that's inconsistent
// with the tuple's inherited tuple spec, but you just shouldn't do that anyway.
let mut diagnostic = builder.into_diagnostic("An empty tuple is always falsy");
add_always_falsy_concise_message(&mut diagnostic);
annotate_inferred_type(&mut diagnostic);
diagnostic
} else if test_type.is_string_literal() {
let mut diagnostic = builder.into_diagnostic("An empty string is always falsy");
add_always_falsy_concise_message(&mut diagnostic);
annotate_inferred_type(&mut diagnostic);
diagnostic
} else if describe_as_condition
&& let Some((subexpr, subexpr_type, subexpr_length)) =
self.length_test_against_type_with_known_length(test)
{
self.report_redundant_length_comparison(
test,
subexpr,
subexpr_type,
subexpr_length,
builder,
)
} else {
let message = if describe_as_condition {
"Condition is always false"
} else {
"Condition is always falsy"
};
let mut diagnostic = builder.into_diagnostic(message);
if describe_as_condition {
describe_condition(&mut diagnostic);
} else {
add_always_falsy_concise_message(&mut diagnostic);
annotate_inferred_type(&mut diagnostic);
}
diagnostic
}
};
Some(diagnostic)
}
/// Return `Some((expr, expr_ty, expr_length))`, where `expr_ty` is the type of an object
/// being tested for length, and `expr_length` is its known length.
///
/// Returns `None` if `test` is not a comparison of the form `len(x) == i` or `len(x) != i`,
/// where `x` has a known length.
fn length_test_against_type_with_known_length<'a>(
&self,
test: &'a ast::Expr,
) -> Option<(&'a ast::Expr, Type<'db>, i64)> {
let db = self.db();
let env = self.program_environment();
if let ast::Expr::Compare(ast::ExprCompare {
left,
ops,
comparators,
..
}) = test
&& let [single_op] = &**ops
&& let [single_comparator] = &**comparators
&& let (ast::Expr::Call(call), other) | (other, ast::Expr::Call(call)) =
(&**left, single_comparator)
&& matches!(single_op, ast::CmpOp::Eq | ast::CmpOp::NotEq)
&& let ast::Arguments { args, keywords, .. } = &call.arguments
&& keywords.is_empty()
&& let [single_arg] = &**args
&& let Type::FunctionLiteral(function) = self.expression_type(&call.func)
&& function.is_known(db, KnownFunction::Len)
&& self.expression_type(other).is_int_literal()
{
let arg_type = self.expression_type(single_arg);
let length = arg_type.len(db, env)?.as_int_literal()?;
Some((single_arg, arg_type, length))
} else {
None
}
}
fn report_redundant_length_comparison<'a>(
&self,
test: &ast::Expr,
test_subexpression: &ast::Expr,
subexpression_type: Type<'db>,
length: i64,
builder: LintDiagnosticGuardBuilder<'a, 'a>,
) -> LintDiagnosticGuard<'a, 'a> {
let db = self.db();
let env = self.program_environment();
let source = source_text(db, self.file());
let mut diagnostic = if !source.contains_line_break(test.range()) {
builder.into_diagnostic(format_args!(
"`{}` always has length {length}",
&source[test_subexpression.range()]
))
} else {
let mut diag =
builder.into_diagnostic(format_args!("Value always has length {length}"));
diag.set_concise_message(format_args!(
"Object of type `{}` always has length {length}",
subexpression_type.display(db, env)
));
diag
};
diagnostic.annotate(
self.context
.secondary(test_subexpression)
.message(format_args!(
"Has type `{}`",
subexpression_type.display(db, env)
)),
);
diagnostic
}
}
crates/ty_python_semantic/src/types/infer/builder/redundant_conditions/exemptions.rs
+711
View File
@@ -0,0 +1,711 @@
//! Redundant-condition exemptions for assertions, environment guards, and defensive exits.
//!
//! "Environment guards" select code for a particular Python version, platform, or type-checking
//! context. For example, `if sys.version_info >= (3, 14)`, `if sys.platform == "win32"`,
//! `if os.name == "posix"`, and `if TYPE_CHECKING` are all environment guards. Their outcomes
//! may be fixed for the configured environment while still serving a purpose in code that is
//! designed to support multiple environments.
use ruff_db::parsed::parsed_module;
use ruff_python_ast::{self as ast, helpers::any_over_expr, name::Name};
use rustc_hash::FxHashMap;
use ty_module_resolver::{KnownModule, file_to_module};
use ty_python_core::{
ProgramFile,
definition::{Definition, DefinitionKind},
place::ScopedPlaceId,
predicate::{PredicateNode, ScopedPredicateId},
reachability_constraints::ScopedReachabilityConstraintId,
scope::ScopeId,
semantic_index, use_def_map,
};
use crate::{
Db, Program, ProgramEnvironment,
types::{
Type, TypeContext,
definition_resolution::{
ImportAliasResolution, ResolvedDefinition, definitions_for_attribute,
definitions_for_name,
},
diagnostic::REDUNDANT_CONDITION_STRICT,
infer::{
TypeInferenceBuilder,
builder::redundant_conditions::{SuiteExitKind, suite_ends_with_exit},
},
infer_definition_types, infer_expression_types,
},
};
use super::{ConditionKind, RedundantCondition};
/// The context in which a boolean test occurs.
///
/// This is used to help determine whether a test should be exempt from one or both
/// redundant-condition rules. For example, the same always-true comparison can be reported in
/// an `if` condition but exempt in an assertion.
///
/// [`ConditionKind`] determines the rule that will be applied if the condition is not exempted.
/// This context determines whether the test serves a purpose that makes reporting it undesirable.
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
pub(super) enum RedundantConditionContext {
/// A boolean test checked without the additional exemptions represented by the other variants.
///
/// This includes ordinary `if` conditions. For example:
///
/// ```python
/// def check(value: str):
/// if isinstance(value, str): # Always true; flagged by `redundant-condition-strict`.
/// print(value)
/// ```
///
/// The special cases for assertions and checks that reject unexpected input are described
/// by [`Self::Assertion`] and [`Self::DefensiveExit`].
Standalone,
/// A test within an assertion, including the complete assertion and tests in call arguments.
///
/// Tests classified as [`ConditionKind::Boolean`] or [`ConditionKind::ShortCircuit`] are exempt.
/// Other always-truthy or always-falsy values remain eligible for `redundant-condition`, or
/// `redundant-condition-strict` if classified as [`ConditionKind::ContainsWalrus`].
///
/// ```python
/// def check(value: int, other: object, flag: bool):
/// assert isinstance(value, int) # Defensive runtime check; exempt.
/// assert flag and (other or True) # No diagnostic on `other or True`.
/// assert other or True # Short-circuit assertion; exempt.
/// ```
///
/// An uncalled function in `assert not ready` is still reported: the function itself is an
/// always-truthy value even though the complete assertion always fails.
Assertion,
/// Whether the branches of an `if` or `elif` test reject unexpected input or an
/// unsupported operation.
///
/// We call that rejection a "defensive exit". For example, a function might raise `TypeError`
/// if its argument has the wrong type. Type annotations do not enforce this at runtime, so
/// the check can still be useful when the function is called from untyped code. We therefore
/// exempt conditions in [`ConditionKind::Boolean`] and [`ConditionKind::ShortCircuit`] when
/// their fixed truthiness rules out taking a defensive branch.
///
/// ```python
/// def check(value: int):
/// # Always false according to the annotation,
/// # but exempted from diagnostics due to the defensive exit in the branch body:
/// if not isinstance(value, int):
/// raise TypeError("expected an integer")
/// ```
///
/// The code that rejects the input can also be in an `else` branch:
///
/// ```python
/// def check(value: int):
/// # Always true according to the annotation,
/// # but exempted from diagnostics due to the defensive exit in the `else`-branch body:
/// if isinstance(value, int):
/// ...
/// else:
/// raise TypeError("expected an integer")
/// ```
///
/// Or after an always-true final `if` or `elif` whose body ends in a recognized exit:
///
/// ```python
/// def check(value: int):
/// # Always true according to the annotation,
/// # but exempted from diagnostics due to the defensive exit in the body
/// # of the "implicit `else`" after the final `if`:
/// if isinstance(value, int): # Always true according to the annotation; exempt.
/// return value
/// raise TypeError("expected an integer")
/// ```
///
/// [`suite_ends_with_exit`] describes the forms of rejection we recognize
/// with [`SuiteExitKind::Defensive`].
///
/// Boolean operands inherit these exemptions even when the complete condition has unknown
/// truthiness. Negation reverses which branch their truthiness selects. Independent tests in
/// call arguments do not inherit the exemptions, and mistakes such as testing an uncalled
/// function can still be reported. Each field records whether that branch ends in a defensive exit.
DefensiveExit {
truthy_branch: bool,
falsy_branch: bool,
},
}
impl RedundantConditionContext {
/// Identify the defensive branches of an `if` or `elif` once for the complete condition.
///
/// This depends on the surrounding statements, not on whether the condition itself has
/// fixed truthiness. A redundant operand can be part of an otherwise ambiguous condition.
pub(super) fn for_if_statement(
builder: &TypeInferenceBuilder<'_, '_>,
body: &[ast::Stmt],
following_clauses: &[ast::ElifElseClause],
following_statements: &[ast::Stmt],
) -> Self {
if !builder.context.is_lint_enabled(&REDUNDANT_CONDITION_STRICT) {
return Self::Standalone;
}
let falsy_suite = match following_clauses {
[else_clause] if else_clause.test.is_none() => Some(else_clause.body.as_slice()),
[] if suite_ends_with_exit(builder, body, SuiteExitKind::Any) => {
Some(following_statements)
}
_ => None,
};
Self::DefensiveExit {
truthy_branch: suite_ends_with_exit(builder, body, SuiteExitKind::Defensive),
falsy_branch: falsy_suite.is_some_and(|suite| {
suite_ends_with_exit(builder, suite, SuiteExitKind::Defensive)
}),
}
}
/// Return `true` if a given diagnostic candidate should be exempted.
///
/// Reasons for exemption could be that the condition serves a defensive-programming purpose,
/// or that the condition depends on an environment/compatibility check such as `sys.version_info`
/// or `sys.platform`.
pub(super) fn exempts(
self,
builder: &TypeInferenceBuilder<'_, '_>,
condition: &RedundantCondition<'_, '_>,
) -> bool {
let defensive = match self {
Self::Assertion => matches!(
&condition.kind,
ConditionKind::Boolean | ConditionKind::ShortCircuit
),
Self::DefensiveExit {
truthy_branch,
falsy_branch,
} => {
let is_boolean_or_short_circuit = matches!(
&condition.kind,
ConditionKind::Boolean | ConditionKind::ShortCircuit
);
is_boolean_or_short_circuit
&& if condition.is_truthy {
falsy_branch
} else {
truthy_branch
}
}
Self::Standalone => false,
};
if defensive {
return true;
}
match condition.expression {
// The inferred types for these will always be the same,
// even if a subexpression is defined in terms of `sys.version_info`, `sys.platform`, `os.name`, or
// `typing.TYPE_CHECKING`. We don't need to recurse into them *at the top level*, and it's more
// accurate not to do so.
//
// Note that this is only true at the *top level* of the condition!
//
// We *do* need to recurse into subexpressions of a generator expression, for example, if that
// generator is a subexpression of a call expression, e.g.
// `any(x for x in range(10) if sys.version_info >= (3, 14))`. Similar concerns apply to lambdas,
// which can be eagerly called, etc. etc.
ast::Expr::StringLiteral(_)
| ast::Expr::BytesLiteral(_)
| ast::Expr::NumberLiteral(_)
| ast::Expr::BooleanLiteral(_)
| ast::Expr::EllipsisLiteral(_)
| ast::Expr::Dict(_)
| ast::Expr::DictComp(_)
| ast::Expr::Generator(_)
| ast::Expr::Lambda(_)
| ast::Expr::Set(_)
| ast::Expr::SetComp(_)
| ast::Expr::List(_)
| ast::Expr::ListComp(_)
| ast::Expr::TString(_)
| ast::Expr::NoneLiteral(_) => false,
// These expressions can contain subexpressions that are defined in terms of `sys.version_info`,
// `sys.platform`, `os.name`, or `typing.TYPE_CHECKING`. We need to recurse into them to check for
// those subexpressions, since the type of the overall expression can depend on the types of those
// subexpressions.
ast::Expr::Name(_)
| ast::Expr::UnaryOp(_)
| ast::Expr::If(_)
| ast::Expr::FString(_)
| ast::Expr::Yield(_)
| ast::Expr::YieldFrom(_)
| ast::Expr::Attribute(_)
| ast::Expr::Named(_)
| ast::Expr::Call(_)
| ast::Expr::Subscript(_)
| ast::Expr::BinOp(_)
| ast::Expr::Await(_)
| ast::Expr::BoolOp(_)
| ast::Expr::Tuple(_)
| ast::Expr::Slice(_)
| ast::Expr::Starred(_)
| ast::Expr::IpyEscapeCommand(_)
| ast::Expr::Compare(_) => any_over_expr(condition.expression, |expression| {
is_special_cased_condition_expression(
builder.db(),
builder.program_file(),
expression,
|expr| builder.expression_type(expr),
)
}),
}
}
/// Reverse the defensive branches for the operand of `not`: a truthy operand makes the
/// containing condition false, and a falsy operand makes it true.
///
/// For example, the body of this `if` is a defensive exit:
///
/// ```python
/// def check(value: int):
/// if not isinstance(value, int):
/// raise TypeError("expected an integer")
/// ```
///
/// The `raise` is reached when `not isinstance(value, int)` is true, which means that
/// `isinstance(value, int)` is false. When checking that operand, we therefore move the
/// defensive-exit flag from `truthy_branch` to `falsy_branch`. This preserves the exemption:
/// although the annotation tells us that `isinstance(value, int)` is always true, the check
/// still protects against unexpected input at runtime.
pub(super) const fn negated(self) -> Self {
match self {
Self::DefensiveExit {
truthy_branch,
falsy_branch,
} => Self::DefensiveExit {
truthy_branch: falsy_branch,
falsy_branch: truthy_branch,
},
context => context,
}
}
/// Return the context for a boolean test inside another expression, such as a `not`
/// expression passed as a call argument.
///
/// An [`Self::Assertion`] context also exempts boolean and integer tests within call arguments.
/// For example, the assertion in `assert consume(not flag)` exempts the test of `flag` if it has
/// a boolean or integer type.
///
/// Tests within call arguments do not select the containing condition's branches, so they
/// do not inherit its [defensive-exit exemptions](Self::DefensiveExit).
pub(super) const fn nested_test(self) -> Self {
match self {
// Assertions also exempt boolean tests embedded in calls or other value expressions.
Self::Assertion => self,
Self::Standalone | Self::DefensiveExit { .. } => Self::Standalone,
}
}
}
/// Return `true` if any subexpression in `expression` is recognized as "tainted" by being defined
/// (directly or indirectly) with respect to `sys.version_info`, `sys.platform`, `os.name`, or
/// `typing.TYPE_CHECKING`.
///
/// Follow assignments and imports so aliases inherit the same exemption as the original guard.
fn is_special_cased_condition_expression<'db>(
db: &'db dyn Db,
file: ProgramFile<'db>,
expression: &ast::Expr,
mut expression_type: impl FnMut(&ast::Expr) -> Type<'db>,
) -> bool {
match expression {
ast::Expr::Name(ast::ExprName { id, .. }) if id == "TYPE_CHECKING" => return true,
ast::Expr::Attribute(ast::ExprAttribute { value, attr, .. }) => match &**attr {
"TYPE_CHECKING" => return true,
"name" => {
let value_type = expression_type(value);
if let Type::ModuleLiteral(module) = value_type
&& module.module(db).is_known(db, KnownModule::Os)
{
return true;
}
if value_type.is_never() {
return true;
}
}
"version_info" | "platform" => {
let value_type = expression_type(value);
if let Type::ModuleLiteral(module) = value_type
&& module.module(db).is_known(db, KnownModule::Sys)
{
return true;
}
if value_type.is_never() {
return true;
}
}
_ => {}
},
_ => {}
}
// We don't recurse through definitions in a flow-sensitive way, but there isn't really any need to.
// The main objective here is to avoid false positives. Flow-sensitive definitions of variables/attributes
// where some paths define the place in terms of `sys.version_info` but other paths don't are pretty rare.
// It's okay to have a small number of false negatives for these very rare edge cases. Attempting to
// recurse through definitions in a flow-sensitive way would be significantly more complicated.
condition_definition_info(db, file, expression, expression_type)
.contains_special_cased_condition
}
/// Resolves the condition's source definitions using a scope or an already-inferred receiver type.
fn condition_definition_info<'db>(
db: &'db dyn Db,
file: ProgramFile<'db>,
expression: &ast::Expr,
mut expression_type: impl FnMut(&ast::Expr) -> Type<'db>,
) -> ConditionDefinitionInfo {
match expression {
ast::Expr::Name(name) => {
let index = semantic_index(db, file);
let Some(scope) = index.try_expression_scope_id(&ast::ExprRef::Name(name)) else {
return ConditionDefinitionInfo::default();
};
name_condition_definition_info(db, scope.to_scope_id(db, file), name.id.clone())
}
ast::Expr::Attribute(attribute) => attribute_condition_definition_info(
db,
file.program(db),
expression_type(&attribute.value),
attribute.attr.id.clone(),
),
_ => ConditionDefinitionInfo::default(),
}
}
/// The information needed for condition exemptions.
#[derive(Clone, Copy, Debug, Default, Eq, PartialEq, get_size2::GetSize, salsa::SalsaValue)]
struct ConditionDefinitionInfo {
contains_special_cased_condition: bool,
}
impl ConditionDefinitionInfo {
/// Summarizes resolved definitions, following assignments to establish environment provenance.
fn from_definitions<'db>(db: &'db dyn Db, definitions: Vec<ResolvedDefinition<'db>>) -> Self {
// A place is a variable or attribute, and several definitions can bind the same place.
// The outer map identifies the place by its scope and place ID: place IDs are only unique
// within a scope. Each inner map associates a definition with the reachability constraint
// describing the paths from the start of that scope to its binding.
//
// For example:
//
// ```python
// import sys
//
// if sys.platform == "win32":
// prefix = "\n"
// else:
// prefix = ""
// ```
//
// There is one outer entry for `prefix` in the module scope. Its inner map has two
// entries, one for each assignment: the `"\n"` binding is guarded by the platform
// comparison, and the `""` binding by its negation. The constraint IDs refer to those
// formulas in the scope's use-def map; they do not store the assigned strings or the
// conditions' evaluated truthiness.
//
// Populate each inner map lazily and reuse it for the rest of this call: scanning all bindings
// for each definition would be quadratic for a variable assigned many times. Although building
// these indexes may look expensive, `name_condition_definition_info` and
// `attribute_condition_definition_info` cache this function's result with Salsa, so repeated
// conditions with the same lookup key reuse the summary without rebuilding the maps.
type ReachabilityByDefinition<'db> =
FxHashMap<Definition<'db>, ScopedReachabilityConstraintId>;
type ReachabilityByPlace<'db> =
FxHashMap<(ScopeId<'db>, ScopedPlaceId), ReachabilityByDefinition<'db>>;
let mut reachability_by_place = ReachabilityByPlace::default();
let contains_special_cased_condition = definitions
.into_iter()
.filter_map(|resolved| resolved.definition())
.any(|definition| {
let scope = definition.scope(db);
let place = definition.place(db);
let reachability_by_definition = reachability_by_place
.entry((scope, place))
.or_insert_with(|| {
use_def_map(db, scope)
.reachable_bindings(place)
.filter_map(|binding| {
Some((
binding.binding.definition()?,
binding.reachability_constraint,
))
})
.collect()
});
let reachability = reachability_by_definition
.get(&definition)
.copied()
.unwrap_or(ScopedReachabilityConstraintId::ALWAYS_TRUE);
definition_contains_special_cased_condition(db, definition, reachability)
});
Self {
contains_special_cased_condition,
}
}
}
/// Caches definition information across uses of the same name in a scope.
///
/// Name lookup considers every reachable binding, so repeating it for every condition can be
/// quadratic in the number of assignments. Caching only the per-definition traversal does not
/// avoid collecting and resolving those bindings again.
#[salsa::tracked(
returns(copy),
cycle_initial = |_, _, _, _| ConditionDefinitionInfo::default(),
heap_size = ruff_memory_usage::heap_size
)]
// Salsa copies this attribute to both the query wrapper and its inner function. The wrapper
// consumes `name`, so `#[expect]` would produce an unfulfilled lint expectation there.
#[allow(clippy::needless_pass_by_value, reason = "Salsa owns the query key")]
fn name_condition_definition_info<'db>(
db: &'db dyn Db,
scope: ScopeId<'db>,
name: Name,
) -> ConditionDefinitionInfo {
ConditionDefinitionInfo::from_definitions(
db,
definitions_for_name(db, scope, &name, ImportAliasResolution::ResolveAliases),
)
}
/// Caches definition information for a member of an already-inferred receiver type.
///
/// Attribute lookup can also repeatedly collect many bindings. Include the receiver type in the
/// key because narrowing or rebinding a receiver can change which definitions its members resolve
/// to. Taking that type from the caller avoids re-entering inference of the use-site scope.
#[salsa::tracked(
returns(copy),
cycle_initial = |_, _, _, _, _| ConditionDefinitionInfo::default(),
heap_size = ruff_memory_usage::heap_size
)]
// Salsa copies this attribute to both the query wrapper and its inner function. The wrapper
// consumes `name`, so `#[expect]` would produce an unfulfilled lint expectation there.
#[allow(clippy::needless_pass_by_value, reason = "Salsa owns the query key")]
fn attribute_condition_definition_info<'db>(
db: &'db dyn Db,
program: Program<'db>,
receiver: Type<'db>,
name: Name,
) -> ConditionDefinitionInfo {
ConditionDefinitionInfo::from_definitions(
db,
definitions_for_attribute(
db,
&ProgramEnvironment::from_program(program),
receiver,
&name,
),
)
}
/// Determines whether a definition originates from an environment-dependent guard.
///
/// Checks both the assigned value and the predicates controlling whether the binding is reached.
/// For example, `if sys.platform == "win32": prefix = "\n"` makes `prefix` environment-dependent
/// even though the assigned string does not mention `sys.platform`.
///
/// Follows aliases recursively and recognizes stub declarations for `sys.version_info`,
/// `sys.platform`, `os.name`, and `typing.TYPE_CHECKING`.
///
/// This Salsa-tracked query reads the definition's AST behind its own incremental boundary, so
/// callers do not depend directly on another file's syntax tree. Cyclic aliases recover as `false`.
/// `reachability` belongs to the use-def map of the definition's scope.
#[salsa::tracked(
returns(copy),
cycle_initial = |_, _, _, _| false,
heap_size = ruff_memory_usage::heap_size
)]
fn definition_contains_special_cased_condition<'db>(
db: &'db dyn Db,
definition: Definition<'db>,
reachability: ScopedReachabilityConstraintId,
) -> bool {
let module = parsed_module(db, definition.python_file(db)).load(db);
let definition_kind = definition.kind(db);
let file = definition.file(db);
let program_file = definition.program_file(db);
let in_known_module = |known| {
file_to_module(db, program_file.resolver_file(db))
.is_some_and(|module| module.is_known(db, known))
};
if let DefinitionKind::AnnotatedAssignment(annotated_assignment) = definition_kind
&& file.is_stub(db)
&& let ast::Expr::Name(ast::ExprName { id, .. }) = annotated_assignment.target(&module)
{
match &**id {
"version_info" | "platform" if in_known_module(KnownModule::Sys) => {
return true;
}
"name" if in_known_module(KnownModule::Os) => {
return true;
}
"TYPE_CHECKING" if in_known_module(KnownModule::Typing) => {
return true;
}
_ => {}
}
}
let source_expression = match definition_kind {
DefinitionKind::Assignment(assignment) => Some(assignment.value(&module)),
DefinitionKind::AnnotatedAssignment(assignment) => assignment.value(&module),
DefinitionKind::NamedExpression(named) => Some(&*named.node(&module).value),
DefinitionKind::AugmentedAssignment(assignment) => Some(&*assignment.node(&module).value),
DefinitionKind::For(for_statement) => Some(for_statement.iterable(&module)),
DefinitionKind::Comprehension(comprehension) => Some(comprehension.iterable(&module)),
DefinitionKind::WithItem(with_item) => Some(with_item.context_expr(&module)),
DefinitionKind::MatchPattern(pattern) => {
Some(pattern.predicate().subject(db).node_ref(db).node(&module))
}
DefinitionKind::Import(_)
| DefinitionKind::ImportFrom(_)
| DefinitionKind::ImportFromSubmodule(_)
| DefinitionKind::StarImport(_)
| DefinitionKind::Function(_)
| DefinitionKind::Class(_)
| DefinitionKind::TypeAlias(_)
| DefinitionKind::DictKeyAssignment(_)
| DefinitionKind::Parameter(_)
| DefinitionKind::LambdaParameter(_)
| DefinitionKind::ExceptHandler(_)
| DefinitionKind::TypeVar(_)
| DefinitionKind::ParamSpec(_)
| DefinitionKind::TypeVarTuple(_)
| DefinitionKind::LoopHeader(_)
| DefinitionKind::NestedBindings(_) => None,
};
let Some(source_expression) = source_expression else {
return false;
};
// A version guard can select a different function signature without changing the fact that
// the function object is always truthy. Only definitions with a source expression above
// inherit the provenance of their guards.
if reachability_contains_special_cased_condition(db, definition.scope(db), reachability) {
return true;
}
// Binding inference does not always retain the source expression's types: unpacked targets
// share a source, and a comprehension's first iterable belongs to the enclosing scope.
// Read those types from the standalone expression query, without re-entering scope inference.
let standalone = semantic_index(db, program_file).try_expression(source_expression);
let mut expression_inference = None;
let mut definition_inference = None;
any_over_expr(source_expression, |expression| {
is_special_cased_condition_expression(db, program_file, expression, |expr| {
if let Some(standalone) = standalone {
expression_inference
.get_or_insert_with(|| {
infer_expression_types(db, standalone, TypeContext::default())
})
.expression_type(expr)
} else {
definition_inference
.get_or_insert_with(|| infer_definition_types(db, definition))
.expression_type(expr)
}
})
})
}
/// Summarize environment provenance for a reachability node and all its descendants.
///
/// Successive statements often share most of their reachability graph. Cache each node's
/// summary so checking another definition does not traverse the shared portion again.
/// All three branches contribute, regardless of the configured environment's truthiness.
#[salsa::tracked(
returns(copy),
cycle_initial = |_, _, _, _| false,
heap_size = ruff_memory_usage::heap_size
)]
fn reachability_contains_special_cased_condition<'db>(
db: &'db dyn Db,
scope: ScopeId<'db>,
reachability: ScopedReachabilityConstraintId,
) -> bool {
if matches!(
reachability,
ScopedReachabilityConstraintId::ALWAYS_TRUE
| ScopedReachabilityConstraintId::ALWAYS_FALSE
| ScopedReachabilityConstraintId::AMBIGUOUS
) {
return false;
}
let node = use_def_map(db, scope)
.reachability_constraints()
.get_interior_node(reachability);
predicate_contains_special_cased_condition(db, scope, node.atom())
|| [node.if_true(), node.if_ambiguous(), node.if_false()]
.into_iter()
.any(|child| reachability_contains_special_cased_condition(db, scope, child))
}
/// Check whether a control-flow predicate depends on an environment guard, regardless of whether
/// the binding is reached when the predicate is true or when it is false.
///
/// Predicate expressions have their own inference queries, so checking a guard does not require
/// completing inference of the scope containing the definition. Cache the result because many
/// definitions can share the same guard, and recover as `false` if the guard refers to the definition.
#[salsa::tracked(
returns(copy),
cycle_initial = |_, _, _, _| false,
heap_size = ruff_memory_usage::heap_size
)]
fn predicate_contains_special_cased_condition<'db>(
db: &'db dyn Db,
scope: ScopeId<'db>,
predicate: ScopedPredicateId,
) -> bool {
let expression = match use_def_map(db, scope).predicates()[predicate].node {
PredicateNode::Expression(expression)
| PredicateNode::Condition(expression)
| PredicateNode::ChainedComparisonCondition(expression)
| PredicateNode::IsNonEmptyIterable(expression) => expression,
PredicateNode::Pattern(pattern) => pattern.subject(db),
PredicateNode::SubjectElementPattern(element) => element.pattern.subject(db),
// These predicates describe implicit control flow rather than a tested value. In
// particular, a statement such as `print(sys.platform)` should not make every later
// definition environment-dependent merely because it is reached after that call returns.
PredicateNode::IsNonTerminalCall(_)
| PredicateNode::ContextManagerSuppresses { .. }
| PredicateNode::FinallyNormalPathImpossible { .. }
| PredicateNode::OrPatternAlternative(_)
| PredicateNode::StarImportPlaceholder(_) => return false,
};
let file = expression.program_file(db);
let module = parsed_module(db, expression.python_file(db)).load(db);
let mut inference = None;
any_over_expr(expression.node_ref(db).node(&module), |subexpression| {
is_special_cased_condition_expression(db, file, subexpression, |expr| {
inference
.get_or_insert_with(|| {
infer_expression_types(db, expression, TypeContext::default())
})
.expression_type(expr)
})
})
}
crates/ty_python_semantic/src/types/infer/builder/redundant_conditions/mod.rs
+967
View File
@@ -0,0 +1,967 @@
//! Logic for reporting boolean tests that are unintentionally always truthy or always falsy.
//! These may be reported under either `redundant-condition` or `redundant-condition-strict`.
//!
//! This module classifies tests and selects which expressions to report. [`exemptions`] handles
//! assertions, defensive branches, and environment checks; [`diagnostic`] builds messages and fixes.
//!
//! ## How we check compound conditions
//!
//! Python tests the truthiness of the whole expression after `if`, `while`, or `assert`. It also
//! tests the truthiness of subexpressions used within `and`, `or`, and `not` expressions.
//!
//! For example, `ready or flag` below is a compound condition: a condition built using a boolean
//! operator. The operands of the `or` expression are `ready` and `flag`. The redundant-condition
//! checker reports that the function object `ready` is always truthy, since the author probably
//! intended to call it:
//!
//! ```python
//! def ready() -> bool:
//! return True
//!
//! def check(flag: bool):
//! if ready or flag: # `redundant-condition` on `ready`.
//! print("ready")
//! ```
//!
//! The whole condition is also always truthy, but reporting the fixed truthiness of the outer
//! condition as well the fixed truthiness of its subexpressions would flag the same mistake
//! twice. To choose which expression to report, the redundant-condition checker therefore needs
//! to see the context of the complete condition. We therefore wait until the
//! [`TypeInferenceBuilder`] has inferred the whole condition before checking any subexpression
//! boolean tests within it. See [`TypeInferenceBuilder::check_condition_redundancy`] for how a
//! check of a smaller expression is postponed until that point.
//!
//! A boolean test can also occur inside an expression that is being passed to a function or
//! stored in a variable. For example, `not ready` below tests the truthiness of `ready` while
//! producing the argument to `consume`. The redundant-condition checker reports that test too,
//! even though there is no `if`, `while`, or `assert` statement.
//!
//! ```python
//! def ready() -> bool:
//! return True
//!
//! def consume(flag: bool):
//! print(flag)
//!
//! consume(not ready) # `redundant-condition` on `ready`.
//! ```
mod diagnostic;
mod exemptions;
use ruff_python_ast::{
self as ast,
helpers::any_over_expr,
visitor::{Visitor, walk_expr},
};
use ty_python_core::{Truthiness, expression::ExpressionContext, predicate::StatementCall};
use crate::{
lint::LintMetadata,
reachability::{analyze_condition_expression, is_non_terminal_call},
types::{
KnownClass, KnownInstanceType, Type,
diagnostic::{REDUNDANT_CONDITION, REDUNDANT_CONDITION_STRICT},
infer::TypeInferenceBuilder,
},
};
use self::exemptions::RedundantConditionContext;
/// Classification of a redundant condition.
///
/// This is used to determine which diagnostic rule would apply to the condition.
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
enum ConditionKind {
/// A condition whose value type is assignable to `int`, including `bool`.
///
/// These tests commonly enforce runtime invariants, so they are only flagged by the
/// opt-in `redundant-condition-strict` rule and are exempted entirely in `assert` tests.
///
/// ```python
/// def check(value: str):
/// if isinstance(value, str): # Always true; flagged only by `redundant-condition-strict`.
/// print(value)
/// assert isinstance(value, str) # Deliberate defensive check; exempted from both rules
/// # due to being in an `assert` test.
/// ```
Boolean,
/// A condition whose outcome is fixed by short-circuit evaluation, despite the fact that its
/// inferred value type does not have fixed truthiness. These conditions are only flagged by
/// the opt-in `redundant-condition-strict` rule.
///
/// ```python
/// def check(value: object):
/// if value and False: # Always false; strict rule.
/// print("unreachable")
/// ```
ShortCircuit,
/// An always-truthy or always-falsy value test containing a walrus expression.
///
/// In general, it is hard to know for sure whether an expression could have a side effect.
/// Walrus expressions are an exception to this, however: they *always* have a side effect,
/// and the user is probably only using a walrus expression specifically to get that side effect.
/// Therefore, we only report a test under the opt-in `redundant-condition-strict` rule if any
/// subexpression (even if that subexpression is in a nested scope, such as the body of a lambda
/// expression or generator expression) is a walrus expression.
///
/// ```python
/// if value := (1, 2): # Always truthy; only reported by `redundant-condition-strict`
/// print(value)
/// ```
ContainsWalrus,
/// A value with fixed truthiness that does not require the strict rule.
///
/// Unlike the other three categories, these tests are reported by the enabled-by-default
/// `redundant-condition` rule.
///
/// ```python
/// def ready() -> bool:
/// return True
///
/// if ready: # The function object is always truthy; reported by `redundant-condition`.
/// print("ready")
/// ```
Value,
}
impl ConditionKind {
/// Return the rule responsible for reporting this category of redundant condition.
const fn rule(self) -> &'static LintMetadata {
match self {
Self::Value => &REDUNDANT_CONDITION,
Self::Boolean | Self::ShortCircuit | Self::ContainsWalrus => {
&REDUNDANT_CONDITION_STRICT
}
}
}
}
/// An already-inferred boolean test.
///
/// The `evaluation` field records whether the expression is tested directly as a condition or
/// first evaluated as a value, as described by [`ExpressionContext`].
///
/// The truthiness of an `and`, `or`, or `not` expression is determined by its operands' truthiness,
/// so the checker visits those operands with the same evaluation context. For example, both
/// `enabled` and `value is not None` below are checked with [`ExpressionContext::Condition`],
/// like the complete `if` test:
///
/// ```python
/// def check(value: int, enabled: bool):
/// if enabled and value is not None:
/// print(value)
/// ```
///
/// The two branches of a conditional expression also inherit its evaluation context: whichever
/// branch is selected supplies the value whose truthiness is tested.
///
/// A truthiness test inside a call argument is checked separately from the truthiness of the
/// call's return value. For example:
///
/// ```python
/// def wrap(flag: bool) -> tuple[bool]:
/// return (flag,)
///
/// def check(value: int):
/// if wrap(not (value is None)):
/// print(value)
/// ```
///
/// Here, `not` tests `value is None` to produce the argument to `wrap`, so that operand is checked
/// with [`ExpressionContext::Value`]. The `if` tests the tuple returned by `wrap`, using
/// [`ExpressionContext::Condition`]. We call these "independent tests": reporting the
/// always-false comparison does not replace the diagnostic on the always-truthy tuple.
#[derive(Clone, Copy)]
struct BooleanTest<'ast, 'db> {
expression: &'ast ast::Expr,
value_type: Type<'db>,
truthiness: Truthiness,
evaluation: ExpressionContext,
}
/// A condition with known truthiness, and the rule category needed to report it.
///
/// We may or may not eventually report a diagnostic for this condition! A condition is classified
/// before we examine the context the condition occurs in, so an instance of this struct represents
/// a "diagnostic candidate" rather than a guarantee that a diagnostic will be emitted.
///
/// The value type is retained for diagnostic messages. The `is_truthy` field describes the condition's
/// outcome, which can depend on short-circuit evaluation as well as the value type (see the doc-comment
/// for [`ExpressionContext`] for more details).
#[derive(Debug)]
struct RedundantCondition<'ast, 'db> {
expression: &'ast ast::Expr,
value_type: Type<'db>,
is_truthy: bool,
kind: ConditionKind,
}
/// Determination of whether `redundant-condition-strict` diagnostics should be reported
/// on subexpressions of a compound condition, or should instead be only reported on the
/// outer expression.
///
/// `redundant-condition` diagnostics are always reported, regardless of this preference.
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
enum BooleanDiagnosticPreference {
/// No enclosing condition has fixed truthiness, so this test can be reported individually.
CurrentCondition,
/// A condition containing this test has fixed truthiness. `redundant-condition-strict`
/// diagnostics should be suppressed for this subexpression.
EnclosingCondition,
}
/// The result of checking a boolean test, indicating whether an expression containing that test
/// should also be checked for redundancy.
///
/// This is returned by [`TypeInferenceBuilder::check_boolean_test`] to prevent duplicate diagnostics
/// on an expression and its subexpressions. A condition can suppress an enclosing diagnostic even
/// when its own diagnostic is disabled, exempt, or ignored.
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
enum ConditionCheckResult {
/// The containing boolean or conditional expression can still be reported.
///
/// For example, the boolean result of `isinstance()` is not reported as a subexpression when
/// the complete `not` expression has fixed truthiness:
///
/// ```python
/// def check(value: str):
/// if not isinstance(value, str): # `redundant-condition-strict` on the whole condition.
/// print("unreachable")
/// ```
CheckEnclosingCondition,
/// The containing boolean or conditional expression should not be reported.
///
/// For example, reporting that `ready` is always truthy suppresses a second diagnostic saying
/// that `not ready` is always false. This also applies if the diagnostic on `ready` is ignored
/// or its rule is disabled:
///
/// ```python
/// def ready() -> bool:
/// return True
///
/// if not ready: # Only `ready` is reported, under `redundant-condition`.
/// print("unreachable")
/// ```
SuppressEnclosingCondition,
}
impl<'db> TypeInferenceBuilder<'db, '_> {
/// Whether we should check for redundant conditions in the current inference context.
///
/// We skip the checks if:
///
/// - It's a third-party file
/// - It's a stub file
/// - We're inside a string annotation. Its expressions are absent from the semantic index,
/// so we cannot group nested tests to avoid duplicate diagnostics.
/// - Neither `redundant-condition` nor `redundant-condition-strict` is enabled
/// in the user's configuration.
fn should_check_redundant_conditions(&self) -> bool {
!self.in_string_annotation()
&& self.db().should_check_file(self.file())
&& !self.file().is_stub(self.db())
&& (self.context.is_lint_enabled(&REDUNDANT_CONDITION)
|| self.context.is_lint_enabled(&REDUNDANT_CONDITION_STRICT))
}
/// Check a condition, for which types have already been inferred, to see if it is redundant.
/// Report a diagnostic if so.
///
/// If `test` is part of a larger expression that Python also tests for truthiness, we
/// short-circuit without reporting anything. The call that checks the largest such
/// expression in this scope will check the smaller tests within it. We call that largest
/// expression the "outermost boolean test".
///
/// For example, `not ready or flag` below is the outermost boolean test: it is the whole
/// expression tested by `if`. The test of `ready` performed by `not` is a nested test because
/// it occurs inside that expression. The whole expression is a compound condition because
/// it combines tests using `not` and `or`:
///
/// ```python
/// def ready() -> bool:
/// return True
///
/// def check(flag: bool):
/// if not ready or flag:
/// print("ready")
/// ```
///
/// Checking the complete condition lets us report that `ready` is an always-truthy function
/// object, without also reporting a duplicate diagnostic regarding the fact that `not ready` is
/// always false.
///
/// Tests in a different scope, such as a lambda body, are checked when that scope is inferred.
pub(super) fn check_condition_redundancy(&self, test: &ast::Expr, test_type: Type<'db>) {
if !self.should_check_redundant_conditions() {
return;
}
if !self.index.is_boolean_test_root(test) {
return;
}
let truthiness = self.condition_truthiness(test);
for condition in self.redundant_conditions(
BooleanTest {
expression: test,
value_type: test_type,
truthiness,
evaluation: ExpressionContext::Condition,
},
RedundantConditionContext::Standalone,
) {
self.report_redundant_condition(&condition);
}
}
/// Evaluates an already-inferred expression as a direct condition.
///
/// Unlike testing a saved expression's value, this does not re-test intermediate
/// short-circuit results, whose truthiness may have changed.
fn condition_truthiness(&self, test: &ast::Expr) -> Truthiness {
let db = self.db();
let env = self.program_environment();
analyze_condition_expression(test, &|node| {
self.comparison_truthiness
.get(&node.into())
.copied()
.or_else(|| self.expression_type(node).bool_if_inhabited(db, env))
})
.unwrap_or(Truthiness::Ambiguous)
}
/// Check whether a `not` expression used as a value contains a redundant boolean test.
/// Report a diagnostic if so.
///
/// For example, `result = not func` will be reported if `func` is an always-truthy function object.
///
/// If the `not` expression is nested inside another boolean test, we leave the diagnostic to
/// the call that checks the complete condition. For example, the `if` condition below reports
/// the test of `ready`, so this method does not report it separately while inferring `not ready`:
///
/// ```python
/// def ready() -> bool:
/// return True
///
/// if not ready: # One `redundant-condition` diagnostic on `ready`.
/// print("unreachable")
/// ```
pub(super) fn check_negation_redundancy(
&self,
unary: &ast::ExprUnaryOp,
operand_type: Type<'db>,
operand_truthiness: Truthiness,
) {
if !self.should_check_redundant_conditions() {
return;
}
// Avoid duplicate diagnostics if we're visiting a nested boolean test.
if !self.index.is_boolean_test_root(&unary.operand) {
return;
}
for condition in self.redundant_conditions(
BooleanTest {
expression: &unary.operand,
value_type: operand_type,
truthiness: operand_truthiness,
evaluation: ExpressionContext::Value,
},
RedundantConditionContext::Standalone,
) {
self.report_redundant_condition(&condition);
}
}
/// Sweep over an entire suite of statements to examine if any direct `if`-statement conditions,
/// `elif`-statement conditions or `assert`-statement conditionsin that suite are redundant.
///
/// We suppress conditions in [`ConditionKind::Boolean`] and [`ConditionKind::ShortCircuit`] when
/// the code they make unreachable is a "defensive exit". See the doc-comment for
/// [`RedundantConditionContext::DefensiveExit`] for more details.
///
/// All types in the suite must already be inferred before this method is called. This is so we
/// can recognize terminal statements from their types, including calls returning `Never` and
/// `return NotImplemented` statements.
///
/// ## Assertions
///
/// Assertions commonly check runtime invariants, so tests classified as
/// [`ConditionKind::Boolean`] or [`ConditionKind::ShortCircuit`] are exempt. This applies to
/// both complete assertion tests and their subexpressions:
///
/// ```python
/// from typing import Literal
///
/// def check(value: str, count: Literal[1]):
/// assert isinstance(value, str) # Always true, but exempt from both rules: a boolean assertion.
/// assert count # Always truthy, but exempt from both rules: an integer assertion.
/// ```
///
/// Other assertions can still indicate a mistake. For example, asserting a function object
/// rather than calling it does not check the function's return value:
///
/// ```python
/// def ready() -> bool:
/// return True
///
/// assert ready # `redundant-condition`: the function object is always truthy.
/// ```
///
/// Always-falsy values are also eligible for the ordinary rule: `assert value` is reported
/// when `value: None`, although a literal `assert None` is exempt. Tests classified as
/// [`ConditionKind::ContainsWalrus`], such as `assert (value := "foo")`, remain eligible for
/// `redundant-condition-strict`.
pub(super) fn check_suite_for_redundant_conditions(&self, suite: &[ast::Stmt]) {
if !self.should_check_redundant_conditions() {
return;
}
for (i, statement) in suite.iter().enumerate() {
match statement {
ast::Stmt::If(if_stmt) => {
let ast::StmtIf {
test,
body,
elif_else_clauses,
..
} = if_stmt;
let branches = std::iter::once((test.as_ref(), body.as_slice())).chain(
elif_else_clauses.iter().map_while(|clause| {
Some((clause.test.as_ref()?, clause.body.as_slice()))
}),
);
for (branch_index, (test, body)) in branches.enumerate() {
let following_clauses = &elif_else_clauses[branch_index..];
let context = RedundantConditionContext::for_if_statement(
self,
body,
following_clauses,
&suite[i + 1..],
);
for condition in self.redundant_conditions(
self.boolean_test(test, ExpressionContext::Condition),
context,
) {
self.report_redundant_condition(&condition);
}
}
}
ast::Stmt::Assert(assert_statement) => {
let test_type = self.expression_type(&assert_statement.test);
let truthiness = self.condition_truthiness(&assert_statement.test);
for condition in self.redundant_conditions(
BooleanTest {
expression: &assert_statement.test,
value_type: test_type,
truthiness,
evaluation: ExpressionContext::Condition,
},
RedundantConditionContext::Assertion,
) {
self.report_redundant_condition(&condition);
}
}
ast::Stmt::While(while_statement) => {
let test_type = self.expression_type(&while_statement.test);
let truthiness = self.condition_truthiness(&while_statement.test);
for condition in self.redundant_conditions(
BooleanTest {
expression: &while_statement.test,
value_type: test_type,
truthiness,
evaluation: ExpressionContext::Condition,
},
RedundantConditionContext::Standalone,
) {
self.report_redundant_condition(&condition);
}
}
_ => continue,
}
}
}
/// Classify a condition using its inferred type and truthiness.
///
/// We return `None` if we cannot determine whether the test is always truthy or always falsy.
/// We also return `None` for literal boolean and integer expressions: a condition written as
/// `True`, `False`, `1`, or `0` is almost certainly deliberate. A variable with an inferred
/// literal type is still classified, however, because its truthiness may be less obvious to
/// the author:
///
/// ```python
/// from typing import Literal
///
/// def check(flag: bool, known: Literal[True]):
/// if flag: # No classification: the outcome is unknown.
/// pass
/// if True: # No classification: a literal boolean expression.
/// pass
/// if 1: # No classification: a literal integer expression.
/// pass
/// if known: # `ConditionKind::Boolean`: the value type is boolean.
/// pass
/// ```
fn classify_redundant_condition<'expr>(
&self,
test: BooleanTest<'expr, 'db>,
) -> Option<RedundantCondition<'expr, 'db>> {
let BooleanTest {
expression,
value_type,
truthiness,
..
} = test;
if truthiness.is_ambiguous() {
return None;
}
if matches!(
expression,
ast::Expr::BooleanLiteral(_)
| ast::Expr::NoneLiteral(_)
| ast::Expr::NumberLiteral(ast::ExprNumberLiteral {
value: ast::Number::Int(_),
..
})
) {
// Literal flags such as `if False` and `while 1` are almost certainly deliberate.
// `assert None` also comes up a weird amount in certain ecosystem projects, too!
return None;
}
let db = self.db();
let env = self.program_environment();
let kind = if value_type.is_assignable_to(db, env, KnownClass::Int.to_instance(db, env)) {
ConditionKind::Boolean
} else if value_type.bool(db, env).is_ambiguous() {
ConditionKind::ShortCircuit
} else if any_over_expr(expression, ast::Expr::is_named_expr) {
// Include deferred bodies: a surrounding call may execute a lambda or generator.
ConditionKind::ContainsWalrus
} else {
ConditionKind::Value
};
Some(RedundantCondition {
expression,
value_type,
is_truthy: truthiness.is_always_true(),
kind,
})
}
/// Read an already-inferred expression's type and truthiness using the requested
/// [`ExpressionContext`]. Return a [`BooleanTest`] instance.
///
/// Use short-circuit evaluation when the expression is tested directly as a condition.
/// Otherwise, determine truthiness from its inferred value type.
///
/// See the [`ExpressionContext`] doc-comment for more details.
fn boolean_test<'expr>(
&self,
expression: &'expr ast::Expr,
evaluation: ExpressionContext,
) -> BooleanTest<'expr, 'db> {
let value_type = self.expression_type(expression);
let truthiness = match evaluation {
ExpressionContext::Condition => self.condition_truthiness(expression),
ExpressionContext::Value => value_type.bool(self.db(), self.program_environment()),
};
BooleanTest {
expression,
value_type,
truthiness,
evaluation,
}
}
/// Check `test` and its operands for redundancy, returning the conditions to report.
///
/// The operands of `and`, `or`, and `not`, and the branches of conditional expressions,
/// are examined before deciding whether to report the expression containing them.
/// This ensures that the same mistake is not reported twice.
///
/// Callers construct diagnostics for the selected conditions using the [`diagnostic`] module,
/// and can add annotations using the surrounding statement context.
///
/// Independent tests within subexpressions are included in the same result.
///
/// If the truthiness of the complete condition is fixed, we only report a single diagnostic
/// for the entire condition. If its truthiness is ambiguous, however, subexpressions with
/// fixed truthiness can be reported individually:
///
/// ```python
/// def check(value: int, flag: bool):
/// if flag and value is not None: # Report `value is not None`.
/// print(value)
/// if isinstance(value, int) and value is not None: # Report the complete condition.
/// print(value)
/// ```
#[must_use]
fn redundant_conditions<'ast>(
&self,
test: BooleanTest<'ast, 'db>,
condition_context: RedundantConditionContext,
) -> Vec<RedundantCondition<'ast, 'db>> {
let mut conditions = Vec::new();
self.check_boolean_test(
test,
condition_context,
BooleanDiagnosticPreference::CurrentCondition,
&mut conditions,
);
conditions
}
/// Check `test` and the boolean tests contributing to its outcome.
///
/// Diagnostic selection follows two rules:
///
/// - Within a condition with fixed truthiness, boolean and short-circuit operands are not
/// reported. The enclosing condition represents their redundancy, even if its diagnostic
/// is exempt or ignored.
/// - Other operands, such as uncalled functions, take precedence over their enclosing
/// conditions because they identify the likely mistake more directly.
///
/// The first rule is decided before visiting operands, since the whole condition's truthiness
/// is already known. The second is decided after visiting them, using [`ConditionCheckResult`].
///
/// Return [`ConditionCheckResult::SuppressEnclosingCondition`] if a caller checking a boolean
/// or conditional expression containing `test` should suppress the diagnostic on that larger
/// expression. For example, the code below receives a diagnostic on `ready` because the
/// function object is always truthy. It does not also receive a diagnostic saying that
/// `not ready` is always false:
///
/// ```python
/// def ready() -> bool:
/// return True
///
/// if not ready: # `redundant-condition` on `ready`.
/// print("unreachable")
/// ```
///
/// Checking `ready` returns [`ConditionCheckResult::SuppressEnclosingCondition`], so the call
/// checking `not ready` knows not to report a diagnostic regarding the larger expression.
///
/// Return the same result if the selected diagnostic is disabled or ignored. Disabling the
/// diagnostic on `ready` should not cause a different diagnostic on `not ready` to appear.
/// Conditions selected for reporting are appended to `conditions` in traversal order.
fn check_boolean_test<'ast>(
&self,
test: BooleanTest<'ast, 'db>,
condition_context: RedundantConditionContext,
preference: BooleanDiagnosticPreference,
conditions: &mut Vec<RedundantCondition<'ast, 'db>>,
) -> ConditionCheckResult {
let operand_preference = if test.truthiness.is_ambiguous() {
preference
} else {
BooleanDiagnosticPreference::EnclosingCondition
};
let mut operand_result = ConditionCheckResult::CheckEnclosingCondition;
let mut check_operand = |expression: &'ast ast::Expr, context, conditions: &mut Vec<_>| {
if self.check_boolean_test(
self.boolean_test(expression, test.evaluation),
context,
operand_preference,
conditions,
) == ConditionCheckResult::SuppressEnclosingCondition
{
operand_result = ConditionCheckResult::SuppressEnclosingCondition;
}
};
match test.expression {
ast::Expr::BoolOp(ast::ExprBoolOp { values, .. }) => {
// Include the final operand: `if flag and func` tests `func` when `flag` is true.
for value in values {
check_operand(value, condition_context, conditions);
}
}
ast::Expr::UnaryOp(ast::ExprUnaryOp {
op: ast::UnaryOp::Not,
operand,
..
}) => {
check_operand(operand, condition_context.negated(), conditions);
}
ast::Expr::If(ast::ExprIf {
test, body, orelse, ..
}) => {
// This test chooses a branch independently of the enclosing truthiness check.
// The selected branch supplies the value tested by the enclosing condition.
self.check_boolean_test(
self.boolean_test(test, ExpressionContext::Condition),
condition_context.nested_test(),
BooleanDiagnosticPreference::CurrentCondition,
conditions,
);
for branch in [body, orelse] {
check_operand(branch, condition_context, conditions);
}
}
_ => self.check_nested_conditions(
test.expression,
condition_context.nested_test(),
conditions,
),
}
if operand_result == ConditionCheckResult::SuppressEnclosingCondition {
return operand_result;
}
if matches!(
test.value_type,
Type::KnownInstance(KnownInstanceType::ConstraintSet(_))
) {
// Boolean tests against `ty_extensions._internal.ConstraintSet` are (hopefully)
// only going to occur in the context of our test suite, and it's very annoying
// if we report them as redundant.
return ConditionCheckResult::SuppressEnclosingCondition;
}
let Some(condition) = self.classify_redundant_condition(test) else {
return ConditionCheckResult::CheckEnclosingCondition;
};
if preference == BooleanDiagnosticPreference::EnclosingCondition
&& matches!(
&condition.kind,
ConditionKind::Boolean | ConditionKind::ShortCircuit
)
{
return ConditionCheckResult::CheckEnclosingCondition;
}
let rule = condition.kind.rule();
if self.context.is_lint_enabled(rule) && !condition_context.exempts(self, &condition) {
conditions.push(condition);
}
ConditionCheckResult::SuppressEnclosingCondition
}
/// Find and check boolean tests within subexpressions of `test`. This includes tests inside
/// call arguments and collection elements.
///
/// A `not` expression or conditional expression can test truthiness while producing a value
/// that is passed to a function. That test is checked separately from any test of the call's
/// return value.
///
/// For example, the following code has two mistakes: `not ready` tests an always-truthy
/// function object, and the `if` tests a call that always returns a nonempty tuple.
/// Two diagnostics are therefore reported on the code: one for `ready`, and one for
/// `consume(not ready)`:
///
/// ```python
/// def ready() -> bool:
/// return True
///
/// def consume(flag: bool) -> tuple[int]:
/// return (1,)
///
/// if consume(not ready):
/// print("ready")
/// ```
///
/// We use the [`SubexpressionChecker`] visitor to find subexpression tests in the current
/// scope. Reporting them does not suppress a diagnostic on `test` itself, since that
/// would hide the second mistake in the above example.
fn check_nested_conditions<'ast>(
&self,
test: &'ast ast::Expr,
condition_context: RedundantConditionContext,
conditions: &mut Vec<RedundantCondition<'ast, 'db>>,
) {
let mut visitor = SubexpressionChecker {
builder: self,
condition_context,
conditions,
};
visitor.visit_expr(test);
}
}
/// An AST visitor for boolean tests that exist as subexpressions of an outer expression being
/// checked by [`TypeInferenceBuilder::check_boolean_test`].
///
/// The visitor passes each `not` operand and each conditional expression's `if` test to the
/// redundant-condition checker. These tests can occur in call arguments or collection elements, as
/// illustrated by [`TypeInferenceBuilder::check_nested_conditions`]. They are checked separately
/// from the truthiness of the containing call or collection.
///
/// The visitor stays within the current inference scope. Lambda bodies and comprehension scopes
/// are checked in a separate pass of the [`TypeInferenceBuilder`].
struct SubexpressionChecker<'a, 'ast, 'db> {
builder: &'a TypeInferenceBuilder<'db, 'a>,
condition_context: RedundantConditionContext,
conditions: &'a mut Vec<RedundantCondition<'ast, 'db>>,
}
impl<'ast> Visitor<'ast> for SubexpressionChecker<'_, 'ast, '_> {
fn visit_expr(&mut self, expression: &'ast ast::Expr) {
let builder = self.builder;
// Lambda bodies and comprehension scopes have their own inference and test owners.
if builder.index.try_expression_scope_id(expression)
!= Some(builder.scope().file_scope_id(builder.db()))
{
return;
}
if builder.try_expression_type(expression).is_none() {
return;
}
match expression {
ast::Expr::UnaryOp(ast::ExprUnaryOp {
op: ast::UnaryOp::Not,
operand,
..
}) => {
builder.check_boolean_test(
builder.boolean_test(operand, ExpressionContext::Value),
self.condition_context,
BooleanDiagnosticPreference::CurrentCondition,
self.conditions,
);
}
ast::Expr::If(ast::ExprIf {
test, body, orelse, ..
}) => {
builder.check_boolean_test(
builder.boolean_test(test, ExpressionContext::Condition),
self.condition_context,
BooleanDiagnosticPreference::CurrentCondition,
self.conditions,
);
self.visit_expr(body);
self.visit_expr(orelse);
}
_ => walk_expr(self, expression),
}
}
}
/// Which exits are accepted when checking the final statement of a suite.
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
enum SuiteExitKind {
/// Accept any return statement, as well as defensive exits.
Any,
/// Only accept exits that can indicate a defensive runtime check.
Defensive,
}
/// Return `true` if `suite` ends in an exit recognized by the redundant-condition heuristic.
///
/// Both kinds of exit accept the following final statements:
/// - a `raise` statement
/// - a potentially failing assertion
/// - a call returning `Never`, including an awaited call
/// - or a nested `if` statement with an explicit `else` where every branch of the
/// `if`/`elif`/`else` ends in a recognized exit of the requested kind.
///
/// [`SuiteExitKind::Any`] also accepts every `return` statement, while
/// [`SuiteExitKind::Defensive`] only accepts `return NotImplemented`.
///
/// Potentially failing assertions count as exits even when they might succeed. This heuristic
/// prioritises avoiding false positives on intentional runtime checks.
fn suite_ends_with_exit(
builder: &TypeInferenceBuilder<'_, '_>,
suite: &[ast::Stmt],
kind: SuiteExitKind,
) -> bool {
let db = builder.db();
let env = builder.program_environment();
suite
.iter()
.rev()
.find(|stmt| !is_trivial_statement(stmt))
.is_some_and(|stmt| match stmt {
ast::Stmt::Raise(_) => true,
ast::Stmt::Assert(ast::StmtAssert { test, .. }) => {
builder.condition_truthiness(test).may_be_false()
}
ast::Stmt::Expr(ast::StmtExpr { value, .. })
if let Some(StatementCall { call, is_await }) =
StatementCall::from_expression(value) =>
{
let callable_type = builder.expression_type(&call.func);
// In a statically unreachable branch, even an ordinary callable can have type
// `Never`. Preserve the exemption in that case to avoid false positives on
// defensive checks made unreachable by type annotations.
callable_type.is_never()
|| is_non_terminal_call(db, env, callable_type, is_await, || {
builder.expression_type(value)
})
.is_always_false()
}
ast::Stmt::Return(ast::StmtReturn { value, .. }) => match kind {
SuiteExitKind::Any => true,
SuiteExitKind::Defensive => value.as_ref().is_some_and(|expr| {
// Known limitation: `Any`, `Unknown`, and `Never` are also assignable to
// `NotImplementedType`, so an ordinary return *can* suppress a diagnostic here.
// We prioritise minimising false positives over minimising false negatives
// when recognizing potentially deliberate defensive checks.
builder.expression_type(expr).is_assignable_to(
db,
env,
KnownClass::NotImplementedType.to_instance(db, env),
)
}),
},
ast::Stmt::If(ast::StmtIf {
body,
elif_else_clauses,
..
}) => {
elif_else_clauses
.last()
.is_some_and(|last_clause| last_clause.test.is_none())
&& suite_ends_with_exit(builder, body, kind)
&& elif_else_clauses
.iter()
.all(|clause| suite_ends_with_exit(builder, &clause.body, kind))
}
_ => false,
})
}
/// Return `true` if `stmt` is a "trivial statement"
/// that has no effect nor side effect at runtime.
fn is_trivial_statement(stmt: &ast::Stmt) -> bool {
match stmt {
ast::Stmt::Pass(_) => true,
ast::Stmt::Expr(ast::StmtExpr { value, .. }) => matches!(
&**value,
ast::Expr::StringLiteral(_) | ast::Expr::EllipsisLiteral(_)
),
_ => false,
}
}
crates/ty_python_semantic/src/types/infer/tests.rs
+219 -39
View File
@@ -3,14 +3,16 @@ use std::fmt::Write;
use super::builder::TypeInferenceBuilder;
use crate::db::tests::{TestDb, TestDbBuilder, setup_db};
use crate::lint::{LintSource, RuleSelection};
use crate::place::symbol;
use crate::place::{ConsideredDefinitions, Place, PlaceAndQualifiers};
use crate::types::{KnownClass, KnownInstanceType, check_types};
use ruff_db::diagnostic::{Diagnostic, DiagnosticId};
use ruff_db::diagnostic::{Diagnostic, DiagnosticId, Severity};
use ruff_db::files::{File, system_path_to_file};
use ruff_db::system::DbWithWritableSystem as _;
use ruff_db::testing::{assert_function_query_was_not_run, assert_function_query_was_run};
use ruff_python_ast::PythonVersion;
use salsa::Database as _;
use salsa::plumbing::AsId;
use ty_python_core::definition::Definition;
use ty_python_core::program::{Program, ProgramSettings};
@@ -336,7 +338,14 @@ fn not_literal_string() -> anyhow::Result<()> {
);
db.write_dedented("src/a.py", &content)?;
assert_file_diagnostics(&db, "src/a.py", &[]);
assert_file_diagnostics(
&db,
"src/a.py",
&[
"An empty string is always falsy",
"An empty string is always falsy",
],
);
Ok(())
}
@@ -520,7 +529,8 @@ fn simple_assignment_does_not_enter_salsa_cycle() {
assert_eq!(cycles, Vec::<String>::new());
}
/// Checks widening when a comparison truthiness override is present in only one iteration.
/// Comparison truthiness widens consistently in expression, statement, and definition inference
/// when an override is present in only one iteration.
///
/// A missing override falls back to the expression type's truthiness. Widening must compare the
/// effective truthiness from both iterations, including this fallback. Discarding an override from
@@ -542,47 +552,217 @@ fn comparison_truthiness_widens_across_sparse_cycle_results() -> anyhow::Result<
let expression = ExpressionNodeKey::from(statement.value.as_ref());
let scope = global_scope(&db, file);
let env = ProgramEnvironment::from_scope(scope);
let inference = |ty, truthiness: Option<Truthiness>| ExpressionInference {
expressions: [(expression, ty)].into_iter().collect(),
extra: truthiness.map(|truthiness| {
Box::new(ExpressionInferenceExtra {
comparison_truthiness: [(expression, truthiness)].into_iter().collect(),
..ExpressionInferenceExtra::default()
})
}),
#[cfg(debug_assertions)]
scope,
let inference = |ty, truthiness: Option<Truthiness>| {
(
ExpressionInference {
expressions: [(expression, ty)].into_iter().collect(),
extra: truthiness.map(|truthiness| {
Box::new(ExpressionInferenceExtra {
comparison_truthiness: [(expression, truthiness)].into_iter().collect(),
..ExpressionInferenceExtra::default()
})
}),
#[cfg(debug_assertions)]
scope,
},
StatementInferenceInner {
expressions: [(expression, ty)].into_iter().collect(),
bindings: Box::default(),
declarations: Box::default(),
extra: truthiness.map(|truthiness| {
Box::new(StatementInferenceInnerExtra {
comparison_truthiness: [(expression, truthiness)].into_iter().collect(),
..StatementInferenceInnerExtra::default()
})
}),
#[cfg(debug_assertions)]
scope,
},
DefinitionInference {
expressions: [(expression, ty)].into_iter().collect(),
types: DefinitionTypes::Empty,
extra: truthiness.map(|truthiness| {
Box::new(DefinitionInferenceExtra::Other(Box::new(
OtherDefinitionInferenceExtra {
comparison_truthiness: [(expression, truthiness)].into_iter().collect(),
..OtherDefinitionInferenceExtra::default()
},
)))
}),
#[cfg(debug_assertions)]
scope,
},
)
};
// A previously widened condition stays ambiguous even when the new result omits its
// override and has a definite value-type fallback.
let previous = inference(Type::bool_literal(false), Some(Truthiness::Ambiguous));
let mut current = inference(Type::bool_literal(false), None);
current.widen_comparison_truthiness(&db, &env, &previous);
assert_eq!(
current.comparison_truthiness(expression),
Some(Truthiness::Ambiguous)
);
for (previous, current, expected) in [
// A previously widened condition stays ambiguous even when the new result omits its
// override and has a definite value-type fallback.
(
(Type::bool_literal(false), Some(Truthiness::Ambiguous)),
(Type::bool_literal(false), None),
Truthiness::Ambiguous,
),
// A new override is compared with the previous result's value-type fallback.
(
(Type::bool_literal(true), None),
(Type::unknown(), Some(Truthiness::AlwaysFalse)),
Truthiness::Ambiguous,
),
// Matching effective truthiness stays precise. Keep the override even though it agrees
// with the current type: subsequent type widening can make that fallback ambiguous again.
(
(Type::unknown(), Some(Truthiness::AlwaysFalse)),
(Type::bool_literal(false), None),
Truthiness::AlwaysFalse,
),
] {
let (previous_expression, previous_statement, previous_definition) =
inference(previous.0, previous.1);
let (mut current_expression, mut current_statement, mut current_definition) =
inference(current.0, current.1);
current_expression.widen_comparison_truthiness(&db, &env, &previous_expression);
current_statement.widen_comparison_truthiness(&db, &env, &previous_statement);
current_definition.widen_comparison_truthiness(&db, &env, &previous_definition);
assert_eq!(
current_expression.comparison_truthiness(expression),
Some(expected)
);
assert_eq!(
current_statement
.extra
.as_deref()
.and_then(|extra| extra.comparison_truthiness.get(&expression))
.copied(),
Some(expected)
);
assert_eq!(
current_definition
.extra
.as_deref()
.and_then(DefinitionInferenceExtra::comparison_truthiness)
.and_then(|overrides| overrides.get(&expression))
.copied(),
Some(expected)
);
}
// A new override is compared with the previous result's value-type fallback.
let previous = inference(Type::bool_literal(true), None);
let mut current = inference(Type::unknown(), Some(Truthiness::AlwaysFalse));
current.widen_comparison_truthiness(&db, &env, &previous);
assert_eq!(
current.comparison_truthiness(expression),
Some(Truthiness::Ambiguous)
);
Ok(())
}
// Matching effective truthiness stays precise. Keep the override even though it agrees with
// the current type: subsequent type widening can make that fallback ambiguous again.
let previous = inference(Type::unknown(), Some(Truthiness::AlwaysFalse));
let mut current = inference(Type::bool_literal(false), None);
current.widen_comparison_truthiness(&db, &env, &previous);
assert_eq!(
current.comparison_truthiness(expression),
Some(Truthiness::AlwaysFalse)
);
/// Resolving environment-guard provenance must not re-enter inference of the scope being checked.
/// This lookup runs during scope inference; asking for completed use-site types would create a
/// Salsa cycle. Cycle recovery can hide that mistake in the final diagnostics, so inspect Salsa's
/// events as well as checking that each condition produces a diagnostic.
#[test]
fn redundant_condition_lookup_does_not_reenter_scope_inference() -> anyhow::Result<()> {
// Cover builtin names, including the numeric-compatibility special cases for `float` and
// `complex`, and attribute lookup using an already-inferred receiver type.
for source in [
"if isinstance({}, dict):\n pass\n",
"if isinstance(1.0, float):\n pass\n",
"if isinstance(1j, complex):\n pass\n",
"class C:\n flag = (1, 2)\n\nif C.flag:\n pass\n",
] {
let registry = crate::default_lint_registry();
let mut rules = RuleSelection::from_registry(registry);
rules.enable(
registry.get("redundant-condition-strict")?,
Severity::Warning,
LintSource::File,
);
let mut db = TestDbBuilder::new()
.with_file("/src/main.py", source)
.with_rule_selection(rules)
.build()?;
let file = system_path_to_file(&db, "/src/main.py")?;
let diagnostics = check_types(&db, program_file(&db, file));
// Require the diagnostic so the cycle check cannot pass merely because the redundant
// condition was never checked.
assert_eq!(diagnostics.len(), 1, "{source}\n{diagnostics:#?}");
let events = db.take_salsa_events();
let scope_cycles = salsa::attach(&db, || {
events
.iter()
.filter_map(|event| match event.kind {
salsa::EventKind::WillIterateCycle { database_key, .. } => {
Some(format!("{database_key:?}"))
}
_ => None,
})
.filter(|query| query.starts_with("infer_scope_types_impl("))
.collect::<Vec<_>>()
});
assert!(scope_cycles.is_empty(), "{source}\n{scope_cycles:#?}");
}
Ok(())
}
/// Repeated conditions on the same name or attribute share one cached definition summary.
/// The first two fixtures combine many assignments to one place with many conditions that test it.
/// Each lookup can inspect every assignment, so repeating it for every condition would make
/// these examples quadratic even if their diagnostics were unchanged.
/// Conditions on distinct names also share the reachability summaries for preceding calls,
/// rather than traversing an increasingly long call prefix for each name.
#[test]
fn repeated_tuple_conditions_share_provenance() -> anyhow::Result<()> {
let repetitions = 100;
let names = "value = (1,)\nif value:\n pass\n".repeat(repetitions);
let attributes = format!(
"class C:\n{}\n{}",
" value = (1,)\n".repeat(repetitions),
"if C.value:\n pass\n".repeat(repetitions),
);
let mut calls = String::from(
"def noop() -> None: ...
",
);
for index in 0..repetitions {
writeln!(
calls,
"noop()
value_{index} = (1,)
if value_{index}:
pass"
)?;
}
for (source, query_name, max_queries) in [
(names, "name_condition_definition_info", 1),
(attributes, "attribute_condition_definition_info", 1),
(
calls,
"reachability_contains_special_cased_condition",
3 * repetitions,
),
] {
let mut db = TestDbBuilder::new()
.with_file("/src/main.py", &source)
.build()?;
let file = system_path_to_file(&db, "/src/main.py")?;
let diagnostics = check_types(&db, program_file(&db, file));
// Sharing the definition lookup must still leave a diagnostic on every condition.
assert_eq!(diagnostics.len(), repetitions);
// Count actual query executions, excluding cache hits. This checks reuse deterministically
// without a timing threshold, which would depend on the machine running the test.
let events = db.take_salsa_events();
let lookups = events
.iter()
.filter(|event| {
matches!(
event.kind,
salsa::EventKind::WillExecute { database_key }
if db.ingredient_debug_name(database_key.ingredient_index()) == query_name
)
})
.count();
assert!(
(1..=max_queries).contains(&lookups),
"{query_name} should be shared across conditions; executed {lookups} queries"
);
}
Ok(())
}
crates/ty_python_semantic/src/types/tuple.rs
+2 -2
View File
@@ -1226,7 +1226,7 @@ impl<T, V> VariableLengthTuple<T, V> {
self.suffix_elements().iter().copied()
}
fn fixed_elements(&self) -> impl Iterator<Item = &T> + '_ {
fn fixed_elements(&self) -> impl ExactSizeIterator<Item = &T> + '_ {
self.fixed_elements.iter()
}
@@ -2515,7 +2515,7 @@ impl<T, V> Tuple<T, V> {
}
/// Returns an iterator of all of the fixed-length element types of this tuple.
pub(crate) fn fixed_elements(&self) -> impl Iterator<Item = &T> + '_ {
pub(crate) fn fixed_elements(&self) -> impl ExactSizeIterator<Item = &T> + '_ {
match self {
Tuple::Fixed(tuple) => Either::Left(tuple.all_elements().iter()),
Tuple::Variable(tuple) => Either::Right(tuple.fixed_elements()),
crates/ty_test/src/db.rs
+4 -3
View File
@@ -354,12 +354,13 @@ fn mdtest_rule_selection(rules: Option<&Rules>, required_rule: Option<&str>) ->
// `experimental-syntax` is also an exception: we make use of `&` and `~` for intersection and
// negation types in our tests for better readability.
"experimental-syntax",
// The `unsound-*` rules are also exceptions because they are very strict, would
// result in lots of additional diagnostics in mdtests, and are not the default behaviour
// we'll show to our users.
// The `unsound-*` rules and `redundant-condition-strict` are also exceptions because they
// are very strict, would result in lots of additional diagnostics in mdtests, and are not
// the default behaviour we'll show to our users.
"unsound-assignment",
"unsound-return-statement",
"unsound-yield",
"redundant-condition-strict",
];
let registry = default_lint_registry();
ty.schema.json
Generated
+20
View File
@@ -1374,6 +1374,26 @@
}
]
},
"redundant-condition": {
"title": "detects conditions that are always truthy or always falsey",
"description": "## What it does\n\nDetects boolean conditions where the condition can be statically inferred to be always true or\nalways false due to the inferred type of the condition.\n\nThis rule is enabled by default, and is deliberately not comprehensive. In order to avoid false\npositives, it excludes conditions that meet any of these criteria:\n\n- The boolean test is inferred as evaluating to `True` itself, `False` itself, or an exact integer\n such as `1` or `0`.\n- The boolean test can be inferred as always evaluating to `True` and `False`, but this inference is\n due to boolean-test short-circuiting in `if` conditions, `while` conditions or `assert` tests\n rather than the inferred type of the boolean test.\n- The condition uses a walrus operator (`:=`). The assignment's side effect may be intentional, even\n when its result has fixed truthiness.\n\n## Why is this bad?\n\nA boolean condition that is always true or always false usually indicates a mistake in your code,\nand can often lead to incorrect behavior. If an `if` condition is inferred as always false,\nmoreover, ty will infer all code within that `if` branch as being unreachable, and will not report\nany diagnostics on code in that region.\n\n## Examples\n\nA common error that triggers this rule is to forget to call a function, for example:\n\n```py\nimport random\n\n\ndef should_do_action() -> bool:\n return random.choice([True, False])\n\n\n# oops! You forgot the parentheses here... this should have been `if should_do_action()`.\n# Because it's not, this will always be `True`:\nif should_do_action: # error: [redundant-condition]\n print(\"Doing stuff...\")\n```\n\nAnother common mistake is to forget to `await` a coroutine:\n\n```py\nimport random\n\n\nasync def should_do_async_action():\n return random.choice([True, False])\n\n\nasync def main():\n # oops! Forgot the await here... this should have been `if await should_do_async_action()`.\n # Because it's not, this will always be `True`:\n if should_do_async_action(): # error: [redundant-condition]\n print(\"Doing stuff async...\")\n```\n\nOr to forget that `tuple[X]` means \"A tuple with exactly one element\" rather than \"a tuple with an\narbitrary number of elements\" (for which you'd use `tuple[X, ...]`):\n\n```py\n# you almost certainly meant to write `tuple[str, ...]` here rather than `tuple[str]`...\ndef consume_tuples(x: tuple[str]):\n # ...and that means that this later condition is inferred as always being True by ty:\n if x: # error: [redundant-condition]\n print(\"Got a non-empty tuple\")\n```\n\nSome Pythonistas fall into the trap of thinking that a generator expression will be falsy if it has\nzero elements inside it -- but generator expressions are lazy, and so they're always truthy unless\nyou collect them into a tuple:\n\n```py\ndef test_my_data(data: list[int]):\n # this will always be `True`, because the asserted object is a `types.GeneratorType` instance,\n # not a `tuple`! `assert any(item for item in data if item > 42)`\n # is probably what you meant instead.\n assert (item for item in data if item > 42) # error: [redundant-condition]\n```\n\n## Boolean operators used to compute values\n\nThe rule checks `and` and `or` operands when the expression is used as a condition: in an `if`,\n`elif`, `while`, or `assert` test, a conditional expression, a comprehension filter, a match guard,\nor as the operand of `not`. It does not flag `and` or `or` expressions used to compute values --\neven if an operand in an `and` or `or` expression is always truthy, it doesn't necessarily make the\nexpression redundant:\n\n```py\ndef f(): ...\ndef g(): ...\n\n\ndef test(coinflip: bool):\n # could also be written as `func = f if coinflip else g`,\n # but use of an `and` expression for this is common in older codebases.\n func = coinflip and f or g\n\n # `func` will be the `f` function if `coinflip` is `True`,\n # and the `g` function otherwise\n func()\n```\n\nThis also allows calls that are deliberately always falsy but are used for their side effects:\n\n```py\nfrom unittest.mock import patch\n\n\ndef ask_to_continue() -> bool:\n return input(\"Continue? \") == \"yes\"\n\n\ndef test_ask_to_continue():\n prompts = []\n with patch(\n \"builtins.input\",\n side_effect=lambda prompt: prompts.append(prompt) or \"yes\",\n ):\n assert ask_to_continue()\n\n assert prompts == [\"Continue? \"]\n```\n\nBy contrast, `not` always produces a boolean, so we will still emit a diagnostic on the following\nexample -- negating the truthiness of a function object is pointless, since a function object is\nalways truthy:\n\n```py\ndef f(): ...\n\n\nvalue = not f # error: [redundant-condition]\n```\n\n## Known issues and workarounds\n\nThis rule can sometimes trigger on code that is not incorrect, but could be written in a clearer\nway. For example, the rule will flag this codLine truncated
"default": "warn",
"oneOf": [
{
"$ref": "#/definitions/Level"
}
]
},
"redundant-condition-strict": {
"title": "detects conditions that are always truthy or always falsey (strict)",
"description": "## What it does\n\nDetects boolean conditions where the condition can be statically inferred to be always true or\nalways false.\n\nThis rule is disabled by default. It exclusively covers cases that its sibling (enabled-by-default)\nrule `redundant-condition` does not cover. These cases often flag real bugs in user code, but also\nhave a significantly higher rate of unavoidable false positives than other cases.\n\nThis rule reports redundant conditions that meet any of these criteria:\n\n- The boolean test is inferred as evaluating to `True` itself, `False` itself, or an exact integer\n such as `1` or `0`.\n- Short-circuit evaluation means the condition can be guaranteed to be always truthy or always falsy\n despite fixed truthiness not being guaranteed by the inferred type of the expression's value\n (see \"Short-circuiting boolean conditions\" below for an example).\n- The condition uses a walrus operator (`:=`). The assignment's side effect may be intentional, even\n when its result has fixed truthiness.\n\n## Why is this bad?\n\nA boolean condition that is always true or always false usually indicates a mistake in your code,\nand can often lead to incorrect behavior. If an `if` condition is inferred as always false,\nmoreover, ty will infer all code within that `if` branch as being unreachable, and will not report\nany diagnostics on code in that region.\n\n## Examples\n\nA common error in Python code is to make the mistake of thinking that indexing into a `bytes` object\nwill get you an object of type `bytes`. But `bytes` work differently to `str`s in Python -- although\na string is a sequence of strings, a bytestring is a sequence of `int`s, so indexing into a `bytes`\nobject gives you an `int`. This rule can catch that error by alerting you to the fact that checking\nwhether a `bytes` object is unequal to an `int` will always evaluate to `True`:\n\n```py\ndef validate_record(data: bytes) -> None:\n if data[0] != b\"\\x1e\": # error: [redundant-condition-strict]\n raise ValueError(\"Invalid record separator\")\n```\n\nAnother common mistake is to assume that annotating `**kwargs` with `dict[str, str]` describes the\ndictionary containing the keyword arguments. In fact, a `**kwargs` annotation describes each\nindividual keyword argument, so this annotation says that every value is itself a dictionary.\nComparing one of those values with a string will therefore always evaluate to `False`:\n\n```py\ndef trace(**kwargs: dict[str, str]) -> None:\n if kwargs.get(\"operation\") == \"task\": # error: [redundant-condition-strict]\n print(\"Tracing task\")\n```\n\n## Short-circuiting boolean conditions\n\nIn some situations, ty can know that a condition will always be true, or it can know that a\ncondition will always be false, even when this is not guaranteed by the inferred type of that\ncondition. This is because of the way that Python short-circuits evaluation of conditions in the\ncontext of `if` tests, `while` tests and `assert` statements.\n\nConsider a class whose comparison method has an `object` return type:\n\n```py\nfrom typing_extensions import reveal_type\n\n\nclass Comparable:\n def __lt__(self, other: int) -> object: ...\n\n\ndef check(value: Comparable):\n reveal_type(value < 1 < 0) # revealed: ~AlwaysTruthy\n\n if value < 1 < 0: # error: [redundant-condition-strict] \"always false\"\n pass\n```\n\nOutside the context of an `if` test, the revealed type of the condition here is `~AlwaysTruthy`: in\nother words, ty knows that this expression is not *always true*, but cannot guarantee that it is\ndefinitely *always false*. It could be an object that is sometimes true and sometimes false -- for\nexample, a `list` (which is falsy when it is empty, and truthy otherwise).\n\nNonetheless, when `value < 1 < 0` is used directly as a condition, ty knows that the condition will\nalways be falsy and the `if` branch will never be taken. Python tests the truthiness of the object\nreturned by `Comparable.__lt__` once: if it is falsy, the condition fails immediately. If it is\ntruthy, Python evaluates `1 < 0`, which is false. There is no second truthiness test of the object\nreturned by `__lt__`.\n\nIf the chained comparison is saved as a variable first, its value can be the object returned by\n`__lt__`, if that object was falsy when first tested. The `if result` statement then tests that\nobject's truthiness again. A user-defined `__bool__` method can return a different result on that\nsecond call, so ty cannot guarantee that the saved value is still falsy, and no diagnostic is\nemitted:\n\n```py\ndef check_saved(value: Comparable):\n result = value < 1 < 0\n if result: # no diagnostic\n pass\n```\n\n## Exemptions\n\nLike `redundant-condition`, this rule checks subexpressions of an `and` or `or` expression only when\nthe outer expression is used as a condition. This is to avoid emitting false-positive diagnostics on\ncLine truncated
"default": "ignore",
"oneOf": [
{
"$ref": "#/definitions/Level"
}
]
},
"redundant-final-classvar": {
"title": "detects redundant combinations of `ClassVar` and `Final`",
"description": "## What it does\n\nChecks for redundant combinations of the `ClassVar` and `Final` type qualifiers.\n\n## Why is this bad?\n\nAn attribute that is marked `Final` in a class body is implicitly a class variable. Marking it as\n`ClassVar` is therefore redundant.\n\nNote that this diagnostic is not emitted for dataclass fields or protocol members, where\n`ClassVar[Final[int]]` has a distinct meaning from `Final[int]`.\n\n## Examples\n\n```python\nfrom typing import ClassVar, Final\n\n\nclass C:\n # redundant\n x: ClassVar[Final[int]] = 1 # error\n # redundant\n y: Final[ClassVar[int]] = 1 # error\n```",