[red-knot] Diagnostic for conflicting declared attribute types

[sharkdp]

We should emit a diagnostic when we see conflicting declared types of attributes, either between the declaration in the class body and declarations in a method, or between annotated assignments in different methods. We have existing tests for this scenario (see TODO comments):

ruff/crates/red_knot_python_semantic/resources/mdtest/attributes.md

Lines 212 to 226 in eb08345

	class C:
	z: int
	def __init__(self) -> None:
	self.x = get_int()
	self.y: int = 1
	def other_method(self):
	self.x = get_str()
	# TODO: this redeclaration should be an error
	self.y: str = "a"
	# TODO: this redeclaration should be an error
	self.z: str = "a"

part of: #14164

[smokyabdulrahman]

I am not sure if this is helpful, but here is what pyright reports for the mentioned test case:

class C:
    """
    Diagnostics:
    Declaration "z" is obscured by a declaration of the same name [reportRedeclaration]
    """

    z: int

    def __init__(self) -> None:
        self.x = get_int()
        """
        Diagnostics:
        Declaration "y" is obscured by a declaration of the same name [reportRedeclaration]
        """
        self.y: int = 1

    def other_method(self):
        self.x = get_str()

        self.y: str = "a"

        self.z: str = "a"

I am trying to learn/understand the technical stuff behind parsers by reading (books, blogs) and contributing to tools I love that has things to do with parsers (ruff ❤).

is this issue about adding a new rule? or just fixing/refactoring red-knot tests mentioned (modify the markdown file and mention the correct error/reveal messages)?

[sharkdp]

is this issue about adding a new rule?

Yes, this is about emitting a new diagnostic (we can probably reuse conflicting-declarations) when we see something like this.

We do want to report them in the same places where pyright reports "declaration … is obscured by …", but I think we want to use a slightly different wording, as we generally allow re-declarations of symbols:

x: int = 1
x: str = "foo"

What's not allowed though, is to have multiple visible conflicting declarations:

def _(flag: bool):
    if flag:
        x: str
    else:
        x: int

    x = 1  # error: [conflicting-declarations]

For the case mentioned in this issue, we want something similar, but for declarations of attributes.

[carljm]

(To be clear, though, this issue is not about adding a new ruff rule, it's about a new red-knot diagnostic. Although they are in the same codebase, red-knot is separate from ruff and shares only the parser/AST with it.)

[smokyabdulrahman]

Thank you all for the information. All clear.

I will give this issue a shot.

[smokyabdulrahman]

While trying to dive deep into the code, I've found the following comments where I think the report_lint should happen.

ruff/crates/red_knot_python_semantic/src/types.rs

Lines 4323 to 4329 in cb8b23d

	Err((declared_ty, _conflicting_declarations)) => {
	// There are conflicting declarations for this attribute in the class body.
	SymbolAndQualifiers(
	Symbol::bound(declared_ty.inner_type()),
	declared_ty.qualifiers(),
	)
	}

Also, I've found this TODO:

ruff/crates/red_knot_python_semantic/src/types.rs

Lines 4224 to 4237 in cb8b23d

	for attribute_assignment in attribute_assignments {
	match attribute_assignment {
	AttributeAssignment::Annotated { annotation } => {
	// We found an annotated assignment of one of the following forms (using 'self' in these
	// examples, but we support arbitrary names for the first parameters of methods):
	//
	// self.name: <annotation>
	// self.name: <annotation> = …
	let annotation_ty = infer_expression_type(db, *annotation);
	// TODO: check if there are conflicting declarations
	return Symbol::bound(annotation_ty);
	}

However, I believe that type.rs doesn't have the context to report the diagnostic for a reason.

Could you please point me to a similar example where a similar rule had been tackled?

My thought process is to implement some logic in:

ruff/crates/red_knot_python_semantic/src/types/infer.rs

Lines 899 to 904 in cb8b23d

	fn add_declaration(
	&mut self,
	node: AnyNodeRef,
	declaration: Definition<'db>,
	ty: TypeAndQualifiers<'db>,
	) {

and:

ruff/crates/red_knot_python_semantic/src/types/infer.rs

Lines 929 to 934 in cb8b23d

	fn add_declaration_with_binding(
	&mut self,
	node: AnyNodeRef,
	definition: Definition<'db>,
	declared_and_inferred_ty: &DeclaredAndInferredType<'db>,
	) {

or to check here if the target is an instance member of class, and check all the symbol declarations in the scope of the class body, by calling symbol_from_declarations and report the conflicting declarations if any.

ruff/crates/red_knot_python_semantic/src/types/infer.rs

Lines 2121 to 2125 in cb8b23d

	fn infer_annotated_assignment_definition(
	&mut self,
	assignment: &ast::StmtAnnAssign,
	definition: Definition<'db>,
	) {

[sharkdp]

While trying to dive deep into the code, I've found the following comments where I think the report_lint should happen.

This is the place where we would end up if there would be conflicting declarations in the class body. This is just one possible source for declarations of an attribute.

Also, I've found this TODO:

This is the other place, correct. Here, we loop over declarations of attributes in methods. We currently return early, but we probably need to change the loop logic to collect all declared types, and then emit a diagnostic in case there are more than one.

Finally, we also need to consider the option that there is a declaration on the class body, and a conflicting declaration in a method.

--

However, I believe that type.rs doesn't have the context to report the diagnostic for a reason.

We are currently in the process of changing some infrastructure to make this easier. Maybe it would be good to split this task into two parts: (1) collect all conflicting declared types so we would in principle be able to emit a diagnostic — assuming we could do it from anywhere, and (2) actually create a new diagnostic and make the required changes to be able to emit it.

I think you could already start with part 1, and for part 2, it might make sense to wait for those changes to the diagnostics infrastructure.

Let me know if I failed to answer any of the questions that would be required for part 1.

[smokyabdulrahman]

Thank you so much, this made it clearer.
I will try and approach part 1.