fix improve enum error suggestion #3128#3145
fix improve enum error suggestion #3128#3145asukaminato0721 wants to merge 1 commit intofacebook:mainfrom
Conversation
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
There was a problem hiding this comment.
Pull request overview
Adds enum-member “did you mean …” suggestions to call-argument type mismatch diagnostics when a provided literal exactly matches an enum’s underlying value (fixing #3128).
Changes:
- Add enum-value → enum-member suggestion helper in enum handling code.
- Extend call-argument type-check error reporting to append an enum-member suggestion line when applicable.
- Add a regression test covering
StrEnumargument mismatch suggestions.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
pyrefly/lib/test/enums.rs |
Adds regression test asserting the new call-argument suggestion text for StrEnum. |
pyrefly/lib/alt/class/enums.rs |
Implements logic to map a raw literal type to a unique enum member suggestion. |
pyrefly/lib/alt/answers_solver.rs |
Injects enum-member suggestion lines into call-argument diagnostics during check_type failures. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let candidate = self.suggest_enum_member_for_value(member, got)?; | ||
| if suggestion | ||
| .as_ref() | ||
| .is_some_and(|existing: &String| existing != &candidate) | ||
| { | ||
| return None; | ||
| } | ||
| suggestion = Some(candidate); |
There was a problem hiding this comment.
Type::Union handling here short-circuits to None when any union member doesn't produce an enum-member suggestion (due to the ?). That means common expected types like T | None (or T | Other) will never suggest T.A even if the provided literal exactly matches a single enum value. Consider collecting suggestions from union members, ignoring None results, and returning a suggestion only when there is exactly one distinct candidate across the union.
| let candidate = self.suggest_enum_member_for_value(member, got)?; | |
| if suggestion | |
| .as_ref() | |
| .is_some_and(|existing: &String| existing != &candidate) | |
| { | |
| return None; | |
| } | |
| suggestion = Some(candidate); | |
| if let Some(candidate) = self.suggest_enum_member_for_value(member, got) { | |
| if suggestion | |
| .as_ref() | |
| .is_some_and(|existing: &String| existing != &candidate) | |
| { | |
| return None; | |
| } | |
| suggestion = Some(candidate); | |
| } |
| let enum_metadata = self.get_metadata_for_class(cls).enum_metadata()?.clone(); | ||
| let mut suggestion = None; | ||
| for lit in self.get_enum_members(cls) { | ||
| let Lit::Enum(lit_enum) = lit else { | ||
| unreachable!("enum members must be represented as enum literals"); | ||
| }; | ||
| let value_ty = | ||
| self.enum_literal_to_value_type((*lit_enum).clone(), enum_metadata.is_django); |
There was a problem hiding this comment.
enum_metadata is cloned here but only is_django is used. To avoid unnecessary cloning (which includes a ClassType), consider storing just the needed boolean (e.g., let is_django = ...enum_metadata()?.is_django;) and passing that into enum_literal_to_value_type.
| let enum_metadata = self.get_metadata_for_class(cls).enum_metadata()?.clone(); | |
| let mut suggestion = None; | |
| for lit in self.get_enum_members(cls) { | |
| let Lit::Enum(lit_enum) = lit else { | |
| unreachable!("enum members must be represented as enum literals"); | |
| }; | |
| let value_ty = | |
| self.enum_literal_to_value_type((*lit_enum).clone(), enum_metadata.is_django); | |
| let is_django = self.get_metadata_for_class(cls).enum_metadata()?.is_django; | |
| let mut suggestion = None; | |
| for lit in self.get_enum_members(cls) { | |
| let Lit::Enum(lit_enum) = lit else { | |
| unreachable!("enum members must be represented as enum literals"); | |
| }; | |
| let value_ty = self.enum_literal_to_value_type((*lit_enum).clone(), is_django); |
| let tcc = tcc(); | ||
| let msg = tcc.kind.format_error( | ||
| &self.for_display(got.clone()), | ||
| &self.for_display(want.clone()), | ||
| errors.module().name(), | ||
| ); | ||
| let mut msg_lines = vec1![msg]; | ||
| if matches!( | ||
| tcc.kind, | ||
| TypeCheckKind::CallArgument(..) | ||
| | TypeCheckKind::CallVarArgs(..) | ||
| | TypeCheckKind::CallKwArgs(..) | ||
| | TypeCheckKind::CallUnpackKwArg(..) | ||
| ) && let Some(suggestion) = self.suggest_enum_member_for_value(want, got) | ||
| { | ||
| msg_lines.push(format!("Did you mean `{suggestion}`?")); | ||
| } | ||
| if let Some(subset_error_msg) = error.to_error_msg() { | ||
| msg_lines.push(subset_error_msg); | ||
| } | ||
| let extra_annotations = tcc.annotations; | ||
| match tcc.context { | ||
| Some(ctx) => { | ||
| errors.add_with_annotations( | ||
| loc, | ||
| ErrorInfo::Context(&|| ctx.clone()), | ||
| msg_lines, | ||
| extra_annotations, | ||
| ); | ||
| } | ||
| None => { | ||
| errors.add_with_annotations( | ||
| loc, | ||
| ErrorInfo::Kind(tcc.kind.as_error_kind()), | ||
| msg_lines, | ||
| extra_annotations, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
check_type now duplicates the error-message construction logic from solver::Solver::error in order to inject the enum suggestion. This creates a maintenance risk (future changes to formatting/annotations in the solver can diverge from this path). Consider refactoring to share the common message-building logic (e.g., a helper that returns Vec1<String> given got/want/tcc/subset_error, with an option to append extra lines).
rchen152
left a comment
There was a problem hiding this comment.
The Copilot feedback looks legitimate here - specifically, the concerns about union handling and code duplication.
Summary
Fixes #3128
The change teaches call-argument diagnostics to suggest an enum member when the provided raw literal exactly matches a single expected enum value.
Test Plan
add test