Skip to content

Commit

Permalink
Make it work, address review feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
MichaReiser committed Dec 24, 2024
1 parent dc54d66 commit 4950c7c
Show file tree
Hide file tree
Showing 5 changed files with 90 additions and 62 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,28 @@ def test() -> int:
return a + 5
```

## Error in decorator
## Error in preceding decorator

Both MyPy and Pyright flag the `unknown_decorator` but we don't.
Don't suppress diagnostics for decorators appearing before the `no_type_check` decorator.

```py
from typing import no_type_check

@unknown_decorator # error: [unresolved-reference]
@no_type_check
def test() -> int:
return a + 5
```

## Error in following decorator

Suppress diagnostics for decorators appearing after the `no_type_check` decorator.

```py
from typing import no_type_check

@unknown_decorator
@no_type_check
@unknown_decorator
def test() -> int:
return a + 5
```
Expand All @@ -71,7 +84,11 @@ def test() -> Undefined:

## `no_type_check` on classes isn't supported

Similar to Pyright, Red Knot does not support `no_type_check` annotations on classes.

Red Knot does not support `no_type_check` annotations on classes currently.
The behaviour of `no_type_check` when applied to classes is [not fully specified currently](https://typing.readthedocs.io/en/latest/spec/directives.html#no-type-check), and applying the decorator to classes is not supported by Pyright and possibly other type checkers.

A future improvement might be to emit a diagnostic if a `no_type_check` annotation is applied to a class.

```py
from typing import no_type_check
Expand All @@ -89,6 +106,6 @@ from typing import no_type_check

