Skip to content
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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

vladNed
Copy link
Contributor

@vladNed vladNed commented Feb 17, 2025

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

  • Checked with type aliases with both mutable and immutable
  • Checked the original behavior was intact.

Copy link
Contributor

github-actions bot commented Feb 17, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@vladNed
Copy link
Contributor Author

vladNed commented Feb 17, 2025

figured that there should be a loop for type inference to resolve until final if a alias is mapped to another alias.

@vladNed
Copy link
Contributor Author

vladNed commented Feb 17, 2025

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 {
Copy link
Member

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

Suggested change
let Some(expr_name) = expr.clone().name_expr() else {
let Some(expr_name) = expr.as_name_expr() else {

Comment on lines +214 to +224
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,
}
Copy link
Member

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.

/// 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)
}

Copy link
Contributor Author

@vladNed vladNed Feb 19, 2025

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 ?

Copy link
Member

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 an Exp 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.

@dhruvmanila dhruvmanila added the bug Something isn't working label Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RUF012 does not respect type aliases
2 participants