Skip to content

[ty] Fix panic for attribute expressions with empty value #19069

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 9, 2025
Merged

[ty] Fix panic for attribute expressions with empty value #19069

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions crates/ty_python_semantic/resources/corpus/88_regression_issue_738.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
[.
7 changes: 7 additions & 0 deletions crates/ty_python_semantic/src/types/infer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5675,6 +5675,13 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
};
(place, None)
} else {
if expr_ref
.as_name_expr()
.is_some_and(|name| name.is_invalid())
{
return (Place::Unbound, None);
}
Comment on lines +5678 to +5683
Copy link
Contributor Author

@sharkdp sharkdp Jul 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably not the best (and not the broadest) fix here. Maybe @mtshiba could have a look at this if you find the time?

Copy link
Contributor

@mtshiba mtshiba Jul 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late review. This fix looks good!

One thing I am wondering about is our current handling of (syntactically) invalid place expressions.
The parser seems to construct the following element for [.a., for example:

Attribute(ExprAttribute {
    node_index: AtomicNodeIndex(3),
    range: 1..3,
    value: Name(ExprName { node_index: AtomicNodeIndex(4), range: 1..1, id: Name(""), ctx: Invalid }),
    attr: Identifier { id: Name("a"), range: 2..3, node_index: AtomicNodeIndex(5) },
    ctx: Load
})

This is also not a valid expression and we can immediately return Unbound. Rather, this seems to cause a panic, but it doesn't. Because it is recorded as a place in the UseDefMap. The inner name is invalid, but the attribute is in the load context. Not harmful, but a useless record.

ruff/crates/ty_python_semantic/src/semantic_index/builder.rs

Lines 1945 to 1962 in f15f930

let (is_use, is_definition) = match (ctx, self.current_assignment()) {
(ast::ExprContext::Store, Some(CurrentAssignment::AugAssign(_))) => {
// For augmented assignment, the target expression is also used.
(true, true)
}
(ast::ExprContext::Load, _) => (true, false),
(ast::ExprContext::Store, _) => (false, true),
(ast::ExprContext::Del, _) => (true, true),
(ast::ExprContext::Invalid, _) => (false, false),
};
let place_id = self.add_place(place_expr);
if is_use {
self.mark_place_used(place_id);
let use_id = self.current_ast_ids().record_use(expr);
self.current_use_def_map_mut()
.record_use(place_id, use_id, node_key);
}

It might be better to validate that the inner expressions are also valid when building the SemanticIndex, or to let the parser propagate the invalid context to the outer expressions.

Copy link
Contributor Author

@sharkdp sharkdp Jul 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dhruvmanila Would you mind also having a look at this, in particular the suggestion here to do part of the work in the parser?

Copy link
Member

@dhruvmanila dhruvmanila Jul 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be better to validate that the inner expressions are also valid when building the SemanticIndex, or to let the parser propagate the invalid context to the outer expressions.

I think it makes sense that if an inner expression is invalid then the entire attribute expression is marked as invalid by the parser. This would then be automatically excluded by the semantic index builder. We do related changes using the helpers::set_expr_context function.

Do we only need to account for the top-level invalid expression or any nested invalid expressions as well? For example, in .a, it's only the top level value expression of the attribute expression that's invalid but in foo(1+).bar, it's the inner expression (+1) that's invalid instead of the outer expression foo(...) and the outer expression corresponds to the value field of an attribute expression. I'm assuming it's the former?

Copy link
Contributor

@mtshiba mtshiba Jul 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we only need to account for the top-level invalid expression or any nested invalid expressions as well? For example, in .a, it's only the top level value expression of the attribute expression that's invalid but in foo(1+).bar, it's the inner expression (+1) that's invalid instead of the outer expression foo(...) and the outer expression corresponds to the value field of an attribute expression. I'm assuming it's the former?

The only expressions that should have invalid context propagated are those that are simple enough that PlaceExpr::try_from succeeds.
A complex expression like f(1+).bar is not recorded as a place in the UseDefMap, so the outer attribute f(...).bar can be used as the load context.

Copy link
Member

@dhruvmanila dhruvmanila Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems a bit inconsistent because the parser shouldn't care whether the expression can be constructed by PlaceExpr or not. And, I don't think it really matters, right? Like, if there are any invalid expressions nested in the ExprAttribute, then the entire ExprAttribute is invalid and the parser shouldn't special case only specific expressions. But, if special casing is important, then I'd prefer to have this logic in ty and not in the parser.

Copy link
Member

@MichaReiser MichaReiser Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is changing the context in the parser significantely simpler than doing it in ty? If not, I'd suggest doing it in ty.

Copy link
Member

@dhruvmanila dhruvmanila Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it won't be that simple change in the parser because we'd need to get this information (ExprContext::Invalid) up the AST to the ExprAttribute which could possibly be achieved by storing it in ParsedExpr (is_invalid: bool field) and using it in parse_attribute_expression. But, yeah, it might be simpler to do it in ty instead.

Copy link
Contributor Author

@sharkdp sharkdp Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks everyone. If this really only causes unnecessary work, I'm not sure it's worth doing something here at all. I'll move this PR to in-review to fix the original crash. It comes up easily in the LSP/playground when editing code.


let use_id = expr_ref.scoped_use_id(db, scope);
let place = place_from_bindings(db, use_def.bindings_at_use(use_id));
(place, Some(use_id))
Expand Down
Loading