-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[ty] Fix normalization of unions containing instances parameterized with unions #18112
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
Conversation
|
7321a11
to
6916b39
Compare
6916b39
to
19fe882
Compare
19fe882
to
fb190e8
Compare
Thank you @AlexWaygood. With this PR, you fixed one particular problem in normalization. I'm not trying to imply that my attempt in #18091 is the right solution, but I think I'm not happy with the current state. There is nothing that prevents us from introducing similar problems in the future. And there is no guarantee that other problems already exist. In fact, it's easy to find other cases that are still problematic by embedding unions in types that we don't have any special handling for, yet. A lot of We have seen in astral-sh/ty#369 that this can lead to extremely subtle bugs (maybe we'll be more aware next time, but it took me several hours to find the root cause). So I with there would be some way we could get help from the compiler to get this right. |
@sharkdp yes, I agree! I think there are probably still other latent bugs with normalisation lurking in our codebase already. This PR fixes some specific issues with normalisation, but it doesn't fix the general problem that it's hard to get this approach right. I'd also love to see if we can explore ways to make our design more robust. I liked the idea you mentioned on Discord the other day of having a generalised visitor pattern for recursing into types and applying transformations to all the sub-nodes — it's possible that might make this easier to get right? |
Summary
Foo[int | str]
andFoo[str | int]
have the same Salsa ID when normalizedUnknown
,Todo
andAny
all toAny
. I'll be honest here: I still don't totally understand why this is necessary, but I can say that it makes the nondeterministic behaviour on hydra-zen go away. And I think it may be a good thing to do anyway:This is stacked on top of #18111, because otherwise it causes some small regressions in our
redundant-cast
mdtests because of the new normalisations of dynamic types.Fixes astral-sh/ty#369
Test Plan
main
using that setup)