@no_type_check
def test():
# error: [unused-ignore-comment]
# error: [unused-ignore-comment]
return x + 5 # knot: ignore[unresolved-reference]
```
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ use rustc_hash::{FxHashMap, FxHashSet};
use ruff_db::files::File;
use ruff_db::parsed::ParsedModule;
use ruff_index::IndexVec;
use ruff_python_ast as ast;
use ruff_python_ast::name::Name;
use ruff_python_ast::visitor::{walk_expr, walk_pattern, walk_stmt, Visitor};
use ruff_python_ast::{self as ast};

use crate::ast_node_ref::AstNodeRef;
use crate::module_name::ModuleName;
Expand Down
7 changes: 2 additions & 5 deletions crates/red_knot_python_semantic/src/semantic_index/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -462,11 +462,8 @@ impl NodeWithScopeKind {
}
}

pub fn expect_function(&self) -> &ast::StmtFunctionDef {
match self {
Self::Function(function) => function.node(),
_ => panic!("expected function"),
}
pub const fn expect_function(&self) -> &ast::StmtFunctionDef {
self.as_function().expect("expected function")
}

pub fn expect_type_alias(&self) -> &ast::StmtTypeAlias {
Expand Down
95 changes: 49 additions & 46 deletions crates/red_knot_python_semantic/src/types/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use ruff_db::{
files::File,
};
use ruff_python_ast::AnyNodeRef;
use ruff_text_size::{Ranged, TextRange};
use ruff_text_size::Ranged;

use super::{binding_ty, KnownFunction, TypeCheckDiagnostic, TypeCheckDiagnostics};

Expand Down Expand Up @@ -35,6 +35,7 @@ pub(crate) struct InferContext<'db> {
scope: ScopeId<'db>,
file: File,
diagnostics: std::cell::RefCell<TypeCheckDiagnostics>,
no_type_check: InNoTypeCheck,
bomb: DebugDropBomb,
}

Expand All @@ -45,6 +46,7 @@ impl<'db> InferContext<'db> {
scope,
file: scope.file(db),
diagnostics: std::cell::RefCell::new(TypeCheckDiagnostics::default()),
no_type_check: InNoTypeCheck::default(),
bomb: DebugDropBomb::new("`InferContext` needs to be explicitly consumed by calling `::finish` to prevent accidental loss of diagnostics."),
}
}
Expand Down Expand Up @@ -81,7 +83,7 @@ impl<'db> InferContext<'db> {
return;
};

if self.is_in_no_type_check(node.range()) {
if self.is_in_no_type_check() {
return;
}

Expand Down Expand Up @@ -124,50 +126,40 @@ impl<'db> InferContext<'db> {
});
}

fn is_in_no_type_check(&self, range: TextRange) -> bool {
// Accessing the semantic index here is fine because
// the index belongs to the same file as for which we emit the diagnostic.
let index = semantic_index(self.db, self.file);

let scope_id = self.scope.file_scope_id(self.db);

// Unfortunately, we can't just use the `scope_id` here because the default values, return type
// and other parts of a function declaration are inferred in the outer scope, and not in the function's scope.
// That's why we walk all child-scopes to see if there's any child scope that fully contains the diagnostic range
// and if there's any, use that scope as the starting scope instead.
// We could probably use a binary search here but it's probably not worth it, considering that most
// scopes have only very few child scopes and binary search also isn't free.
let enclosing_scope = index
.child_scopes(scope_id)
.find_map(|(child_scope_id, scope)| {
if scope
.node()
.as_function()
.is_some_and(|function| function.range().contains_range(range))
{
Some(child_scope_id)
} else {
None
}
})
.unwrap_or(scope_id);

// Inspect all enclosing function scopes walking bottom up and infer the function's type.
let mut function_scope_tys = index
.ancestor_scopes(enclosing_scope)
.filter_map(|(_, scope)| scope.node().as_function())
.filter_map(|function| {
binding_ty(self.db, index.definition(function)).into_function_literal()
});

// Iterate over all functions and test if any is decorated with `@no_type_check`.
function_scope_tys.any(|function_ty| {
function_ty
.decorators(self.db)
.iter()
.filter_map(|decorator| decorator.into_function_literal())
.any(|decorator_ty| decorator_ty.is_known(self.db, KnownFunction::NoTypeCheck))
})
pub(super) fn set_in_no_type_check(&mut self, no_type_check: InNoTypeCheck) {
self.no_type_check = no_type_check;
}

fn is_in_no_type_check(&self) -> bool {
match self.no_type_check {
InNoTypeCheck::IfEnclosingFunction => {
// Accessing the semantic index here is fine because
// the index belongs to the same file as for which we emit the diagnostic.
let index = semantic_index(self.db, self.file);

let scope_id = self.scope.file_scope_id(self.db);

// Inspect all enclosing function scopes walking bottom up and infer the function's type.
let mut function_scope_tys = index
.ancestor_scopes(scope_id)
.filter_map(|(_, scope)| scope.node().as_function())
.filter_map(|function| {
binding_ty(self.db, index.definition(function)).into_function_literal()
});

// Iterate over all functions and test if any is decorated with `@no_type_check`.
function_scope_tys.any(|function_ty| {
function_ty
.decorators(self.db)
.iter()
.filter_map(|decorator| decorator.into_function_literal())
.any(|decorator_ty| {
decorator_ty.is_known(self.db, KnownFunction::NoTypeCheck)
})
})
}
InNoTypeCheck::Yes => true,
}
}

#[must_use]
Expand All @@ -189,6 +181,17 @@ impl fmt::Debug for InferContext<'_> {
}
}

#[derive(Copy, Clone, Debug, PartialEq, Eq, Default)]
pub(crate) enum InNoTypeCheck {
/// The inference might be in a `no_type_check` block but only if any
/// enclosing function is decorated with `@no_type_check`.
#[default]
IfEnclosingFunction,

/// The inference is known to be in an `@no_type_check` decorated function.
Yes,
}

pub(crate) trait WithDiagnostics {
fn diagnostics(&self) -> &TypeCheckDiagnostics;
}
21 changes: 16 additions & 5 deletions crates/red_knot_python_semantic/src/types/infer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ use crate::unpack::Unpack;
use crate::util::subscript::{PyIndex, PySlice};
use crate::Db;

use super::context::{InferContext, WithDiagnostics};
use super::context::{InNoTypeCheck, InferContext, WithDiagnostics};
use super::diagnostic::{
report_index_out_of_bounds, report_invalid_exception_caught, report_invalid_exception_cause,
report_invalid_exception_raised, report_non_subscriptable,
Expand Down Expand Up @@ -1024,10 +1024,19 @@ impl<'db> TypeInferenceBuilder<'db> {
decorator_list,
} = function;

let decorator_tys: Box<[Type]> = decorator_list
.iter()
.map(|decorator| self.infer_decorator(decorator))
.collect();
let mut decorator_tys = Vec::with_capacity(decorator_list.len());
for decorator in decorator_list {
let ty = self.infer_decorator(decorator);
decorator_tys.push(ty);

// Check if the function is decorated with the `no_type_check` decorator
// and, if so, suppress any errors.
if let Type::FunctionLiteral(function) = ty {
if function.is_known(self.db(), KnownFunction::NoTypeCheck) {
self.context.set_in_no_type_check(InNoTypeCheck::Yes);
}
}
}

for default in parameters
.iter_non_variadic_params()
Expand All @@ -1050,6 +1059,8 @@ impl<'db> TypeInferenceBuilder<'db> {
}
}

let decorator_tys = decorator_tys.into_boxed_slice();

let function_kind =
KnownFunction::try_from_definition_and_name(self.db(), definition, name);

Expand Down

0 comments on commit 4950c7c

Please sign in to comment.