fix: skip unannotated-return check for @no_type_check functions#3572
Open
Arths17 wants to merge 5 commits into
Open
fix: skip unannotated-return check for @no_type_check functions#3572Arths17 wants to merge 5 commits into
Arths17 wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts Pyrefly’s decorator-aware function validation so that functions decorated with @typing.no_type_check (or typing_extensions.no_type_check) are excluded from return-annotation validation, addressing false-positive unannotated-return reports (Fixes #3561). It also adds a targeted decorator diagnostic for a common “missing injected parameter” pattern.
Changes:
- Track
@no_type_checkas a special decorator and store it inFuncFlags. - Skip return-annotation (and currently other signature) validations when
has_no_type_checkis set. - Add a specialized
InvalidDecoratormessage when a decorator injects exactly one required parameter that the decoratee is missing, plus tests for both behaviors.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
pyrefly/lib/test/untyped_def_behaviors.rs |
Adds a regression testcase ensuring @no_type_check suppresses return-annotation validation. |
pyrefly/lib/test/decorators.rs |
Adds a testcase for the new “missing injected parameter” decorator diagnostic. |
pyrefly/lib/alt/types/decorated_function.rs |
Introduces SpecialDecorator::NoTypeCheck. |
pyrefly/lib/alt/function.rs |
Detects @no_type_check, sets FuncFlags::has_no_type_check, gates validation, and adds decorator diagnostic specialization. |
crates/pyrefly_types/src/callable.rs |
Adds has_no_type_check to FuncFlags. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+575
to
611
| let contains_self_type = |ty: &Type| ty.any(|t| matches!(t, Type::SelfType(_))); | ||
|
|
||
| if !def.metadata.flags.has_no_type_check { | ||
| // `stmt.returns` is always set to None because the binding step calls `mem::take` on it | ||
| let has_return_annotation = self.bindings().function_has_return_annotation(&stmt.name); | ||
| if !has_return_annotation { | ||
| self.error( | ||
| errors, | ||
| p.name().range(), | ||
| ErrorKind::ImplicitAnyParameter, | ||
| format!( | ||
| "`{}` is missing an annotation for parameter `{name}`", | ||
| stmt.name | ||
| ), | ||
| stmt.name.range(), | ||
| ErrorKind::UnannotatedReturn, | ||
| format!("`{}` is missing a return annotation", stmt.name), | ||
| ); | ||
| } | ||
| } | ||
| // Only validate TypeGuard/TypeIs functions when they have an explicit return annotation. | ||
| // Functions that return a TypeGuard value without an explicit annotation should not be | ||
| // treated as TypeGuard functions. | ||
| if has_return_annotation { | ||
| if matches!(&ret, Type::TypeGuard(_) | Type::TypeIs(_)) { | ||
| self.validate_type_guard_positional_argument_count( | ||
| &def.params, | ||
| def.id_range(), | ||
| &def.defining_cls, | ||
| def.metadata.flags.is_staticmethod, | ||
| errors, | ||
| ); | ||
|
|
||
| // The first parameter of a non-static method is the implicit self/cls | ||
| // parameter and does not require an annotation, regardless of its name. | ||
| // __new__ is an implicit staticmethod but still takes cls as its first parameter. | ||
| // If the first parameter is variadic (e.g. *args), self is passed inside it, | ||
| // so there is no separate implicit parameter to skip. | ||
| for (i, p) in stmt.parameters.iter().enumerate() { | ||
| // Skip first param if it's implicit self/cls and not variadic | ||
| if i == 0 && has_implicit_self_or_cls_param && !p.is_variadic() { | ||
| continue; | ||
| } | ||
| if p.annotation().is_none() { | ||
| let name = p.name().as_str(); | ||
| self.error( | ||
| errors, | ||
| p.name().range(), | ||
| ErrorKind::ImplicitAnyParameter, | ||
| format!( | ||
| "`{}` is missing an annotation for parameter `{name}`", | ||
| stmt.name | ||
| ), | ||
| ); | ||
| } | ||
| } |
Comment on lines
+1624
to
+1670
| fn decorator_missing_injected_parameter_message( | ||
| &self, | ||
| decorator: &Type, | ||
| decoratee_name: &Identifier, | ||
| decoratee: &Type, | ||
| ) -> Option<String> { | ||
| let decorator_name = decorator | ||
| .visit_toplevel_func_metadata(&|meta| Some(meta.kind.function_name().to_string()))?; | ||
| let expected_decoratee = decorator.callable_first_param(self.heap)?; | ||
| let expected_signature = expected_decoratee | ||
| .callable_signatures() | ||
| .into_iter() | ||
| .find(|signature| { | ||
| matches!(&signature.params, Params::ParamSpec(prefix, _) if !prefix.is_empty()) | ||
| })?; | ||
|
|
||
| let actual_signature = match decoratee { | ||
| Type::Function(func) => &func.signature, | ||
| Type::Callable(callable) => callable.as_ref(), | ||
| _ => return None, | ||
| }; | ||
|
|
||
| let Params::ParamSpec(prefix, _) = &expected_signature.params else { | ||
| return None; | ||
| }; | ||
| let Params::List(actual_params) = &actual_signature.params else { | ||
| return None; | ||
| }; | ||
| if actual_params.len() + 1 != prefix.len() { | ||
| return None; | ||
| } | ||
|
|
||
| let missing = prefix.get(actual_params.len())?; | ||
| let missing_ty = match missing { | ||
| PrefixParam::PosOnly(_, ty, Required::Required) | ||
| | PrefixParam::Pos(_, ty, Required::Required) => ty, | ||
| PrefixParam::PosOnly(_, _, Required::Optional(_)) | ||
| | PrefixParam::Pos(_, _, Required::Optional(_)) => return None, | ||
| }; | ||
| Some(format!( | ||
| "Function `{}` is missing parameter of type `{}` injected by decorator `{}`", | ||
| decoratee_name.as_str(), | ||
| self.for_display(missing_ty.clone()), | ||
| decorator_name, | ||
| )) | ||
| } | ||
|
|
Comment on lines
+495
to
+503
| testcase!( | ||
| test_no_type_check_without_return_annotation, | ||
| r#" | ||
| from typing import no_type_check | ||
|
|
||
| @no_type_check | ||
| def f(): | ||
| pass | ||
| "#, |
This comment has been minimized.
This comment has been minimized.
yangdanny97
requested changes
May 26, 2026
Contributor
yangdanny97
left a comment
yangdanny97
left a comment
There was a problem hiding this comment.
Can you clarify the changes in this PR? They seem much larger than I would expect is needed for implementing this feature.
I expected def.metadata.flags.has_no_type_check to be saved in some variable at the top, and then a check to that variable to be added to the gate of the unannotated-return, like so
if !has_return_annotation && !no_type_check {
self.error(
errors,
stmt.name.range(),
ErrorKind::UnannotatedReturn,
format!("`{}` is missing a return annotation", stmt.name),
);
}
I'm not sure why a lot of other logic was changed in function.rs
Contributor
Author
|
You’re right. I trimmed this PR down to the minimal @no_type_check fix: it now only skips the UnannotatedReturn check, and I removed the unrelated decorator changes. The branch has been updated. |
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Before
@no_type_checkfunctions could still triggerunannotated-return.After
Functions marked with
@no_type_checkare excluded from return-annotation validation.Guarantee
Normal function behavior is unchanged.
This only affects the decorated skip logic for
@no_type_checkfunctions.Fixes #3561