fix [feature] warn when passing str to something that accepts iterable[str] or sequence[str] #673#2293
fix [feature] warn when passing str to something that accepts iterable[str] or sequence[str] #673#2293asukaminato0721 wants to merge 9 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
yangdanny97
left a comment
yangdanny97
left a comment
There was a problem hiding this comment.
This seems to change the positions of a bunch of unrelated errors.
I think the change could be smaller and more self-contained.
For now, what if we didn't consider unions or assignability at all, and just made it error if the expected type is exactly Iterable[str] or Sequence[str] and the actual type is exactly str?
For the initial impl we probably don't even need to consider unions.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@yangdanny97 has imported this pull request. If you are a Meta employee, you can view this in D92301565. |
|
This is a lot of new errors, I'll need to think about how to restrict this so we don't give so many false positives. Additionally, it should probably be off by default |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Adds a new diagnostic to Pyrefly that warns when a str is passed to parameters typed as Iterable[str] or Sequence[str], aiming to catch the common footgun where strings are treated as iterables of characters.
Changes:
- Introduces
ErrorKind::StringAsIterable(default severitywarn) and emits it during argument type-checking when the pattern is detected. - Documents the new error kind and adds a regression test covering
Iterable[str]/Sequence[str]and an explicit| strallowlist case. - Refactors overload calling to optionally retry without a hint when hint-driven checking produces call errors.
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 |
|---|---|
website/docs/error-kinds.mdx |
Adds documentation for the new string-as-iterable diagnostic. |
pyrefly/lib/test/calls.rs |
Adds a test case asserting the new warning message for Iterable[str] and Sequence[str]. |
pyrefly/lib/alt/overload.rs |
Updates overload call flow to retry without hint when hint-driven call errors occur. |
pyrefly/lib/alt/callable.rs |
Implements the string-as-iterable warning and threads it through argument checking. |
crates/pyrefly_config/src/error_kind.rs |
Adds StringAsIterable to ErrorKind and assigns default severity warn. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn check_expr_argument( | ||
| &self, | ||
| expr: &Expr, | ||
| hint: &Type, | ||
| range: TextRange, | ||
| arg_errors: &ErrorCollector, | ||
| call_errors: &ErrorCollector, | ||
| tcc: &dyn Fn() -> TypeCheckContext, | ||
| ) -> (Type, bool) { | ||
| if hint.is_any() { | ||
| return ( | ||
| self.expr_infer_type_info_with_hint(expr, None, arg_errors) | ||
| .into_ty(), | ||
| false, | ||
| ); | ||
| } | ||
| let got = self.expr_infer_type_info_with_hint( | ||
| expr, | ||
| Some(HintRef::new(hint, Some(call_errors))), | ||
| arg_errors, | ||
| ); | ||
| let ok = self.check_type(got.ty(), hint, range, call_errors, tcc); | ||
| (got.into_ty(), ok) | ||
| } |
There was a problem hiding this comment.
check_expr_argument always returns the inferred type even when check_type fails. Previously expr_with_separate_check_errors would return want on failure (via check_and_return_type_info), which helps prevent cascading type errors and keeps downstream inference consistent. Consider restoring that behavior by returning hint (or got.with_ty(hint.clone())) when ok is false.
| if got.is_error() || got.is_any() || want.is_any() { | ||
| return; | ||
| } | ||
| let got_is_str = matches!(got, Type::ClassType(cls) if cls.is_builtin("str")); | ||
| if !got_is_str { | ||
| return; | ||
| } |
There was a problem hiding this comment.
The new warning only triggers when got is exactly Type::ClassType(str), so common cases like passing a string literal ("hello") or a LiteralString-typed value (both represented as literal-string types) won't warn even though they are definitely str at runtime. Consider treating got.is_literal_string() as str for this check (and add a test for the literal-literal case).
| // We want to use our hint to contextually type the arguments, but errors resulting | ||
| // from the hint should not influence overload selection. If there are call errors, we | ||
| // try again without a hint in case we can still match this overload. | ||
| let (call_errors, res, expected_types) = try_call(hint); | ||
| let (call_errors, res, expected_types) = | ||
| if tparams.is_some() && hint.is_some() && !call_errors.is_empty() { | ||
| try_call(None) | ||
| } else { | ||
| (call_errors, res, expected_types) | ||
| }; |
There was a problem hiding this comment.
This overload-calling logic now may run callable_infer twice (first with a hint, then without) to avoid hint-driven errors affecting overload matching. Since this is a behavior/perf change beyond the new string-as-iterable warning, it would be good to mention it in the PR description (or split it) so the diagnostic/selection change is reviewed intentionally.
There was a problem hiding this comment.
is this the reason for the new errors o nmatching overloads? is this necessary for this change?
There was a problem hiding this comment.
Yes, but only for specialization errors, not NoMatchingOverload.
We still retry without the hint when there are call_errors, so hint-driven call-shape failures should not affect overload selection.
The change is that we do not retry away specialization errors anymore.
That is necessary here because the contextual hint from bounded_str(...) is what exposes the TypeVar bound violation; retrying without the hint loses the expected diagnostic.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
those new errors don't seem correct, do they? also, some test failures |
This comment has been minimized.
This comment has been minimized.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
yangdanny97
left a comment
yangdanny97
left a comment
There was a problem hiding this comment.
Review automatically exported from Phabricator review in Meta.
|
This pull request has been merged in 24e203f. |
|
fyi: I redid a little of your PR in this based upon some feedback:
|
Summary
Fixes #673
Implemented the new string-as-iterable warning and documented it, with checks that fire only when the argument is definitely str and the parameter is Iterable[str]/Sequence[str] without explicitly allowing str.
Test Plan
add test