-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ruff] Resolve type alias for annotations (RUF012) #16210
base: main
Are you sure you want to change the base?
[ruff] Resolve type alias for annotations (RUF012) #16210
Conversation
|
figured that there should be a loop for type inference to resolve until final if a alias is mapped to another alias. |
added a small improvement, should be good for review now |
/// If an annotation [`Expr`] is bound to a type alias, this function will return the inferred | ||
/// value of the type alias. | ||
pub(super) fn map_annotation_binding<'a>(expr: &'a Expr, semantic: &'a SemanticModel) -> &'a Expr { | ||
let Some(expr_name) = expr.clone().name_expr() else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can avoid the clone here
let Some(expr_name) = expr.clone().name_expr() else { | |
let Some(expr_name) = expr.as_name_expr() else { |
let Some(binding) = semantic | ||
.resolve_name(&expr_name) | ||
.map(|id| semantic.binding(id)) | ||
else { | ||
return expr; | ||
}; | ||
|
||
match find_binding_value(binding, semantic) { | ||
Some(value) => map_annotation_binding(map_subscript(value), semantic), | ||
None => expr, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use find_assigned_value
instead? This will only look in the current scope and will work with forward references as well.
ruff/crates/ruff_python_semantic/src/analyze/typing.rs
Lines 1137 to 1151 in f58a54f
/// Find the assigned [`Expr`] for a given symbol, if any. | |
/// | |
/// For example given: | |
/// ```python | |
/// foo = 42 | |
/// (bar, bla) = 1, "str" | |
/// ``` | |
/// | |
/// This function will return a `NumberLiteral` with value `Int(42)` when called with `foo` and a | |
/// `StringLiteral` with value `"str"` when called with `bla`. | |
pub fn find_assigned_value<'a>(symbol: &str, semantic: &'a SemanticModel<'a>) -> Option<&'a Expr> { | |
let binding_id = semantic.lookup_symbol(symbol)?; | |
let binding = semantic.binding(binding_id); | |
find_binding_value(binding, semantic) | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhruvmanila I tested with find_assigned_value
but its not fixing the forwarded value. If you import the type it does not resolves it completely.
I tried using multiple method with ResolvedReferences
and with resolve_qualified_name
but I didn't had any success on this.
Is there any function that resolves a FromImport
to an Exp
somehow ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late reply, I somehow missed this in the notification.
Can you provide an example of what you're trying to test here?
Is there any function that resolves a
FromImport
to anExp
somehow ?
Do you mean to follow the import and resolve it to the expression that defines it? That's not possible as it requires multi-file analysis.
Summary
Check on mutable class default was not resolving the annotation that was a type alias thus the default values might have been flagged as mutable.
Added a helper function similar to
map_subscript
in order to check bindings on annotation to resolve to the final type.Resolves #16174
Test Plan