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

[red-knot] Find conflicting declarations for attribute in class methods #16240

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

Conversation

smokyabdulrahman
Copy link
Contributor

@smokyabdulrahman smokyabdulrahman commented Feb 18, 2025

Summary

Handling one more case for class attributes: checking for conflicting declarations for class attributes in methods.
part of #15964

Test Plan

No test for now

@MichaReiser MichaReiser added the red-knot Multi-file analysis & type inference label Feb 18, 2025
@MichaReiser MichaReiser changed the title Find conflicting declarations for attribute in class methods [red-not] Find conflicting declarations for attribute in class methods Feb 18, 2025
@MichaReiser MichaReiser changed the title [red-not] Find conflicting declarations for attribute in class methods [red-knot] Find conflicting declarations for attribute in class methods Feb 18, 2025
@MichaReiser MichaReiser marked this pull request as draft February 19, 2025 17:29
@MichaReiser
Copy link
Member

I put this back in draft as it doesn't seem finished.

@sharkdp
Copy link
Contributor

sharkdp commented Feb 19, 2025

I put this back in draft as it doesn't seem finished.

Just for additional context, I suggested splitting the implementation 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.

But even part (1) doesn't seem finished, so keeping this in draft seems fine. @smokyabdulrahman Let us know if you need additional help to proceed here.

@smokyabdulrahman
Copy link
Contributor Author

I put this back in draft as it doesn't seem finished.

Just for additional context, I suggested splitting the implementation 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.

But even part (1) doesn't seem finished, so keeping this in draft seems fine. @smokyabdulrahman Let us know if you need additional help to proceed here.

Hey @sharkdp,
I couldn't think of any other place to tackle and add the logic of collecting conflicting declared types to.
Also, I am not sure how to test for part (1) even in a hacky way.

@sharkdp
Copy link
Contributor

sharkdp commented Feb 20, 2025

I couldn't think of any other place to tackle and add the logic of collecting conflicting declared types to.

We identified two spots in the linked issue. If I understand it correctly, you solved it for one of these, but the more difficult one would be in the change to the loop logic. I can go into more details tomorrow, if that would help.

Also, I am not sure how to test for part (1) even in a hacky way.

The infrastructure for emitting diagnostics from Type::member (and friends) is still not in place. If you're okay with waiting for that, you could still test your implementation by running Red Knot manually on a test file, and "emitting" diagnostics via eprintln! or similar.

The important thing for part 1 would be to get the semantics correct, i.e. to identify two or three test cases that would clearly show that we would be able to generate the right diagnostic messages.

If that sounds too complicated, I'm also okay with leaving that open for a bit longer, and revisit it once the diagnostics infrastructure is in place.

@sharkdp
Copy link
Contributor

sharkdp commented Feb 21, 2025

The infrastructure for emitting diagnostics from Type::member (and friends) is still not in place

This is being tracked in #16298

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants